Re: [xdp-hints] Re: [PATCH bpf-next v3 11/12] mlx5: Support RX XDP metadata

[Stanislav Fomichev <sdf@google.com>]

On Fri, Dec 9, 2022 at 8:45 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Fri, 09 Dec 2022 15:42:37 +0100 Toke Høiland-Jørgensen wrote:
> > If we expect the program to do out of band probing, we could just get
> > rid of the _supported() functions entirely?
> >
> > I mean, to me, the whole point of having the separate _supported()
> > function for each item was to have a lower-overhead way of checking if
> > the metadata item was supported. But if the overhead is not actually
> > lower (because both incur a function call), why have them at all? Then
> > we could just change the implementation from this:
> >
> > bool mlx5e_xdp_rx_hash_supported(const struct xdp_md *ctx)
> > {
> >       const struct mlx5_xdp_buff *_ctx = (void *)ctx;
> >
> >       return _ctx->xdp.rxq->dev->features & NETIF_F_RXHASH;
> > }
> >
> > u32 mlx5e_xdp_rx_hash(const struct xdp_md *ctx)
> > {
> >       const struct mlx5_xdp_buff *_ctx = (void *)ctx;
> >
> >       return be32_to_cpu(_ctx->cqe->rss_hash_result);
> > }
> >
> > to this:
> >
> > u32 mlx5e_xdp_rx_hash(const struct xdp_md *ctx)
> > {
> >       const struct mlx5_xdp_buff *_ctx = (void *)ctx;
> >
> >       if (!(_ctx->xdp.rxq->dev->features & NETIF_F_RXHASH))
> >                 return 0;
> >
> >       return be32_to_cpu(_ctx->cqe->rss_hash_result);
> > }
>
> Are there no corner cases? E.g. in case of an L2 frame you'd then
> expect a hash of 0? Rather than no hash?
>
> If I understand we went for the _supported() thing to make inlining
> the check easier than inlining the actual read of the field.
> But we're told inlining is a bit of a wait.. so isn't the motivation
> for the _supported() pretty much gone? And we should we go back to
> returning an error from the actual read?

Seems fair, we can always bring those _supported() calls back in the
future when the inlining is available and having those separate calls
shows clear benefit.
Then let's go back to a more conventional form below?

int mlx5e_xdp_rx_hash(const struct xdp_md *ctx, u32 *timestamp)
{
      const struct mlx5_xdp_buff *_ctx = (void *)ctx;

       if (!(_ctx->xdp.rxq->dev->features & NETIF_F_RXHASH))
                 return -EOPNOTSUPP;

       *timestamp = be32_to_cpu(_ctx->cqe->rss_hash_result);
       return 0;
 }

> Is partial inlining hard? (inline just the check and generate a full
> call for the read, ending up with the same code as with _supported())

I'm assuming you're suggesting to do this partial inlining manually
(as in, writing the code to output this bytecode)?
This probably also falls into the "manual bpf asm generation tech debt" bucket?
LMK if I missed your point.