| Message ID | 20240201170937.3549878-6-edumazet@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: more factorization in cleanup_net() paths | expand |
Hello, Quoting Eric Dumazet (2024-02-01 18:09:26) > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c > index 4e0600c7b050f21c82a8862e224bb055e95d5039..181da7ea389312d7c851ca51c35b871c07144b6b 100644 > --- a/drivers/net/bonding/bond_main.c > +++ b/drivers/net/bonding/bond_main.c > @@ -6419,34 +6419,34 @@ static void __net_exit bond_net_exit_batch(struct list_head *net_list) > { > struct bond_net *bn; > struct net *net; > - LIST_HEAD(list); > > list_for_each_entry(net, net_list, exit_list) { > bn = net_generic(net, bond_net_id); > bond_destroy_sysfs(bn); > + bond_destroy_proc_dir(bn); > } > +} > + > +static void __net_exit bond_net_exit_batch_rtnl(struct list_head *net_list, > + struct list_head *dev_kill_list) > +{ > + struct bond_net *bn; > + struct net *net; > > /* Kill off any bonds created after unregistering bond rtnl ops */ > - rtnl_lock(); > list_for_each_entry(net, net_list, exit_list) { > struct bonding *bond, *tmp_bond; > > bn = net_generic(net, bond_net_id); > list_for_each_entry_safe(bond, tmp_bond, &bn->dev_list, bond_list) > - unregister_netdevice_queue(bond->dev, &list); > - } > - unregister_netdevice_many(&list); > - rtnl_unlock(); > - > - list_for_each_entry(net, net_list, exit_list) { > - bn = net_generic(net, bond_net_id); > - bond_destroy_proc_dir(bn); > + unregister_netdevice_queue(bond->dev, dev_kill_list); > } > } This changes the logic of how bond net & devices are destroyed. Before this patch the logic was: 1. bond_destroy_sysfs; called first so no new bond devices can be registered later. 2. unregister_netdevice; cleans up any new bond device added before 1. This now is: 1. unregister_netdevice // Here new bond devices can still be registered. 2. bond_destroy_sysfs Looking briefly at the history, the above is done on purpose to avoid issues when unloading the bonding module. See 23fa5c2caae0 and especially 69b0216ac255. Thanks, Antoine
On Fri, Feb 2, 2024 at 3:29 PM Antoine Tenart <atenart@kernel.org> wrote: > > Hello, > > Quoting Eric Dumazet (2024-02-01 18:09:26) > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c > > index 4e0600c7b050f21c82a8862e224bb055e95d5039..181da7ea389312d7c851ca51c35b871c07144b6b 100644 > > --- a/drivers/net/bonding/bond_main.c > > +++ b/drivers/net/bonding/bond_main.c > > @@ -6419,34 +6419,34 @@ static void __net_exit bond_net_exit_batch(struct list_head *net_list) > > { > > struct bond_net *bn; > > struct net *net; > > - LIST_HEAD(list); > > > > list_for_each_entry(net, net_list, exit_list) { > > bn = net_generic(net, bond_net_id); > > bond_destroy_sysfs(bn); > > + bond_destroy_proc_dir(bn); > > } > > +} > > + > > +static void __net_exit bond_net_exit_batch_rtnl(struct list_head *net_list, > > + struct list_head *dev_kill_list) > > +{ > > + struct bond_net *bn; > > + struct net *net; > > > > /* Kill off any bonds created after unregistering bond rtnl ops */ > > - rtnl_lock(); > > list_for_each_entry(net, net_list, exit_list) { > > struct bonding *bond, *tmp_bond; > > > > bn = net_generic(net, bond_net_id); > > list_for_each_entry_safe(bond, tmp_bond, &bn->dev_list, bond_list) > > - unregister_netdevice_queue(bond->dev, &list); > > - } > > - unregister_netdevice_many(&list); > > - rtnl_unlock(); > > - > > - list_for_each_entry(net, net_list, exit_list) { > > - bn = net_generic(net, bond_net_id); > > - bond_destroy_proc_dir(bn); > > + unregister_netdevice_queue(bond->dev, dev_kill_list); > > } > > } > > This changes the logic of how bond net & devices are destroyed. Before > this patch the logic was: > > 1. bond_destroy_sysfs; called first so no new bond devices can be > registered later. > 2. unregister_netdevice; cleans up any new bond device added before 1. > > This now is: > 1. unregister_netdevice > // Here new bond devices can still be registered. > 2. bond_destroy_sysfs > > Looking briefly at the history, the above is done on purpose to avoid > issues when unloading the bonding module. See 23fa5c2caae0 and > especially 69b0216ac255. Nice catch, thanks. Hmmm, it seems we should perform the bond_destroy_sysfs(bn) call earlier then, from the pre_exit() method... Order of calls is : 1) pre_exit() 2) exit_batch_rtnl() 3) exit(), exit_batch() Something like the following (that I would squash on the current patch) diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 181da7ea389312d7c851ca51c35b871c07144b6b..7edd3daa7e6d977e6b0220226b3cd4f8f67a7952 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -6415,6 +6415,13 @@ static int __net_init bond_net_init(struct net *net) return 0; } +static void __net_exit bond_net_pre_exit(struct net *net) +{ + struct bond_net *bn = net_generic(net, bond_net_id); + + bond_destroy_sysfs(bn); +} + static void __net_exit bond_net_exit_batch(struct list_head *net_list) { struct bond_net *bn; @@ -6422,7 +6429,6 @@ static void __net_exit bond_net_exit_batch(struct list_head *net_list) list_for_each_entry(net, net_list, exit_list) { bn = net_generic(net, bond_net_id); - bond_destroy_sysfs(bn); bond_destroy_proc_dir(bn); } } @@ -6445,8 +6451,9 @@ static void __net_exit bond_net_exit_batch_rtnl(struct list_head *net_list, static struct pernet_operations bond_net_ops = { .init = bond_net_init, - .exit_batch = bond_net_exit_batch, + .pre_exit = bond_net_pre_exit, .exit_batch_rtnl = bond_net_exit_batch_rtnl, + .exit_batch = bond_net_exit_batch, .id = &bond_net_id, .size = sizeof(struct bond_net), };
Quoting Eric Dumazet (2024-02-02 15:51:43) > On Fri, Feb 2, 2024 at 3:29 PM Antoine Tenart <atenart@kernel.org> wrote: > > Quoting Eric Dumazet (2024-02-01 18:09:26) > > > > This changes the logic of how bond net & devices are destroyed. Before > > this patch the logic was: > > > > 1. bond_destroy_sysfs; called first so no new bond devices can be > > registered later. > > 2. unregister_netdevice; cleans up any new bond device added before 1. > > > > This now is: > > 1. unregister_netdevice > > // Here new bond devices can still be registered. > > 2. bond_destroy_sysfs > > > > Looking briefly at the history, the above is done on purpose to avoid > > issues when unloading the bonding module. See 23fa5c2caae0 and > > especially 69b0216ac255. > > Nice catch, thanks. > > Hmmm, it seems we should perform the bond_destroy_sysfs(bn) call earlier then, > from the pre_exit() method... > > Order of calls is : > 1) pre_exit() > 2) exit_batch_rtnl() > 3) exit(), exit_batch() > > Something like the following (that I would squash on the current patch) Looks good, thanks! > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c > index 181da7ea389312d7c851ca51c35b871c07144b6b..7edd3daa7e6d977e6b0220226b3cd4f8f67a7952 > 100644 > --- a/drivers/net/bonding/bond_main.c > +++ b/drivers/net/bonding/bond_main.c > @@ -6415,6 +6415,13 @@ static int __net_init bond_net_init(struct net *net) > return 0; > } > > +static void __net_exit bond_net_pre_exit(struct net *net) > +{ > + struct bond_net *bn = net_generic(net, bond_net_id); > + > + bond_destroy_sysfs(bn); > +} > + > static void __net_exit bond_net_exit_batch(struct list_head *net_list) > { > struct bond_net *bn; > @@ -6422,7 +6429,6 @@ static void __net_exit > bond_net_exit_batch(struct list_head *net_list) > > list_for_each_entry(net, net_list, exit_list) { > bn = net_generic(net, bond_net_id); > - bond_destroy_sysfs(bn); > bond_destroy_proc_dir(bn); > } > } > @@ -6445,8 +6451,9 @@ static void __net_exit > bond_net_exit_batch_rtnl(struct list_head *net_list, > > static struct pernet_operations bond_net_ops = { > .init = bond_net_init, > - .exit_batch = bond_net_exit_batch, > + .pre_exit = bond_net_pre_exit, > .exit_batch_rtnl = bond_net_exit_batch_rtnl, > + .exit_batch = bond_net_exit_batch, > .id = &bond_net_id, > .size = sizeof(struct bond_net), > }; >
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 4e0600c7b050f21c82a8862e224bb055e95d5039..181da7ea389312d7c851ca51c35b871c07144b6b 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -6419,34 +6419,34 @@ static void __net_exit bond_net_exit_batch(struct list_head *net_list) { struct bond_net *bn; struct net *net; - LIST_HEAD(list); list_for_each_entry(net, net_list, exit_list) { bn = net_generic(net, bond_net_id); bond_destroy_sysfs(bn); + bond_destroy_proc_dir(bn); } +} + +static void __net_exit bond_net_exit_batch_rtnl(struct list_head *net_list, + struct list_head *dev_kill_list) +{ + struct bond_net *bn; + struct net *net; /* Kill off any bonds created after unregistering bond rtnl ops */ - rtnl_lock(); list_for_each_entry(net, net_list, exit_list) { struct bonding *bond, *tmp_bond; bn = net_generic(net, bond_net_id); list_for_each_entry_safe(bond, tmp_bond, &bn->dev_list, bond_list) - unregister_netdevice_queue(bond->dev, &list); - } - unregister_netdevice_many(&list); - rtnl_unlock(); - - list_for_each_entry(net, net_list, exit_list) { - bn = net_generic(net, bond_net_id); - bond_destroy_proc_dir(bn); + unregister_netdevice_queue(bond->dev, dev_kill_list); } } static struct pernet_operations bond_net_ops = { .init = bond_net_init, .exit_batch = bond_net_exit_batch, + .exit_batch_rtnl = bond_net_exit_batch_rtnl, .id = &bond_net_id, .size = sizeof(struct bond_net), };
exit_batch_rtnl() is called while RTNL is held, and devices to be unregistered can be queued in the dev_kill_list. This saves one rtnl_lock()/rtnl_unlock() pair, and one unregister_netdevice_many() call. Signed-off-by: Eric Dumazet <edumazet@google.com> --- drivers/net/bonding/bond_main.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)