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

[Jesper Dangaard Brouer <jbrouer@redhat.com>]

On 21/03/2023 13.24, Toke Høiland-Jørgensen wrote:
> 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...
> 

Good point, I'll send this patch against 'bpf' tree instead.

--Jesper