Date:   Thu, 23 Feb 2023 23:44:55 -0800
From:   Martin KaFai Lau <martin.lau@...ux.dev>
To:     Jesper Dangaard Brouer <jbrouer@...hat.com>
Cc:     brouer@...hat.com, bpf@...r.kernel.org, netdev@...r.kernel.org,
        martin.lau@...nel.org, ast@...nel.org, daniel@...earbox.net,
        alexandr.lobakin@...el.com, larysa.zaremba@...el.com,
        xdp-hints@...-project.net, Stanislav Fomichev <sdf@...gle.com>
Subject: Re: [PATCH bpf-next V3] xdp: bpf_xdp_metadata use EOPNOTSUPP for no
 driver support

On 2/22/23 1:49 PM, Jesper Dangaard Brouer wrote:
> 
> On 21/02/2023 22.58, Martin KaFai Lau wrote:
>> On 2/21/23 12:39 PM, Jesper Dangaard Brouer wrote:
>>> For me this is more about the API we are giving the BPF-programmer.
>>>
>>> There can be natural cases why a driver doesn't provide any hardware
>>> info for a specific hint.  The RX-timestamp is a good practical example,
>>> as often only PTP packets will be timestamped by hardware.
>>>
>>> I can write a BPF-prog that create a stats-map for counting
>>> RX-timestamps, expecting to catch any PTP packets with timestamps.  The
>>> problem is my stats-map cannot record the difference of EOPNOTSUPP vs
>>> ENODATA.  Thus, the user of my RX-timestamps stats program can draw the
>>> wrong conclusion, that there are no packets with (PTP) timestamps, when
>>> this was actually a case of driver not implementing this.
>>>
>>> I hope this simple stats example make is clearer that the BPF-prog can
>>> make use of this info runtime.  It is simply a question of keeping these
>>> cases as separate return codes. Is that too much to ask for from an API?
>>
>> Instead of reserving an errno for this purpose, it can be decided at load time 
>> instead of keep calling a kfunc always returning the same dedicated errno. I 
>> still don't hear why xdp-features + bpf global const won't work.
>>
> 
> Sure, exposing this to xdp-features and combining this with a bpf global
> const is a cool idea, slightly extensive work for the BPF-programmer,
> but sure BPF is all about giving the BPF programmer flexibility.
> 
> I do feel it is orthogonal whether the API should return a consistent
> errno when the driver doesn't implement the kfunc.
> 
> I'm actually hoping in the future that we can achieve dead code
> elimination automatically without having to special case this.
> When we do Stanislav's BPF unroll tricks we get a constant e.g.
> EOPNOTSUPP when driver doesn't implement the kfunc.  This should allow
> the verifier to do deadcode elimination right?
> 
> For my stats example, where I want to count both packets with and
> without timestamps, but not miscount packets that actually had a
> timestamp, but my driver just doesn't support querying this.
> 
> Consider program-A:
> 
>   int err = bpf_xdp_metadata_rx_timestamp(ctx, &ts);
>   if (!err) {
>      ts_stats[HAVE_TS]++;
>   } else {
>      ts_stats[NO_TS_DATA]++;
>   }
> 
> Program-A clearly does the miscount issue. The const propagation and
> deadcode code elimination would work, but is still miscounts.
> Yes, program-A could be extended with the cool idea of xdp-feature
> detection that updates a prog const, for solving the issue.
> 
> Consider program-B:
> 
>   int err = bpf_xdp_metadata_rx_timestamp(ctx, &ts);
>   if (!err) {
>      ts_stats[HAVE_TS]++;
>   } else if (err == -ENODATA) {
>      ts_stats[NO_TS_DATA]++;
>   }
> 
> If I had a separate return, then I can avoid the miscount as demonstrate
> in program-B.  In this program the const propagation and deadcode
> elimination would *also* work and still avoid the miscounts.  It should
> elimination any updates to ts_stats map.
> 
> I do get the cool idea of bpf global const, but we will hopefully get
> this automatically when we can do BPF unroll.

I think the direction is to dual compile a kfunc to native code and bpf code and 
to get away from the manual unroll or hand written bpf insn. Not sure if the 
verifier can (and should) further check whether a compiled bpf subprog always 
returns a const scalar to optimize this particular case.

I think enough words have been exchanged on this subject. A few ways (eg. at 
load time) have been suggested to detect it without reserving an errno for an 
empty function. Beside, it is hard to miss when the stats is all one sided if 
the driver does not implement a xdp-hint. Quickly query the xdp-feature will 
confirm it. I assume ethtool will be able to check that soon also. It is what 
xdp-feature is for instead of reserving a run time value to detect if a driver 
has implemented each individual xdp feature.

May be a tie break vote is needed.

Powered by blists - more mailing lists