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.
