[xdp-hints] Re: [PATCH bpf-next V1 1/7] xdp: bpf_xdp_metadata use EOPNOTSUPP for no driver support

["Toke Høiland-Jørgensen" <toke@redhat.com>]

Jesper Dangaard Brouer <jbrouer@redhat.com> writes:

> On 17/03/2023 22.21, Stanislav Fomichev wrote:
>> On 03/17, Jesper Dangaard Brouer wrote:
>>> When driver doesn't implement a bpf_xdp_metadata kfunc the fallback
>>> implementation returns EOPNOTSUPP, which indicate device driver doesn't
>>> implement this kfunc.
>> 
>>> Currently many drivers also return EOPNOTSUPP when the hint isn't
>>> available, which is inconsistent from an API point of view. Instead
>>> change drivers to return ENODATA in these cases.
>> 
>>> There can be natural cases why a driver doesn't provide any hardware
>>> info for a specific hint, even on a frame to frame basis (e.g. PTP).
>>> Lets keep these cases as separate return codes.
>> 
>>> When describing the return values, adjust the function kernel-doc layout
>>> to get proper rendering for the return values.
>> 
>>> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
>> 
>> I don't remember whether the previous discussion ended in something?
>> IIRC Martin was preferring to use xdp-features for this instead?
>> 
>
> IIRC Martin asked for a second vote/opinion to settle the vote.
> The xdp-features use is orthogonal and this patch does not prohibit the
> later implementation of xdp-features, to detect if driver doesn't
> implement kfuncs via using global vars.  Not applying this patch leaves
> the API in an strange inconsistent state, because of an argument that in
> the *future* we can use xdp-features to solve *one* of the discussed
> use-cases for another return code.
> I argued for a practical PTP use-case where not all frames contain the
> PTP timestamp.  This patch solve this use-case *now*, so I don't see why
> we should stall solving this, because of a "future" feature we might
> never get around to implement, which require the user to use global vars.
>
>
>> Personally I'm fine with having this convention, but I'm not sure how well
>> we'll be able to enforce them. (In general, I'm not a fan of userspace
>> changing it's behavior based on errno. If it's mostly for
>> debugging/development - seems ok)
>>
>
> We enforce the API by documenting the return behavior, like below.  If a 
> driver violate this, then we will fix the driver code with a fixes tag.
>
> My ask is simply let not have ambiguous return codes.

FWIW I don't get the opposition to this patch: having distinct return
codes strictly increases the amount of information that is available to
the caller. Even if some driver happens to use the "wrong" return code,
it's still an improvement for all the drivers that do the right thing
(and, well, we can fix broken drivers). And if a BPF program doesn't
care about the type of failure they can just ignore treat all error
codes the same; realistically, that is what most programs will do, but
that doesn't mean we can't provide the more-granular error codes to the
programs that do care.

My only concern with this patch is that it targets bpf-next and carries
no Fixes tag, so we'll end up with a kernel release that doesn't have
this change...

-Toke