On Thu, Jul 16, 2020 at 1:31 PM Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote: > > On Thu, Jul 16, 2020 at 12:01 PM David Ahern <dsahern@xxxxxxxxx> wrote: > > > > On 7/15/20 10:55 PM, Andrii Nakryiko wrote: > > > Instead of delegating to drivers, maintain information about which BPF > > > programs are attached in which XDP modes (generic/skb, driver, or hardware) > > > locally in net_device. This effectively obsoletes XDP_QUERY_PROG command. > > > > > > Such re-organization simplifies existing code already. But it also allows to > > > further add bpf_link-based XDP attachments without drivers having to know > > > about any of this at all, which seems like a good setup. > > > XDP_SETUP_PROG/XDP_SETUP_PROG_HW are just low-level commands to driver to > > > install/uninstall active BPF program. All the higher-level concerns about > > > prog/link interaction will be contained within generic driver-agnostic logic. > > > > > > All the XDP_QUERY_PROG calls to driver in dev_xdp_uninstall() were removed. > > > It's not clear for me why dev_xdp_uninstall() were passing previous prog_flags > > > when resetting installed programs. That seems unnecessary, plus most drivers > > > don't populate prog_flags anyways. Having XDP_SETUP_PROG vs XDP_SETUP_PROG_HW > > > should be enough of an indicator of what is required of driver to correctly > > > reset active BPF program. dev_xdp_uninstall() is also generalized as an > > > iteration over all three supported mode. > > > > > > Signed-off-by: Andrii Nakryiko <andriin@xxxxxx> > > > --- > > > include/linux/netdevice.h | 17 +++- > > > net/core/dev.c | 158 +++++++++++++++++++++----------------- > > > > Similar to my comment on a v1 patch, this change is doing multiple > > things that really should be split into 2 patches - one moving code > > around and the second making the change you want. As is the patch is > > difficult to properly review. > > > > You mean xdp_uninstall? In patch 1 leave it as three separate > sections, but switch to different querying. And then in a separate > patch do a loop? > > Alright, I'll split that up as well. But otherwise I don't really see > much more opportunities to split it. So I ended up not doing that. Given dev_xdp_uninstall() is just 15 lines of code, half of which are trivial, it just doesn't make sense to split dev_xdp_uninstall() refactor into two phases. > > > Given that you need a v4 anyways, can you split this patch into 2?
