Message ID | 1493437911-27167-1-git-send-email-gfree.wind@foxmail.com |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
On 4/28/17 9:51 PM, gfree.wind@foxmail.com wrote: > diff --git a/drivers/net/veth.c b/drivers/net/veth.c > index 8c39d6d..418376a 100644 > --- a/drivers/net/veth.c > +++ b/drivers/net/veth.c > @@ -224,9 +224,21 @@ static int veth_dev_init(struct net_device *dev) > return 0; > } > > -static void veth_dev_free(struct net_device *dev) > +static void veth_destructor_free(struct net_device *dev) _destructor in the name is confusing since veth_dev_free is the dev->destructor. Perhaps that should be veth_free_stats. > { > free_percpu(dev->vstats); > +} > + > +static void veth_dev_uninit(struct net_device *dev) > +{ > + /* dev is not registered, perform the free instead of destructor */ > + if (dev->reg_state == NETREG_UNINITIALIZED) > + veth_destructor_free(dev); > +} > + > +static void veth_dev_free(struct net_device *dev) > +{ > + veth_destructor_free(dev); > free_netdev(dev); > } > > @@ -284,6 +296,7 @@ static void veth_set_rx_headroom(struct net_device *dev, int new_hr) > > static const struct net_device_ops veth_netdev_ops = { > .ndo_init = veth_dev_init, > + .ndo_uninit = veth_dev_uninit, > .ndo_open = veth_open, > .ndo_stop = veth_close, > .ndo_start_xmit = veth_xmit, > Functionally, it looks good to me. Acked-by: David Ahern <dsa@cumulusnetworks.com>
On Sat, Apr 29, 2017 at 11:51 AM, <gfree.wind@foxmail.com> wrote: > From: Gao Feng <gfree.wind@foxmail.com> > > The veth driver allocates some resources in its ndo_init func, and > free them in its destructor func. Then there is one memleak that some > errors happen after register_netdevice invokes the ndo_init callback. > Because the destructor would not be invoked to free the resources. > > Now create one new func veth_destructor_free to free the mem in the > destructor, and add ndo_uninit func also invokes it when fail to register > the veth device. > > It's not only free all resources, but also follow the original desgin > that the resources are freed in the destructor normally after > register the device successfully. > > Signed-off-by: Gao Feng <gfree.wind@foxmail.com> > --- > v3: Split one patch to multiple commits, per David Ahern > v2: Move the free in ndo_uninit when fail to register, per Herbert Xu > v1: initial version > > drivers/net/veth.c | 15 ++++++++++++++- > 1 file changed, 14 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/veth.c b/drivers/net/veth.c > index 8c39d6d..418376a 100644 > --- a/drivers/net/veth.c > +++ b/drivers/net/veth.c > @@ -224,9 +224,21 @@ static int veth_dev_init(struct net_device *dev) > return 0; > } > > -static void veth_dev_free(struct net_device *dev) > +static void veth_destructor_free(struct net_device *dev) > { > free_percpu(dev->vstats); > +} not sure why you needed to add this function. to use free_percpu() directly may be clearer. > + > +static void veth_dev_uninit(struct net_device *dev) > +{ call free_percpu() here, no need to check dev->reg_state. free_percpu will just return if dev->vstats is NULL. > + /* dev is not registered, perform the free instead of destructor */ > + if (dev->reg_state == NETREG_UNINITIALIZED) > + veth_destructor_free(dev); > +} > + > +static void veth_dev_free(struct net_device *dev) > +{ > + veth_destructor_free(dev); use free_percpu here as well. > free_netdev(dev); > } > > @@ -284,6 +296,7 @@ static void veth_set_rx_headroom(struct net_device *dev, int new_hr) > > static const struct net_device_ops veth_netdev_ops = { > .ndo_init = veth_dev_init, > + .ndo_uninit = veth_dev_uninit, > .ndo_open = veth_open, > .ndo_stop = veth_close, > .ndo_start_xmit = veth_xmit, > -- > 2.7.4 >
> From: David Ahern [mailto:dsa@cumulusnetworks.com] > Sent: Monday, May 1, 2017 11:08 PM > On 4/28/17 9:51 PM, gfree.wind@foxmail.com wrote: > > diff --git a/drivers/net/veth.c b/drivers/net/veth.c index > > 8c39d6d..418376a 100644 > > --- a/drivers/net/veth.c > > +++ b/drivers/net/veth.c > > @@ -224,9 +224,21 @@ static int veth_dev_init(struct net_device *dev) > > return 0; > > } > > > > -static void veth_dev_free(struct net_device *dev) > > +static void veth_destructor_free(struct net_device *dev) > > _destructor in the name is confusing since veth_dev_free is the > dev->destructor. Perhaps that should be veth_free_stats. Because I want to emphasize it should be invoked in the destructor. What's your opinion ? [...] > Functionally, it looks good to me. > > Acked-by: David Ahern <dsa@cumulusnetworks.com> Thanks David. I have sent the v4 patches with a series according to David's advice. BTW, because I send multiple patches too fast today, the email server blocks my account. So I have to reply you with a different email account. Sorry. Regards Feng
> From: Xin Long [mailto:lucien.xin@gmail.com] > Sent: Tuesday, May 2, 2017 3:56 PM > On Sat, Apr 29, 2017 at 11:51 AM, <gfree.wind@foxmail.com> wrote: > > From: Gao Feng <gfree.wind@foxmail.com> [...] > > -static void veth_dev_free(struct net_device *dev) > > +static void veth_destructor_free(struct net_device *dev) > > { > > free_percpu(dev->vstats); > > +} > not sure why you needed to add this function. > to use free_percpu() directly may be clearer. Because both of ndo_uninit and destructor need to perform same free statements. It is good at maintain the codes with the common function. > > > + > > +static void veth_dev_uninit(struct net_device *dev) { > call free_percpu() here, no need to check dev->reg_state. > free_percpu will just return if dev->vstats is NULL. It would break the original design if don't check the reg_state. The original logic is that free the resources in the destructor, not in ndo_init. BTW, because I send multiple patches too fast today, the email server blocks my account. So I have to reply you with a different email account. Sorry. Best Regards Feng > [...]
diff --git a/drivers/net/veth.c b/drivers/net/veth.c index 8c39d6d..418376a 100644 --- a/drivers/net/veth.c +++ b/drivers/net/veth.c @@ -224,9 +224,21 @@ static int veth_dev_init(struct net_device *dev) return 0; } -static void veth_dev_free(struct net_device *dev) +static void veth_destructor_free(struct net_device *dev) { free_percpu(dev->vstats); +} + +static void veth_dev_uninit(struct net_device *dev) +{ + /* dev is not registered, perform the free instead of destructor */ + if (dev->reg_state == NETREG_UNINITIALIZED) + veth_destructor_free(dev); +} + +static void veth_dev_free(struct net_device *dev) +{ + veth_destructor_free(dev); free_netdev(dev); } @@ -284,6 +296,7 @@ static void veth_set_rx_headroom(struct net_device *dev, int new_hr) static const struct net_device_ops veth_netdev_ops = { .ndo_init = veth_dev_init, + .ndo_uninit = veth_dev_uninit, .ndo_open = veth_open, .ndo_stop = veth_close, .ndo_start_xmit = veth_xmit,