netdev - RE: [RFC PATCH bpf-next 0/8] XDP_REDIRECT_XSK and Batched AF

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: <PH0PR11MB4791ABCE7F631A4BBEAF1B5E8E9C9@PH0PR11MB4791.namprd11.prod.outlook.com>
Date:   Fri, 19 Nov 2021 15:48:13 +0000
From:   "Loftus, Ciara" <ciara.loftus@...el.com>
To:     Toke Høiland-Jørgensen <toke@...hat.com>,
        "Jesper Dangaard Brouer" <jbrouer@...hat.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "bpf@...r.kernel.org" <bpf@...r.kernel.org>
CC:     "brouer@...hat.com" <brouer@...hat.com>,
        "ast@...nel.org" <ast@...nel.org>,
        "daniel@...earbox.net" <daniel@...earbox.net>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "hawk@...nel.org" <hawk@...nel.org>,
        "john.fastabend@...il.com" <john.fastabend@...il.com>,
        "bjorn@...nel.org" <bjorn@...nel.org>,
        "Karlsson, Magnus" <magnus.karlsson@...el.com>,
        "jonathan.lemon@...il.com" <jonathan.lemon@...il.com>,
        "Fijalkowski, Maciej" <maciej.fijalkowski@...el.com>
Subject: RE: [RFC PATCH bpf-next 0/8] XDP_REDIRECT_XSK and Batched AF_XDP Rx

> "Loftus, Ciara" <ciara.loftus@...el.com> writes:
> 
> >> I'm fine with adding a new helper, but I don't like introducing a new
> >> XDP_REDIRECT_XSK action, which requires updating ALL the drivers.
> >>
> >> With XDP_REDIRECT infra we beleived we didn't need to add more
> >> XDP-action code to drivers, as we multiplex/add new features by
> >> extending the bpf_redirect_info.
> >> In this extreme performance case, it seems the this_cpu_ptr "lookup" of
> >> bpf_redirect_info is the performance issue itself.
> >>
> >> Could you experiement with different approaches that modify
> >> xdp_do_redirect() to handle if new helper bpf_redirect_xsk was called,
> >> prior to this_cpu_ptr() call.
> >> (Thus, avoiding to introduce a new XDP-action).
> >
> > Thanks for your feedback Jesper.
> > I understand the hesitation of adding a new action. If we can achieve the
> same improvement without
> > introducing a new action I would be very happy!
> > Without new the action we'll need a new way to indicate that the
> bpf_redirect_xsk helper was
> > called. Maybe another new field in the netdev alongside the xsk_refcnt. Or
> else extend
> > bpf_redirect_info - if we find a new home for it that it's too costly to
> access.
> > Thanks for your suggestions. I'll experiment as you suggested and
> > report back.
> 
> I'll add a +1 to the "let's try to solve this without a new return code" :)
> 
> Also, I don't think we need a new helper either; the bpf_redirect()
> helper takes a flags argument, so we could just use ifindex=0,
> flags=DEV_XSK or something like that.

The advantage of a new helper is that we can access the netdev 
struct from it and check if there's a valid xsk stored in it, before
returning XDP_REDIRECT without the xskmap lookup. However,
I think your suggestion could work too. We would just
have to delay the check until xdp_do_redirect. At this point
though, if there isn't a valid xsk we might have to drop the packet
instead of falling back to the xskmap.

> 
> Also, I think the batching in the driver idea can be generalised: we
> just need to generalise the idea of "are all these packets going to the
> same place" and have a batched version of xdp_do_redirect(), no? The
> other map types do batching internally already, though, so I'm wondering
> why batching in the driver helps XSK?
> 

With the current infrastructure figuring out if "all the packets are going
to the same place" looks like an expensive operation which could undo
the benefits of the batching that would come after it. We would need
to run the program N=batch_size times, store the actions and
bpf_redirect_info for each run and perform a series of compares. The new
action really helped here because it could easily indicate if all the
packets in a batch were going to the same place. But I understand it's
not an option. Maybe if we can mitigate the cost of accessing the
bpf_redirect_info as Jesper suggested, we can use a flag in it to signal
what the new action was signalling.
I'm not familiar with how the other map types and how they handle
batching so I will look into that.

Appreciate your feedback. I have a few avenues to explore.

Ciara

> -Toke