[xdp-hints] [PATCH bpf-next V1] xdp: bpf_xdp_metadata use NODEV for no device support

[xdp-hints] [PATCH bpf-next V1] xdp: bpf_xdp_metadata use NODEV for no device support

[Jesper Dangaard Brouer]

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(-)

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();

[xdp-hints] Re: [PATCH bpf-next V1] xdp: bpf_xdp_metadata use NODEV for no device support

[Larysa Zaremba]

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.

> 
> 

[xdp-hints] Re: [PATCH bpf-next V1] xdp: bpf_xdp_metadata use NODEV for no device support

[Alexander Lobakin]

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

[xdp-hints] Re: [PATCH bpf-next V1] xdp: bpf_xdp_metadata use NODEV for no device support

[Jesper Dangaard Brouer]

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
> 

[xdp-hints] Re: [PATCH bpf-next V1] xdp: bpf_xdp_metadata use NODEV for no device support

[Alexander Lobakin]

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

[xdp-hints] Re: [PATCH bpf-next V1] xdp: bpf_xdp_metadata use NODEV for no device support

[Larysa Zaremba]

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
> > 
> 

[xdp-hints] Re: [PATCH bpf-next V1] xdp: bpf_xdp_metadata use NODEV for no device support

[Toke Høiland-Jørgensen]

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