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. > Given that you need a v4 anyways, can you split this patch into 2?
