> From: [email protected] [mailto:[email protected]] > On Behalf Of Xin Long > Sent: Wednesday, May 3, 2017 12:59 AM > On Tue, May 2, 2017 at 7:03 PM, Gao Feng <[email protected]> wrote: > >> From: Xin Long [mailto:[email protected]] > >> Sent: Tuesday, May 2, 2017 3:56 PM > >> On Sat, Apr 29, 2017 at 11:51 AM, <[email protected]> wrote: > >> > From: Gao Feng <[email protected]> > > [...] > >> > -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. > I got what you're doing now, can you pls try to fix this with: > > --- a/drivers/net/veth.c > +++ b/drivers/net/veth.c > @@ -219,10 +219,9 @@ static int veth_dev_init(struct net_device *dev) > return 0; > } > > -static void veth_dev_free(struct net_device *dev) > +static void veth_dev_uninit(struct net_device *dev) > { > free_percpu(dev->vstats); > - free_netdev(dev); > } > > #ifdef CONFIG_NET_POLL_CONTROLLER > @@ -279,6 +278,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, > @@ -317,7 +317,7 @@ static void veth_setup(struct net_device *dev) > NETIF_F_HW_VLAN_STAG_TX | > NETIF_F_HW_VLAN_CTAG_RX | > NETIF_F_HW_VLAN_STAG_RX); > - dev->destructor = veth_dev_free; > + dev->destructor = free_netdev; > dev->max_mtu = ETH_MAX_MTU; > > dev->hw_features = VETH_FEATURES; > > > just as what other virtual nic drivers do (vxlan, geneve, macsec, bridge ....) >
The fix you mentioned change the original logic. The dev->vstats is freed in advance in the ndo_uninit, not destructor. It may break the backward. Regards Feng
