Message-ID: <bff4d5eb-fe4d-786e-f41d-1c45f07a7282@redhat.com>
Date:   Wed, 22 Feb 2023 22:49:58 +0100
From:   Jesper Dangaard Brouer <jbrouer@...hat.com>
To:     Martin KaFai Lau <martin.lau@...ux.dev>,
        Jesper Dangaard Brouer <jbrouer@...hat.com>,
        Stanislav Fomichev <sdf@...gle.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
Subject: Re: [PATCH bpf-next V3] xdp: bpf_xdp_metadata use EOPNOTSUPP for no
 driver support


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 hope this make it more clear, why I think it is valuable to "reserve"
an errno for the case when kfunc isn't implemented by driver.

Thanks for reading this far,
--Jesper

Powered by blists - more mailing lists