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

[xdp-hints] [PATCH bpf-next V2] 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 device driver is too old.

Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---
 Documentation/networking/xdp-rx-metadata.rst |    3 ++-
 net/core/xdp.c                               |    8 ++++++--
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/Documentation/networking/xdp-rx-metadata.rst b/Documentation/networking/xdp-rx-metadata.rst
index aac63fc2d08b..89f6a7d1be38 100644
--- a/Documentation/networking/xdp-rx-metadata.rst
+++ b/Documentation/networking/xdp-rx-metadata.rst
@@ -26,7 +26,8 @@ consumers, an XDP program can store it into the metadata area carried
 ahead of the packet.
 
 Not all kfuncs have to be implemented by the device driver; when not
-implemented, the default ones that return ``-EOPNOTSUPP`` will be used.
+implemented, the default ones that return ``-ENODEV`` will be used to
+indicate the device driver have not implemented this kfunc.
 
 Within an XDP frame, the metadata layout (accessed via ``xdp_buff``) is
 as follows::
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 V2] xdp: bpf_xdp_metadata use NODEV for no device support

[Stanislav Fomichev]

On 02/17, 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.

> 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 device driver is too old.

I agree with the v1 comments that I'm not sure how it helps.
Why can't we update the doc in the same fashion and say that
the drivers shouldn't return EOPNOTSUPP?

I'm fine with the change if you think it makes your/users life
easier. Although I don't really understand how. We can, as Toke
mentioned, ask the users to provide jited program dump if it's
mostly about user reports.

> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> ---
>   Documentation/networking/xdp-rx-metadata.rst |    3 ++-
>   net/core/xdp.c                               |    8 ++++++--
>   2 files changed, 8 insertions(+), 3 deletions(-)

> diff --git a/Documentation/networking/xdp-rx-metadata.rst  
> b/Documentation/networking/xdp-rx-metadata.rst
> index aac63fc2d08b..89f6a7d1be38 100644
> --- a/Documentation/networking/xdp-rx-metadata.rst
> +++ b/Documentation/networking/xdp-rx-metadata.rst
> @@ -26,7 +26,8 @@ consumers, an XDP program can store it into the  
> metadata area carried
>   ahead of the packet.

>   Not all kfuncs have to be implemented by the device driver; when not
> -implemented, the default ones that return ``-EOPNOTSUPP`` will be used.
> +implemented, the default ones that return ``-ENODEV`` will be used to
> +indicate the device driver have not implemented this kfunc.

>   Within an XDP frame, the metadata layout (accessed via ``xdp_buff``) is
>   as follows::
> 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 V2] xdp: bpf_xdp_metadata use NODEV for no device support

[Martin KaFai Lau]

On 2/17/23 9:32 AM, Stanislav Fomichev wrote:
> On 02/17, 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.
> 
>> 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 device driver is too old.
> 
> I agree with the v1 comments that I'm not sure how it helps.
> Why can't we update the doc in the same fashion and say that
> the drivers shouldn't return EOPNOTSUPP?
> 
> I'm fine with the change if you think it makes your/users life
> easier. Although I don't really understand how. We can, as Toke
> mentioned, ask the users to provide jited program dump if it's
> mostly about user reports.

and there is xdp-features also.

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

[Stanislav Fomichev]

On Fri, Feb 17, 2023 at 9:39 AM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>
> On 2/17/23 9:32 AM, Stanislav Fomichev wrote:
> > On 02/17, 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.
> >
> >> 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 device driver is too old.
> >
> > I agree with the v1 comments that I'm not sure how it helps.
> > Why can't we update the doc in the same fashion and say that
> > the drivers shouldn't return EOPNOTSUPP?
> >
> > I'm fine with the change if you think it makes your/users life
> > easier. Although I don't really understand how. We can, as Toke
> > mentioned, ask the users to provide jited program dump if it's
> > mostly about user reports.
>
> and there is xdp-features also.

Yeah, I was going to suggest it, but then I wasn't sure how to
reconcile our 'kfunc is not a uapi' with xdp-features (that probably
is a uapi)?

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

[Martin KaFai Lau]

On 2/17/23 9:40 AM, Stanislav Fomichev wrote:
> On Fri, Feb 17, 2023 at 9:39 AM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>>
>> On 2/17/23 9:32 AM, Stanislav Fomichev wrote:
>>> On 02/17, 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.
>>>
>>>> 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 device driver is too old.
>>>
>>> I agree with the v1 comments that I'm not sure how it helps.
>>> Why can't we update the doc in the same fashion and say that
>>> the drivers shouldn't return EOPNOTSUPP?
>>>
>>> I'm fine with the change if you think it makes your/users life
>>> easier. Although I don't really understand how. We can, as Toke
>>> mentioned, ask the users to provide jited program dump if it's
>>> mostly about user reports.
>>
>> and there is xdp-features also.
> 
> Yeah, I was going to suggest it, but then I wasn't sure how to
> reconcile our 'kfunc is not a uapi' with xdp-features (that probably
> is a uapi)?

uapi concern is a bit in xdp-features may go away because the kfunc may go away ?

May be a list of xdp kfunc names that it supports? A list of kfunc btf id will 
do also and the user space will need to map it back. Not sure if it is easily 
doable in xdp-features.

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

[Stanislav Fomichev]

On Fri, Feb 17, 2023 at 9:55 AM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>
> On 2/17/23 9:40 AM, Stanislav Fomichev wrote:
> > On Fri, Feb 17, 2023 at 9:39 AM Martin KaFai Lau <martin.lau@linux.dev> wrote:
> >>
> >> On 2/17/23 9:32 AM, Stanislav Fomichev wrote:
> >>> On 02/17, 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.
> >>>
> >>>> 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 device driver is too old.
> >>>
> >>> I agree with the v1 comments that I'm not sure how it helps.
> >>> Why can't we update the doc in the same fashion and say that
> >>> the drivers shouldn't return EOPNOTSUPP?
> >>>
> >>> I'm fine with the change if you think it makes your/users life
> >>> easier. Although I don't really understand how. We can, as Toke
> >>> mentioned, ask the users to provide jited program dump if it's
> >>> mostly about user reports.
> >>
> >> and there is xdp-features also.
> >
> > Yeah, I was going to suggest it, but then I wasn't sure how to
> > reconcile our 'kfunc is not a uapi' with xdp-features (that probably
> > is a uapi)?
>
> uapi concern is a bit in xdp-features may go away because the kfunc may go away ?

Yeah, if it's another kind of bitmask we'd have to retain those bits
(in case of a particular kfunc ever going away)..

> May be a list of xdp kfunc names that it supports? A list of kfunc btf id will
> do also and the user space will need to map it back. Not sure if it is easily
> doable in xdp-features.

Good point. A string list / btf_id list of kfuncs implemented by
netdev might be a good alternative.

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

[Toke Høiland-Jørgensen]

Stanislav Fomichev <sdf@google.com> writes:

> On Fri, Feb 17, 2023 at 9:55 AM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>>
>> On 2/17/23 9:40 AM, Stanislav Fomichev wrote:
>> > On Fri, Feb 17, 2023 at 9:39 AM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>> >>
>> >> On 2/17/23 9:32 AM, Stanislav Fomichev wrote:
>> >>> On 02/17, 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.
>> >>>
>> >>>> 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 device driver is too old.
>> >>>
>> >>> I agree with the v1 comments that I'm not sure how it helps.
>> >>> Why can't we update the doc in the same fashion and say that
>> >>> the drivers shouldn't return EOPNOTSUPP?
>> >>>
>> >>> I'm fine with the change if you think it makes your/users life
>> >>> easier. Although I don't really understand how. We can, as Toke
>> >>> mentioned, ask the users to provide jited program dump if it's
>> >>> mostly about user reports.
>> >>
>> >> and there is xdp-features also.
>> >
>> > Yeah, I was going to suggest it, but then I wasn't sure how to
>> > reconcile our 'kfunc is not a uapi' with xdp-features (that probably
>> > is a uapi)?
>>
>> uapi concern is a bit in xdp-features may go away because the kfunc may go away ?
>
> Yeah, if it's another kind of bitmask we'd have to retain those bits
> (in case of a particular kfunc ever going away)..
>
>> May be a list of xdp kfunc names that it supports? A list of kfunc btf id will
>> do also and the user space will need to map it back. Not sure if it is easily
>> doable in xdp-features.
>
> Good point. A string list / btf_id list of kfuncs implemented by
> netdev might be a good alternative.

Yup, Lorenzo and I discussed something similar at one point, I think
having this as part of the feature thing would be useful!

-Toke

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

[Jesper Dangaard Brouer]

On 17/02/2023 21.49, Toke Høiland-Jørgensen wrote:
> Stanislav Fomichev <sdf@google.com> writes:
> 
>> On Fri, Feb 17, 2023 at 9:55 AM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>>>
>>> On 2/17/23 9:40 AM, Stanislav Fomichev wrote:
>>>> On Fri, Feb 17, 2023 at 9:39 AM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>>>>>
>>>>> On 2/17/23 9:32 AM, Stanislav Fomichev wrote:
>>>>>> On 02/17, 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.
>>>>>>
>>>>>>> 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 device driver is too old.
>>>>>>
>>>>>> I agree with the v1 comments that I'm not sure how it helps.
>>>>>> Why can't we update the doc in the same fashion and say that
>>>>>> the drivers shouldn't return EOPNOTSUPP?

Okay, lets go in this direction then.
I will update the drivers to not return EOPNOTSUPP.

What should drivers then return instead.
I will propose that driver return ENODATA instead?

--Jesper