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

[Stanislav Fomichev <sdf@google.com>]

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?
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?


> >   /**
> > diff --git a/include/net/xdp.h b/include/net/xdp.h
> > index 55dbc68bfffc..c24aba5c363b 100644
> > --- a/include/net/xdp.h
> > +++ b/include/net/xdp.h
> > @@ -409,4 +409,33 @@ void xdp_attachment_setup(struct xdp_attachment_info *info,
> >
> >   #define DEV_MAP_BULK_SIZE XDP_BULK_QUEUE_SIZE
> >
> > +#define XDP_METADATA_KFUNC_xxx       \
> > +     XDP_METADATA_KFUNC(XDP_METADATA_KFUNC_RX_TIMESTAMP_SUPPORTED, \
> > +                        bpf_xdp_metadata_rx_timestamp_supported) \
> > +     XDP_METADATA_KFUNC(XDP_METADATA_KFUNC_RX_TIMESTAMP, \
> > +                        bpf_xdp_metadata_rx_timestamp) \
> > +     XDP_METADATA_KFUNC(XDP_METADATA_KFUNC_RX_HASH_SUPPORTED, \
> > +                        bpf_xdp_metadata_rx_hash_supported) \
> > +     XDP_METADATA_KFUNC(XDP_METADATA_KFUNC_RX_HASH, \
> > +                        bpf_xdp_metadata_rx_hash) \
> > +
> > +enum {
> > +#define XDP_METADATA_KFUNC(name, str) name,
> > +XDP_METADATA_KFUNC_xxx
> > +#undef XDP_METADATA_KFUNC
> > +MAX_XDP_METADATA_KFUNC,
> > +};
> > +
> > +#ifdef CONFIG_NET
> > +u32 xdp_metadata_kfunc_id(int id);
> > +#else
> > +static inline u32 xdp_metadata_kfunc_id(int id) { return 0; }
> > +#endif
> > +
> > +struct xdp_md;
> > +bool bpf_xdp_metadata_rx_timestamp_supported(const struct xdp_md *ctx);
> > +u64 bpf_xdp_metadata_rx_timestamp(const struct xdp_md *ctx);
> > +bool bpf_xdp_metadata_rx_hash_supported(const struct xdp_md *ctx);
> > +u32 bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx);
> > +
>