| Message ID | 167645577609.1860229.12489295285473044895.stgit@firesoul (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next,V1] xdp: bpf_xdp_metadata use NODEV for no device support | expand |
On Wed, Feb 15, 2023 at 11:09:36AM +0100, Jesper Dangaard Brouer wrote: > With our XDP-hints kfunc approach, where individual drivers overload the > default implementation, it can be hard for API users to determine > whether or not the current device driver have this kfunc available. > > Change the default implementations to use an errno (ENODEV), that > drivers shouldn't return, to make it possible for BPF runtime to > determine if bpf kfunc for xdp metadata isn't implemented by driver. I think it diverts ENODEV usage from its original purpose too much. Maybe providing information in dmesg would be a better solution? > > This is intended to ease supporting and troubleshooting setups. E.g. > when users on mailing list report -19 (ENODEV) as an error, then we can > immediately tell them their kernel is too old. Do you mean driver being too old, not kernel? > > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> > --- > net/core/xdp.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/net/core/xdp.c b/net/core/xdp.c > index 26483935b7a4..7bb5984ae4f7 100644 > --- a/net/core/xdp.c > +++ b/net/core/xdp.c > @@ -722,10 +722,12 @@ __diag_ignore_all("-Wmissing-prototypes", > * @timestamp: Return value pointer. > * > * Returns 0 on success or ``-errno`` on error. > + * > + * -ENODEV (19): means device driver doesn't implement kfunc > */ > __bpf_kfunc int bpf_xdp_metadata_rx_timestamp(const struct xdp_md *ctx, u64 *timestamp) > { > - return -EOPNOTSUPP; > + return -ENODEV; > } > > /** > @@ -734,10 +736,12 @@ __bpf_kfunc int bpf_xdp_metadata_rx_timestamp(const struct xdp_md *ctx, u64 *tim > * @hash: Return value pointer. > * > * Returns 0 on success or ``-errno`` on error. > + * > + * -ENODEV (19): means device driver doesn't implement kfunc > */ > __bpf_kfunc int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx, u32 *hash) > { > - return -EOPNOTSUPP; > + return -ENODEV; > } > > __diag_pop(); Documentation contains the following lines: Not all kfuncs have to be implemented by the device driver; when not implemented, the default ones that return ``-EOPNOTSUPP`` will be used. If you decide to proceed with current implementation, you'd need to update them in v2. > >
From: Zaremba, Larysa <larysa.zaremba@intel.com> Date: Wed, 15 Feb 2023 16:45:18 +0100 > On Wed, Feb 15, 2023 at 11:09:36AM +0100, Jesper Dangaard Brouer wrote: >> With our XDP-hints kfunc approach, where individual drivers overload the >> default implementation, it can be hard for API users to determine >> whether or not the current device driver have this kfunc available. >> >> Change the default implementations to use an errno (ENODEV), that >> drivers shouldn't return, to make it possible for BPF runtime to >> determine if bpf kfunc for xdp metadata isn't implemented by driver. > > I think it diverts ENODEV usage from its original purpose too much. Maybe > providing information in dmesg would be a better solution? +1, -%ENODEV shouldn't be used here. It stands for "no device", for example the driver probing core doesn't treat it as an error or that something is not supported (rather than there's no device installed in a slot / on a bus etc.). > >> >> This is intended to ease supporting and troubleshooting setups. E.g. >> when users on mailing list report -19 (ENODEV) as an error, then we can >> immediately tell them their kernel is too old. > > Do you mean driver being too old, not kernel? > >> >> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> >> --- [...] Thanks, Olek
On 15/02/2023 18.11, Alexander Lobakin wrote: > From: Zaremba, Larysa <larysa.zaremba@intel.com> > Date: Wed, 15 Feb 2023 16:45:18 +0100 > >> On Wed, Feb 15, 2023 at 11:09:36AM +0100, Jesper Dangaard Brouer wrote: >>> With our XDP-hints kfunc approach, where individual drivers overload the >>> default implementation, it can be hard for API users to determine >>> whether or not the current device driver have this kfunc available. >>> >>> Change the default implementations to use an errno (ENODEV), that >>> drivers shouldn't return, to make it possible for BPF runtime to >>> determine if bpf kfunc for xdp metadata isn't implemented by driver. >> >> I think it diverts ENODEV usage from its original purpose too much. Can you suggest a errno that is a better fit? >> Maybe providing information in dmesg would be a better solution? IMHO we really don't want to print any information in this code path, as this is being executed as part of the BPF-prog. This will lead to unfortunate latency issues. Also considering the packet rates this need to operate at. > > +1, -%ENODEV shouldn't be used here. It stands for "no device", for > example the driver probing core doesn't treat it as an error or that > something is not supported (rather than there's no device installed > in a slot / on a bus etc.). > I wanted to choose something that isn't natural for a device driver developer to choose as a return code. I choose the "no device", because the "device" driver doesn't implement this. The important part is help ourselves (and support) to reliably determine if a device driver implements this kfunc or not. I'm not married to the specific errno. I hit this issue myself, when developing these kfuncs for igc. I was constantly loading and unloading the driver while developing this. And my kfunc would return -EOPNOTSUPP in some cases, and I couldn't understand why my code changes was not working, but in reality I was hitting the default kfunc implementation as it wasn't the correct version of the driver I had loaded. It would in practice have save me time while developing... Please suggest a better errno if the color is important to you. >> >>> >>> This is intended to ease supporting and troubleshooting setups. E.g. >>> when users on mailing list report -19 (ENODEV) as an error, then we can >>> immediately tell them their kernel is too old. >> >> Do you mean driver being too old, not kernel? Sure I guess, I do mean the driver version. I guess you are thinking in the lines of Intel customers compiling Intel out-of-tree kernel modules, this will also be practical and ease troubleshooting for Intel support teams. >>> >>> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> >>> --- > [...] > > Thanks, > Olek >
From: Jesper Dangaard Brouer <jbrouer@redhat.com> Date: Wed, 15 Feb 2023 18:50:10 +0100 > > On 15/02/2023 18.11, Alexander Lobakin wrote: >> From: Zaremba, Larysa <larysa.zaremba@intel.com> >> Date: Wed, 15 Feb 2023 16:45:18 +0100 [...] >>> I think it diverts ENODEV usage from its original purpose too much. > > Can you suggest a errno that is a better fit? > >>> Maybe providing information in dmesg would be a better solution? > > IMHO we really don't want to print any information in this code path, as > this is being executed as part of the BPF-prog. This will lead to > unfortunate latency issues. Also considering the packet rates this need > to operate at. > >> >> +1, -%ENODEV shouldn't be used here. It stands for "no device", for >> example the driver probing core doesn't treat it as an error or that >> something is not supported (rather than there's no device installed >> in a slot / on a bus etc.). >> > > I wanted to choose something that isn't natural for a device driver > developer to choose as a return code. I choose the "no device", because > the "device" driver doesn't implement this. > > The important part is help ourselves (and support) to reliably determine > if a device driver implements this kfunc or not. I'm not married to the > specific errno. > > I hit this issue myself, when developing these kfuncs for igc. I was > constantly loading and unloading the driver while developing this. And > my kfunc would return -EOPNOTSUPP in some cases, and I couldn't > understand why my code changes was not working, but in reality I was > hitting the default kfunc implementation as it wasn't the correct > version of the driver I had loaded. It would in practice have save me > time while developing... So you suggest to pick the properly wrong errno only to make the life of developers who messed up something in their code a bit easier? I see no issues with using -%EOPNOTSUPP in every case when the driver can't provide BPF prog with the hints requested by it. What you suggest is basically something that we usually do locally to test WIP stuff at early stages. > > Please suggest a better errno if the color is important to you. > >>> >>>> >>>> This is intended to ease supporting and troubleshooting setups. E.g. >>>> when users on mailing list report -19 (ENODEV) as an error, then we can >>>> immediately tell them their kernel is too old. >>> >>> Do you mean driver being too old, not kernel? > > Sure I guess, I do mean the driver version. > > I guess you are thinking in the lines of Intel customers compiling Intel > out-of-tree kernel modules, this will also be practical and ease > troubleshooting for Intel support teams. The last thing our team thinks of is the Intel customers using out-of-tree drivers xD > >>>> >>>> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> >>>> --- >> [...] >> >> Thanks, >> Olek >> > Thanks, Olek
On Wed, Feb 15, 2023 at 06:50:10PM +0100, Jesper Dangaard Brouer wrote: > > On 15/02/2023 18.11, Alexander Lobakin wrote: > > From: Zaremba, Larysa <larysa.zaremba@intel.com> > > Date: Wed, 15 Feb 2023 16:45:18 +0100 > > > > > On Wed, Feb 15, 2023 at 11:09:36AM +0100, Jesper Dangaard Brouer wrote: > > > > With our XDP-hints kfunc approach, where individual drivers overload the > > > > default implementation, it can be hard for API users to determine > > > > whether or not the current device driver have this kfunc available. > > > > > > > > Change the default implementations to use an errno (ENODEV), that > > > > drivers shouldn't return, to make it possible for BPF runtime to > > > > determine if bpf kfunc for xdp metadata isn't implemented by driver. > > > > > > I think it diverts ENODEV usage from its original purpose too much. > > Can you suggest a errno that is a better fit? EOPNOTSUPP fits just fine. > > > > Maybe providing information in dmesg would be a better solution? > > IMHO we really don't want to print any information in this code path, as > this is being executed as part of the BPF-prog. This will lead to > unfortunate latency issues. Also considering the packet rates this need > to operate at. I meant printing messages at bpf program load time... When driver functions are patched-in, you have all the information you may need to inform user, if the default implementation for a particular function is used instead. > > > > > +1, -%ENODEV shouldn't be used here. It stands for "no device", for > > example the driver probing core doesn't treat it as an error or that > > something is not supported (rather than there's no device installed > > in a slot / on a bus etc.). > > > > I wanted to choose something that isn't natural for a device driver > developer to choose as a return code. I choose the "no device", because > the "device" driver doesn't implement this. > > The important part is help ourselves (and support) to reliably determine > if a device driver implements this kfunc or not. I'm not married to the > specific errno. > > I hit this issue myself, when developing these kfuncs for igc. I was > constantly loading and unloading the driver while developing this. And > my kfunc would return -EOPNOTSUPP in some cases, and I couldn't > understand why my code changes was not working, but in reality I was > hitting the default kfunc implementation as it wasn't the correct > version of the driver I had loaded. It would in practice have save me > time while developing... > > Please suggest a better errno if the color is important to you. > > > > > > > > > > > > This is intended to ease supporting and troubleshooting setups. E.g. > > > > when users on mailing list report -19 (ENODEV) as an error, then we can > > > > immediately tell them their kernel is too old. > > > > > > Do you mean driver being too old, not kernel? > > Sure I guess, I do mean the driver version. > > I guess you are thinking in the lines of Intel customers compiling Intel > out-of-tree kernel modules, this will also be practical and ease > troubleshooting for Intel support teams. > > > > > > > > > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> > > > > --- > > [...] > > > > Thanks, > > Olek > > >
Larysa Zaremba <larysa.zaremba@intel.com> writes: > On Wed, Feb 15, 2023 at 06:50:10PM +0100, Jesper Dangaard Brouer wrote: >> >> On 15/02/2023 18.11, Alexander Lobakin wrote: >> > From: Zaremba, Larysa <larysa.zaremba@intel.com> >> > Date: Wed, 15 Feb 2023 16:45:18 +0100 >> > >> > > On Wed, Feb 15, 2023 at 11:09:36AM +0100, Jesper Dangaard Brouer wrote: >> > > > With our XDP-hints kfunc approach, where individual drivers overload the >> > > > default implementation, it can be hard for API users to determine >> > > > whether or not the current device driver have this kfunc available. >> > > > >> > > > Change the default implementations to use an errno (ENODEV), that >> > > > drivers shouldn't return, to make it possible for BPF runtime to >> > > > determine if bpf kfunc for xdp metadata isn't implemented by driver. >> > > >> > > I think it diverts ENODEV usage from its original purpose too much. >> >> Can you suggest a errno that is a better fit? > > EOPNOTSUPP fits just fine. An alternative to changing the return code of the default kfuncs is also to just not have the driver functions themselves use that error code? :) >> > > Maybe providing information in dmesg would be a better solution? >> >> IMHO we really don't want to print any information in this code path, as >> this is being executed as part of the BPF-prog. This will lead to >> unfortunate latency issues. Also considering the packet rates this need >> to operate at. > > I meant printing messages at bpf program load time... > When driver functions are patched-in, you have all the information you may need > to inform user, if the default implementation for a particular function is used > instead. If you dump the byte code with bpftool (using `bpftool prog dump xlated`), the name of the function being called will be in the output, which is also a way to detect if the driver kfunc is being called... -Toke
diff --git a/net/core/xdp.c b/net/core/xdp.c index 26483935b7a4..7bb5984ae4f7 100644 --- a/net/core/xdp.c +++ b/net/core/xdp.c @@ -722,10 +722,12 @@ __diag_ignore_all("-Wmissing-prototypes", * @timestamp: Return value pointer. * * Returns 0 on success or ``-errno`` on error. + * + * -ENODEV (19): means device driver doesn't implement kfunc */ __bpf_kfunc int bpf_xdp_metadata_rx_timestamp(const struct xdp_md *ctx, u64 *timestamp) { - return -EOPNOTSUPP; + return -ENODEV; } /** @@ -734,10 +736,12 @@ __bpf_kfunc int bpf_xdp_metadata_rx_timestamp(const struct xdp_md *ctx, u64 *tim * @hash: Return value pointer. * * Returns 0 on success or ``-errno`` on error. + * + * -ENODEV (19): means device driver doesn't implement kfunc */ __bpf_kfunc int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx, u32 *hash) { - return -EOPNOTSUPP; + return -ENODEV; } __diag_pop();
With our XDP-hints kfunc approach, where individual drivers overload the default implementation, it can be hard for API users to determine whether or not the current device driver have this kfunc available. Change the default implementations to use an errno (ENODEV), that drivers shouldn't return, to make it possible for BPF runtime to determine if bpf kfunc for xdp metadata isn't implemented by driver. This is intended to ease supporting and troubleshooting setups. E.g. when users on mailing list report -19 (ENODEV) as an error, then we can immediately tell them their kernel is too old. Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> --- net/core/xdp.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)