Message-ID: <87ilwkcl2i.fsf@toke.dk>
Date:   Mon, 22 Nov 2021 15:38:13 +0100
From:   Toke Høiland-Jørgensen <toke@...hat.com>
To:     "Loftus, Ciara" <ciara.loftus@...el.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:

>> "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.

I think it's OK to require the user to make sure that there is such a
socket loaded before using that flag...

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

Yes, it would probably require comparing the contents of the whole
bpf_redirect_info, at least as it is now; but assuming we can find a way
to mitigate the cost of accessing that structure, this may not be so
bad, and I believe there is some potential for compressing the state
further so we can get down to a single, or maybe two, 64-bit compares...

-Toke

Powered by blists - more mailing lists