Messages in this thread | | | From | Matteo Croce <> | Date | Sat, 14 Dec 2019 14:14:13 +0100 | Subject | Re: [PATCH net-next] bonding: don't init workqueues on error |
| |
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 :)
-- Matteo Croce per aspera ad upstream
| |