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