Re: [xdp-hints] Re: [PATCH bpf-next v2 12/20] xdp: Add checksum level hint

[Alexander Lobakin <aleksander.lobakin@intel.com>]

From: Larysa Zaremba <larysa.zaremba@intel.com>
Date: Thu, 6 Jul 2023 14:49:44 +0200

> On Thu, Jul 06, 2023 at 02:38:33PM +0200, Larysa Zaremba wrote:
>> On Thu, Jul 06, 2023 at 11:04:49AM +0200, Jesper Dangaard Brouer wrote:
>>>
>>>
>>> On 06/07/2023 07.50, John Fastabend wrote:
>>>> Larysa Zaremba wrote:
>>>>> On Tue, Jul 04, 2023 at 12:39:06PM +0200, Jesper Dangaard Brouer wrote:
>>>>>> Cc. DaveM+Alex Duyck, as I value your insights on checksums.

[...]

>>>>>>>>> + * Return:
>>>>>>>>> + * * Returns 0 on success or ``-errno`` on error.
>>>>>>>>> + * * ``-EOPNOTSUPP`` : device driver doesn't implement kfunc
>>>>>>>>> + * * ``-ENODATA``    : Checksum was not validated
>>>>>>>>> + */
>>>>>>>>> +__bpf_kfunc int bpf_xdp_metadata_rx_csum_lvl(const struct xdp_md *ctx, u8 *csum_level)
>>>>>>>>
>>>>>>>> Istead of ENODATA should we return what would be put in the ip_summed field
>>>>>>>> CHECKSUM_{NONE, UNNECESSARY, COMPLETE, PARTIAL}? Then sig would be,
>>>>>>
>>>>>> I was thinking the same, what about checksum "type".
>>>>>>
>>>>>>>>
>>>>>>>>    bpf_xdp_metadata_rx_csum_lvl(const struct xdp_md *ctx, u8 *type, u8 *lvl);
>>>>>>>>
>>>>>>>> or something like that? Or is the thought that its not really necessary?
>>>>>>>> I don't have a strong preference but figured it was worth asking.
>>>>>>>>
>>>>>>>
>>>>>>> I see no value in returning CHECKSUM_COMPLETE without the actual checksum value.
>>>>>>> Same with CHECKSUM_PARTIAL and csum_start. Returning those values too would
>>>>>>> overcomplicate the function signature.
>>>>>>
>>>>>> So, this kfunc bpf_xdp_metadata_rx_csum_lvl() success is it equivilent to
>>>>>> CHECKSUM_UNNECESSARY?
>>>>>
>>>>> This is 100% true for physical NICs, it's more complicated for veth, bacause it
>>>>> often receives CHECKSUM_PARTIAL, which shouldn't normally apprear on RX, but is
>>>>> treated by the network stack as a validated checksum, because there is no way
>>>>> internally generated packet could be messed up. I would be grateful if you could
>>>>> look at the veth patch and share your opinion about this.
>>>>>
>>>>>>
>>>>>> Looking at documentation[1] (generated from skbuff.h):
>>>>>>   [1] https://kernel.org/doc/html/latest/networking/skbuff.html#checksumming-of-received-packets-by-device
>>>>>>
>>>>>> Is the idea that we can add another kfunc (new signature) than can deal
>>>>>> with the other types of checksums (in a later kernel release)?
>>>>>>
>>>>>
>>>>> Yes, that is the idea.
>>>>
>>>> If we think there is a chance we might need another kfunc we should add it
>>>> in the same kfunc. It would be unfortunate to have to do two kfuncs when
>>>> one would work. It shouldn't cost much/anything(?) to hardcode the type for
>>>> most cases? I think if we need it later I would advocate for updating this
>>>> kfunc to support it. Of course then userspace will have to swivel on the
>>>> kfunc signature.
>>>>
>>>
>>> I think it might make sense to have 3 kfuncs for checksumming.

Isn't that overcomplicating? 3 callbacks for just one damn thing. IOW I
agree with John.

PARTIAL and COMPLETE are mutually exclusive. Their "additional" output
can be unionized. Level is 2 bits, status is 2 bits. Level makes sense
only with UNNECESSARY (correct me if I'm wrong).
IOW the kfunc could return:

-errno - not implemented or something went wrong
0 - none
1 - complete
2 - partial
3 + lvl - unnecessary

(CHECKSUM_* defs could be shuffled accordingly)

Then `if (ret > 2)` would mean UNNECESSARY and most programs could stop
here already. Programs wanting to extract the level can do `ret - 3`.
One additional pointer to u32 (union) to fetch additional data. I would
even say "BPF prog can pass NULL if it doesn't care", but OTOH I dunno
how to validate PARTIAL then :D (COMPLETE usually assumes it's valid)

>>> As this would allow BPF-prog to focus on CHECKSUM_UNNECESSARY, and then
>>> only call additional kfunc for extracting e.g csum_start  + csum_offset
>>> when type is CHECKSUM_PARTIAL.
>>>
>>> We could extend bpf_xdp_metadata_rx_csum_lvl() to give the csum_type
>>> CHECKSUM_{NONE, UNNECESSARY, COMPLETE, PARTIAL}.
>>>
>>>  int bpf_xdp_metadata_rx_csum_lvl(*ctx, u8 *csum_level, u8 *csum_type)
>>>
>>> And then add two kfunc e.g.
>>>  (1) bpf_xdp_metadata_rx_csum_partial(ctx, start, offset)
>>>  (2) bpf_xdp_metadata_rx_csum_complete(ctx, csum)
>>>
>>> Pseudo BPF-prog code:
>>>
>>>  err = bpf_xdp_metadata_rx_csum_lvl(ctx, level, type);
>>>  if (!err && type != CHECKSUM_UNNECESSARY) {

And hurt cool HW which by default returns COMPLETE? }:>

>>>      if (type == CHECKSUM_PARTIAL)
>>>          err = bpf_xdp_metadata_rx_csum_partial(ctx, start, offset);
>>>      if (type == CHECKSUM_COMPLETE)
>>>          err = bpf_xdp_metadata_rx_csum_complete(ctx, csum);

I don't feel like 1 hotpath `if` is worth multiplying kfuncs.

[...]

Thanks,
Olek