| Message ID | 20191210152454.86247-1-mcroce@redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next] bonding: don't init workqueues on error | expand |
On Tue, 10 Dec 2019 16:24:54 +0100, Matteo Croce wrote: > bond_create() initialize six workqueues used later on. Work _entries_ not _queues_ no? > In the unlikely event that the device registration fails, these > structures are initialized unnecessarily, so move the initialization > out of the error path. Also, create an error label to remove some > duplicated code. Does the initialization of work entries matter? Is this prep for further changes? > Signed-off-by: Matteo Croce <mcroce@redhat.com> > --- > drivers/net/bonding/bond_main.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c > index fcb7c2f7f001..8756b6a023d7 100644 > --- a/drivers/net/bonding/bond_main.c > +++ b/drivers/net/bonding/bond_main.c > @@ -4889,8 +4889,8 @@ int bond_create(struct net *net, const char *name) > bond_setup, tx_queues); > if (!bond_dev) { > pr_err("%s: eek! can't alloc netdev!\n", name); If this is a clean up patch I think this pr_err() could also be removed? Memory allocation usually fail very loudly so there should be no reason to print more errors. > - rtnl_unlock(); > - return -ENOMEM; > + res = -ENOMEM; > + goto out_unlock; > } > > /* > @@ -4905,14 +4905,17 @@ int bond_create(struct net *net, const char *name) > bond_dev->rtnl_link_ops = &bond_link_ops; > > res = register_netdevice(bond_dev); > + if (res < 0) { > + free_netdev(bond_dev); > + goto out_unlock; > + } > > netif_carrier_off(bond_dev); > > bond_work_init_all(bond); > > +out_unlock: > rtnl_unlock(); > - if (res < 0) > - free_netdev(bond_dev); > return res; > } > I do appreciate that the change makes the error handling follow a more usual kernel pattern, but IMHO it'd be even better if the error handling was completely moved. IOW the success path should end with return 0; and the error path should contain free_netdev(bond_dev); - int res; + int err; [...] rtnl_unlock(); return 0; err_free_netdev: free_netdev(bond_dev); err_unlock: rtnl_unlock(); return err; I'm just not 100% sold on the improvement made by this patch being worth the code churn, please convince me, respin or get an ack from one of the maintainers? :)
On Sat, Dec 14, 2019 at 3:11 AM Jakub Kicinski <jakub.kicinski@netronome.com> wrote: > > On Tue, 10 Dec 2019 16:24:54 +0100, Matteo Croce wrote: > > bond_create() initialize six workqueues used later on. > > Work _entries_ not _queues_ no? > Right > > In the unlikely event that the device registration fails, these > > structures are initialized unnecessarily, so move the initialization > > out of the error path. Also, create an error label to remove some > > duplicated code. > > Does the initialization of work entries matter? Is this prep for further > changes? > Not a big issue, I just found useless to initialize those data and free a bit later. Just a cleanup. > > Signed-off-by: Matteo Croce <mcroce@redhat.com> > > --- > > drivers/net/bonding/bond_main.c | 11 +++++++---- > > 1 file changed, 7 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c > > index fcb7c2f7f001..8756b6a023d7 100644 > > --- a/drivers/net/bonding/bond_main.c > > +++ b/drivers/net/bonding/bond_main.c > > @@ -4889,8 +4889,8 @@ int bond_create(struct net *net, const char *name) > > bond_setup, tx_queues); > > if (!bond_dev) { > > pr_err("%s: eek! can't alloc netdev!\n", name); > > If this is a clean up patch I think this pr_err() could also be removed? > Memory allocation usually fail very loudly so there should be no reason > to print more errors. > Sure, I just didn't want to alter the behaviour too much. > > - rtnl_unlock(); > > - return -ENOMEM; > > + res = -ENOMEM; > > + goto out_unlock; > > } > > > > /* > > @@ -4905,14 +4905,17 @@ int bond_create(struct net *net, const char *name) > > bond_dev->rtnl_link_ops = &bond_link_ops; > > > > res = register_netdevice(bond_dev); > > + if (res < 0) { > > + free_netdev(bond_dev); > > + goto out_unlock; > > + } > > > > netif_carrier_off(bond_dev); > > > > bond_work_init_all(bond); > > > > +out_unlock: > > rtnl_unlock(); > > - if (res < 0) > > - free_netdev(bond_dev); > > return res; > > } > > > > I do appreciate that the change makes the error handling follow a more > usual kernel pattern, but IMHO it'd be even better if the error > handling was completely moved. IOW the success path should end with > return 0; and the error path should contain free_netdev(bond_dev); > > - int res; > + int err; > > [...] > > rtnl_unlock(); > > return 0; > > err_free_netdev: > free_netdev(bond_dev); > err_unlock: > rtnl_unlock(); > return err; > > I'm just not 100% sold on the improvement made by this patch being > worth the code churn, please convince me, respin or get an ack from > one of the maintainers? :) > ACK :)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index fcb7c2f7f001..8756b6a023d7 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -4889,8 +4889,8 @@ int bond_create(struct net *net, const char *name) bond_setup, tx_queues); if (!bond_dev) { pr_err("%s: eek! can't alloc netdev!\n", name); - rtnl_unlock(); - return -ENOMEM; + res = -ENOMEM; + goto out_unlock; } /* @@ -4905,14 +4905,17 @@ int bond_create(struct net *net, const char *name) bond_dev->rtnl_link_ops = &bond_link_ops; res = register_netdevice(bond_dev); + if (res < 0) { + free_netdev(bond_dev); + goto out_unlock; + } netif_carrier_off(bond_dev); bond_work_init_all(bond); +out_unlock: rtnl_unlock(); - if (res < 0) - free_netdev(bond_dev); return res; }
bond_create() initialize six workqueues used later on. In the unlikely event that the device registration fails, these structures are initialized unnecessarily, so move the initialization out of the error path. Also, create an error label to remove some duplicated code. Signed-off-by: Matteo Croce <mcroce@redhat.com> --- drivers/net/bonding/bond_main.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)