Re: [PATCH bpf-next v3 03/12] bpf: XDP metadata RX kfuncs

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

On 09/12/2022 18.47, Stanislav Fomichev wrote:
> On Fri, Dec 9, 2022 at 3:11 AM Jesper Dangaard Brouer
> <jbrouer@redhat.com> wrote:
>>
>>
>> On 06/12/2022 03.45, Stanislav Fomichev wrote:
>>> There is an ndo handler per kfunc, the verifier replaces a call to the
>>> generic kfunc with a call to the per-device one.
>>>
>>> For XDP, we define a new kfunc set (xdp_metadata_kfunc_ids) which
>>> implements all possible metatada kfuncs. Not all devices have to
>>> implement them. If kfunc is not supported by the target device,
>>> the default implementation is called instead.
>>>
>>> Upon loading, if BPF_F_XDP_HAS_METADATA is passed via prog_flags,
>>> we treat prog_index as target device for kfunc resolution.
>>>
>>
>> [...cut...]
>>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>>> index 5aa35c58c342..2eabb9157767 100644
>>> --- a/include/linux/netdevice.h
>>> +++ b/include/linux/netdevice.h
>>> @@ -74,6 +74,7 @@ struct udp_tunnel_nic_info;
>>>    struct udp_tunnel_nic;
>>>    struct bpf_prog;
>>>    struct xdp_buff;
>>> +struct xdp_md;
>>>
>>>    void synchronize_net(void);
>>>    void netdev_set_default_ethtool_ops(struct net_device *dev,
>>> @@ -1611,6 +1612,10 @@ struct net_device_ops {
>>>        ktime_t                 (*ndo_get_tstamp)(struct net_device *dev,
>>>                                                  const struct skb_shared_hwtstamps *hwtstamps,
>>>                                                  bool cycles);
>>> +     bool                    (*ndo_xdp_rx_timestamp_supported)(const struct xdp_md *ctx);
>>> +     u64                     (*ndo_xdp_rx_timestamp)(const struct xdp_md *ctx);
>>> +     bool                    (*ndo_xdp_rx_hash_supported)(const struct xdp_md *ctx);
>>> +     u32                     (*ndo_xdp_rx_hash)(const struct xdp_md *ctx);
>>>    };
>>>
>>
>> Would it make sense to add a 'flags' parameter to ndo_xdp_rx_timestamp
>> and ndo_xdp_rx_hash ?
>>
>> E.g. we could have a "STORE" flag that asks the kernel to store this
>> information for later. This will be helpful for both the SKB and
>> redirect use-cases.
>> For redirect e.g into a veth, then BPF-prog can use the same function
>> bpf_xdp_metadata_rx_hash() to receive the RX-hash, as it can obtain the
>> "stored" value (from the BPF-prog that did the redirect).
>>
>> (p.s. Hopefully a const 'flags' variable can be optimized when unrolling
>> to eliminate store instructions when flags==0)
> 
> Are we concerned that doing this without a flag and with another
> function call will be expensive?

Yes, but if we can unroll (to avoid the function calls) it would be more
flexible and explicit API with below instead.

> For xdp->skb path, I was hoping we would be to do something like:
> 
> timestamp = bpf_xdp_metadata_rx_hash(ctx);
> bpf_xdp_metadata_export_rx_hash_to_skb(ctx, timestamp);
> 
> This should also let the users adjust the metadata before storing it.
> Am I missing something here? Why would the flag be preferable?

I do like this ability to let the users adjust the metadata before
storing it.  This would be a more flexible API for the BPF-programmer.
I like your "export" suggestion.  The main concern for me was
performance overhead of the extra function call, which I guess can be
removed via unrolling later.
Unrolling these 'export' functions might be easier to accept from a
maintainer perspective, as it is not device driver specific, thus we can
place that in the core BPF code.

--Jesper