On Wed, May 3, 2017 at 10:07 AM, Gao Feng <[email protected]> wrote: >> 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. Sorry, I didn't get your "backward" I can't see there will be any problem caused by it.
can you say this patch also break the 'backward' ? https://patchwork.ozlabs.org/patch/748964/ It's really weird to do dev->reg_state check in ndo_unint ndo_unint is supposed to free the memory alloced in ndo_init. > > Regards > Feng > >
