RE: [RFC PATCH bpf-next 0/8] XDP_REDIRECT_XSK and Batched AF_XDP Rx

["Toke Høiland-Jørgensen" <toke@redhat.com>]

"Loftus, Ciara" <ciara.loftus@intel.com> writes:

>> "Loftus, Ciara" <ciara.loftus@intel.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