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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAKH8qBsx4pPuvYenpM18NgdnGCG8QjqnsNY40Uc44EXTUVabMA@mail.gmail.com>
Date:   Fri, 9 Dec 2022 09:46:20 -0800
From:   Stanislav Fomichev <sdf@...gle.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Toke Høiland-Jørgensen <toke@...hat.com>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        bpf <bpf@...r.kernel.org>, Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Martin KaFai Lau <martin.lau@...ux.dev>,
        Song Liu <song@...nel.org>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>, Hao Luo <haoluo@...gle.com>,
        Jiri Olsa <jolsa@...nel.org>,
        Saeed Mahameed <saeedm@...dia.com>,
        David Ahern <dsahern@...il.com>,
        Willem de Bruijn <willemb@...gle.com>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        Anatoly Burakov <anatoly.burakov@...el.com>,
        Alexander Lobakin <alexandr.lobakin@...el.com>,
        Magnus Karlsson <magnus.karlsson@...il.com>,
        Maryam Tahhan <mtahhan@...hat.com>, xdp-hints@...-project.net,
        Network Development <netdev@...r.kernel.org>
Subject: Re: [xdp-hints] Re: [PATCH bpf-next v3 11/12] mlx5: Support RX XDP metadata

On Fri, Dec 9, 2022 at 8:45 AM Jakub Kicinski <kuba@...nel.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.