IPoIB uninit
Subsystems (5)
cover IPoIB uninit
From: Leon Romanovsky <[email protected]>
IP link was broken due to the changes in IPoIB for the rdma_netdev support after commit cd565b4b51e5 ("IB/IPoIB: Support acceleration options callbacks").
This patchset restores IPoIB pkey creation and removal using rtnetlink.
It is completely rewritten variant of
https://marc.info/?l=linux-rdma&m=151553425830918&w=2 patch series.
Thanks
Erez Shitrit (2):
IB/ipoib: Use cancel_delayed_work_sync for neigh-clean task IB/ipoib: Make ipoib_neigh_hash_uninit fully fence its work
Jason Gunthorpe (8):
IB/ipoib: Get rid of IPOIB_FLAG_GOING_DOWN
IB/ipoib: Move all uninit code into ndo_uninit IB/ipoib: Move init code to ndo_init
RDMA/netdev: Use priv_destructor for netdev cleanup IB/ipoib: Get rid of the sysfs_mutex
IB/ipoib: Do not remove child devices from within the ndo_uninit IB/ipoib: Maintain the child_intfs list from ndo_init/uninit IB/ipoib: Consolidate checking of the proposed child interface
drivers/infiniband/hw/mlx5/main.c | 10 - drivers/infiniband/ulp/ipoib/ipoib.h | 16 +- drivers/infiniband/ulp/ipoib/ipoib_cm.c | 14 +- drivers/infiniband/ulp/ipoib/ipoib_main.c | 419 ++++++++++++--------- drivers/infiniband/ulp/ipoib/ipoib_netlink.c | 23 -- drivers/infiniband/ulp/ipoib/ipoib_vlan.c | 259 +++++++------ .../net/ethernet/mellanox/mlx5/core/ipoib/ipoib.c | 37 +- include/linux/mlx5/driver.h | 3 - include/rdma/ib_verbs.h | 6 +- 9 files changed, 428 insertions(+), 359 deletions(-)
PATCH 1/10 IB/ipoib: Get rid of IPOIB_FLAG_GOING_DOWN
From: Jason Gunthorpe <[email protected]>
This essentially duplicates the netdev's reg_state, so just use that directly. The reg_state is updated under the rntl_lock, and all places using GOING_DOWN already acquire the rtnl_lock so checking is safe.
Since the only place we use GOING_DOWN is for the parent device this does not fix any bugs, but it is a step to tidy up the unregister flow so that after later patches the flow is uniform and sane.
--- drivers/infiniband/ulp/ipoib/ipoib.h | 1 - drivers/infiniband/ulp/ipoib/ipoib_cm.c | 9 ++++++--- drivers/infiniband/ulp/ipoib/ipoib_main.c | 3 --- drivers/infiniband/ulp/ipoib/ipoib_vlan.c | 18 ++++++++++++------ 4 files changed, 18 insertions(+), 13 deletions(-)
drivers/infiniband/ulp/ipoib/ipoib.h +0 -1
@@ -95,7 +95,6 @@ enum {IPOIB_NEIGH_TBL_FLUSH =12 ,IPOIB_FLAG_DEV_ADDR_SET =13 ,IPOIB_FLAG_DEV_ADDR_CTRL =14 , -IPOIB_FLAG_GOING_DOWN =15 , IPOIB_MAX_BACKOFF_SECONDS = 16,
drivers/infiniband/ulp/ipoib/ipoib_cm.c +6 -3
@@ -1516,9 +1516,6 @@ static ssize_t set_mode(struct device *d, struct device_attribute *attr,int ret;struct ipoib_dev_priv *priv =ipoib_priv (dev); -if (test_bit (IPOIB_FLAG_GOING_DOWN , &priv->flags)) -return -EPERM ; -if (!mutex_trylock (&priv->sysfs_mutex))return restart_syscall ();
@@ -1527,6 +1524,12 @@ static ssize_t set_mode(struct device *d, struct device_attribute *attr,return restart_syscall (); } +if (dev->reg_state !=NETREG_REGISTERED ) { +rtnl_unlock (); +mutex_unlock (&priv->sysfs_mutex); +return -EPERM ; + } + ret =ipoib_set_mode (dev, buf);/* The assumption is that the function ipoib_set_mode returned
drivers/infiniband/ulp/ipoib/ipoib_main.c +0 -3
@@ -2406,9 +2406,6 @@ static void ipoib_remove_one(struct ib_device *device, void *client_data)ib_unregister_event_handler (&priv->event_handler);flush_workqueue (ipoib_workqueue); -/* mark interface in the middle of destruction */ -set_bit (IPOIB_FLAG_GOING_DOWN , &priv->flags); -rtnl_lock ();dev_change_flags (priv->dev, priv->dev->flags & ~IFF_UP );rtnl_unlock ();
drivers/infiniband/ulp/ipoib/ipoib_vlan.c +12 -6
@@ -127,9 +127,6 @@ int ipoib_vlan_add(struct net_device *pdev, unsigned short pkey) ppriv =ipoib_priv (pdev); -if (test_bit (IPOIB_FLAG_GOING_DOWN , &ppriv->flags)) -return -EPERM ; -snprintf (intf_name,sizeof (intf_name),"%s.%04x" , ppriv->dev->name, pkey);
@@ -141,6 +138,12 @@ int ipoib_vlan_add(struct net_device *pdev, unsigned short pkey)return restart_syscall (); } +if (pdev->reg_state !=NETREG_REGISTERED ) { +rtnl_unlock (); +mutex_unlock (&ppriv->sysfs_mutex); +return -EPERM ; + } +if (!down_write_trylock (&ppriv->vlan_rwsem)) {rtnl_unlock ();mutex_unlock (&ppriv->sysfs_mutex);
@@ -199,9 +202,6 @@ int ipoib_vlan_delete(struct net_device *pdev, unsigned short pkey) ppriv =ipoib_priv (pdev); -if (test_bit (IPOIB_FLAG_GOING_DOWN , &ppriv->flags)) -return -EPERM ; -if (!mutex_trylock (&ppriv->sysfs_mutex))return restart_syscall ();
@@ -210,6 +210,12 @@ int ipoib_vlan_delete(struct net_device *pdev, unsigned short pkey)return restart_syscall (); } +if (pdev->reg_state !=NETREG_REGISTERED ) { +rtnl_unlock (); +mutex_unlock (&ppriv->sysfs_mutex); +return -EPERM ; + } +if (!down_write_trylock (&ppriv->vlan_rwsem)) {rtnl_unlock ();mutex_unlock (&ppriv->sysfs_mutex);
Signature · 1 line
2.14.4
PATCH 2/10 IB/ipoib: Use cancel_delayed_work_sync for neigh-clean task
In reply to Leon RomanovskyFrom: Erez Shitrit <[email protected]>
The neigh_reap_task is self restarting, but so long as we call cancel_delayed_work_sync() it will be guaranteed to not be running and never start again. Thus we don't need to have the racy IPOIB_STOP_NEIGH_GC bit, or the confusing mismatch of places sometimes calling flush_workqueue after the cancel.
This fixes a situation where the GC work could have been left running in some rare situations.
--- drivers/infiniband/ulp/ipoib/ipoib_main.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/infiniband/ulp/ipoib/ipoib_main.c +4 -4
@@ -1661,7 +1661,7 @@ static void ipoib_neigh_hash_uninit(struct net_device *dev)/* Stop GC if called at init fail need to cancel work */ stopped =test_and_set_bit (IPOIB_STOP_NEIGH_GC , &priv->flags);if (!stopped) -cancel_delayed_work (&priv->neigh_reap_task); +cancel_delayed_work_sync (&priv->neigh_reap_task);ipoib_flush_neighs (priv);
@@ -1837,7 +1837,7 @@ void ipoib_dev_cleanup(struct net_device *dev)list_for_each_entry_safe (cpriv, tcpriv, &priv->child_intfs, list) {/* Stop GC on child */ set_bit (IPOIB_STOP_NEIGH_GC , &cpriv->flags); -cancel_delayed_work (&cpriv->neigh_reap_task); +cancel_delayed_work_sync (&cpriv->neigh_reap_task);unregister_netdevice_queue (cpriv->dev, &head); }unregister_netdevice_many (&head);
@@ -2346,7 +2346,7 @@ static struct net_device *ipoib_add_port(const char *format,flush_workqueue (ipoib_workqueue);/* Stop GC if started before flush */ set_bit (IPOIB_STOP_NEIGH_GC , &priv->flags); -cancel_delayed_work (&priv->neigh_reap_task); +cancel_delayed_work_sync (&priv->neigh_reap_task);flush_workqueue (priv->wq);ipoib_dev_cleanup (priv->dev);
@@ -2412,7 +2412,7 @@ static void ipoib_remove_one(struct ib_device *device, void *client_data)/* Stop GC */ set_bit (IPOIB_STOP_NEIGH_GC , &priv->flags); -cancel_delayed_work (&priv->neigh_reap_task); +cancel_delayed_work_sync (&priv->neigh_reap_task);flush_workqueue (priv->wq);/* Wrap rtnl_lock/unlock with mutex to protect sysfs calls */
Signature · 1 line
2.14.4
PATCH 3/10 IB/ipoib: Move all uninit code into ndo_uninit
In reply to Leon RomanovskyFrom: Jason Gunthorpe <[email protected]>
Currently uninit is sometimes done twice in error flows, and is sprinkled a bit all over the place.
Improve the clarity of the design by moving all uninit only into ndo_uinit.
Some duplication is removed:
- Sometimes IPOIB_STOP_NEIGH_GC was done before unregister, but this duplicates the process in ipoib_neigh_hash_init
- Flushing priv->wq was sometimes done before unregister, but that duplicates what has been done in ndo_uninit
Uniniting the IB event queue must remain before unregister_netdev as it requires the RTNL lock to be dropped, this is moved to a helper to make that flow really clear and remove some duplication in error flows.
If register_netdev fails (and ndo_init is NULL) then it almost always calls ndo_uninit, which lets us remove all the extra code from the error unwinds. The next patch in the series will close the 'almost always' hole by pairing a proper ndo_init with ndo_uninit.
--- drivers/infiniband/ulp/ipoib/ipoib.h | 1 - drivers/infiniband/ulp/ipoib/ipoib_main.c | 62 ++++++++++++++++--------------- drivers/infiniband/ulp/ipoib/ipoib_vlan.c | 5 +-- 3 files changed, 34 insertions(+), 34 deletions(-)
drivers/infiniband/ulp/ipoib/ipoib.h +0 -1
@@ -510,7 +510,6 @@ int ipoib_ib_dev_stop_default(struct net_device *dev);void ipoib_pkey_dev_check_presence (struct net_device *dev);int ipoib_dev_init (struct net_device *dev,struct ib_device *ca,int port); -void ipoib_dev_cleanup (struct net_device *dev);void ipoib_mcast_join_task (struct work_struct *work);void ipoib_mcast_carrier_on_task (struct work_struct *work);
drivers/infiniband/ulp/ipoib/ipoib_main.c +33 -29
106 lines not on this page · raw message
drivers/infiniband/ulp/ipoib/ipoib_vlan.c +1 -4
@@ -83,7 +83,7 @@ int __ipoib_vlan_add(struct ipoib_dev_priv *ppriv, struct ipoib_dev_priv *priv, result =register_netdevice (priv->dev);if (result) {ipoib_warn (priv,"failed to initialize; error %i" , result); - goto register_failed; + goto err; }/* RTNL childs don't need proprietary sysfs entries */
@@ -108,9 +108,6 @@ int __ipoib_vlan_add(struct ipoib_dev_priv *ppriv, struct ipoib_dev_priv *priv, result = -ENOMEM ;unregister_netdevice (priv->dev); -register_failed: -ipoib_dev_cleanup (priv->dev); - err:return result; }
Signature · 1 line
2.14.4
PATCH 4/10 IB/ipoib: Make ipoib_neigh_hash_uninit fully fence its work
In reply to Leon RomanovskyFrom: Erez Shitrit <[email protected]>
The neigh_reap_task is self restarting, but so long as we call cancel_delayed_work_sync() it will be guaranteed to not be running and never start again. Thus there is no longer any purpose to IPOIB_STOP_NEIGH_GC (which was racy/buggy anyhow), so remove it too.
This fixes a situation where the GC work could have been running
--- drivers/infiniband/ulp/ipoib/ipoib.h | 1 - drivers/infiniband/ulp/ipoib/ipoib_main.c | 25 +++++++------------------ 2 files changed, 7 insertions(+), 19 deletions(-)
drivers/infiniband/ulp/ipoib/ipoib.h +0 -1
@@ -91,7 +91,6 @@ enum {IPOIB_STOP_REAPER =7 ,IPOIB_FLAG_ADMIN_CM =9 ,IPOIB_FLAG_UMCAST =10 , -IPOIB_STOP_NEIGH_GC =11 ,IPOIB_NEIGH_TBL_FLUSH =12 ,IPOIB_FLAG_DEV_ADDR_SET =13 , IPOIB_FLAG_DEV_ADDR_CTRL = 14,
drivers/infiniband/ulp/ipoib/ipoib_main.c +7 -18
@@ -1305,9 +1305,6 @@ static void __ipoib_reap_neigh(struct ipoib_dev_priv *priv)int i;LIST_HEAD (remove_list); -if (test_bit (IPOIB_STOP_NEIGH_GC , &priv->flags)) -return ; -spin_lock_irqsave (&priv->lock, flags); htbl = rcu_dereference_protected(ntbl->htbl,
@@ -1319,9 +1316,6 @@ static void __ipoib_reap_neigh(struct ipoib_dev_priv *priv)/* neigh is obsolete if it was idle for two GC periods */ dt =2 * arp_tbl.gc_interval; neigh_obsolete = jiffies - dt; -/* handle possible race condition */ -if (test_bit (IPOIB_STOP_NEIGH_GC , &priv->flags)) - goto out_unlock;for (i =0 ; i < htbl->size; i++) {struct ipoib_neigh *neigh;
@@ -1359,9 +1353,8 @@ static void ipoib_reap_neigh(struct work_struct *work)__ipoib_reap_neigh (priv); -if (!test_bit (IPOIB_STOP_NEIGH_GC , &priv->flags)) -queue_delayed_work (priv->wq, &priv->neigh_reap_task, - arp_tbl.gc_interval); +queue_delayed_work (priv->wq, &priv->neigh_reap_task, + arp_tbl.gc_interval); }
@@ -1523,7 +1516,6 @@ static int ipoib_neigh_hash_init(struct ipoib_dev_priv *priv) htbl =kzalloc (sizeof (*htbl),GFP_KERNEL );if (!htbl)return -ENOMEM ; -set_bit (IPOIB_STOP_NEIGH_GC , &priv->flags); size =roundup_pow_of_two (arp_tbl.gc_thresh3); buckets =kvcalloc (size,sizeof (*buckets),GFP_KERNEL );if (!buckets) {
@@ -1538,7 +1530,6 @@ static int ipoib_neigh_hash_init(struct ipoib_dev_priv *priv)atomic_set (&ntbl->entries,0 );/* start garbage collection */ -clear_bit (IPOIB_STOP_NEIGH_GC , &priv->flags);queue_delayed_work (priv->wq, &priv->neigh_reap_task, arp_tbl.gc_interval);
@@ -1648,15 +1639,11 @@ static void ipoib_flush_neighs(struct ipoib_dev_priv *priv)static void ipoib_neigh_hash_uninit (struct net_device *dev) {struct ipoib_dev_priv *priv =ipoib_priv (dev); -int stopped;ipoib_dbg (priv,"ipoib_neigh_hash_uninit\n" );init_completion (&priv->ntbl.deleted); -/* Stop GC if called at init fail need to cancel work */ - stopped =test_and_set_bit (IPOIB_STOP_NEIGH_GC , &priv->flags); -if (!stopped) -cancel_delayed_work_sync (&priv->neigh_reap_task); +cancel_delayed_work_sync (&priv->neigh_reap_task);ipoib_flush_neighs (priv);
@@ -1796,12 +1783,15 @@ int ipoib_dev_init(struct net_device *dev, struct ib_device *ca, int port)if (ipoib_ib_dev_open (dev)) {pr_warn ("%s failed to open device\n" , dev->name); ret = -ENODEV ; - goto out_dev_uninit; + goto out_hash_uninit; } }return 0 ; +out_hash_uninit: +ipoib_neigh_hash_uninit (dev); + out_dev_uninit:ipoib_ib_dev_cleanup (dev);
@@ -1857,7 +1847,6 @@ static void ipoib_ndo_uninit(struct net_device *dev)/* Delete any child interfaces first */ list_for_each_entry_safe (cpriv, tcpriv, &priv->child_intfs, list) {/* Stop GC on child */ -set_bit (IPOIB_STOP_NEIGH_GC , &cpriv->flags);cancel_delayed_work_sync (&cpriv->neigh_reap_task);unregister_netdevice_queue (cpriv->dev, &head); }
Signature · 1 line
2.14.4
PATCH 5/10 IB/ipoib: Move init code to ndo_init
In reply to Leon RomanovskyFrom: Jason Gunthorpe <[email protected]>
Now that we have a proper ndo_uninit, move code that naturally pairs with the ndo_uninit into ndo_init. This allows the netdev core to natually handle ordering.
This fixes the situation where register_netdev can fail before calling ndo_init, in which case it wouldn't call ndo_uninit either.
Also move a bunch of duplicated init code that is shared between child and parent for clarity. Now the child and parent register functions look very similar.
--- drivers/infiniband/ulp/ipoib/ipoib.h | 3 - drivers/infiniband/ulp/ipoib/ipoib_main.c | 193 +++++++++++++++------------ drivers/infiniband/ulp/ipoib/ipoib_netlink.c | 6 - drivers/infiniband/ulp/ipoib/ipoib_vlan.c | 31 +---- 4 files changed, 114 insertions(+), 119 deletions(-)
drivers/infiniband/ulp/ipoib/ipoib.h +0 -3
@@ -508,8 +508,6 @@ void ipoib_ib_dev_down(struct net_device *dev);int ipoib_ib_dev_stop_default (struct net_device *dev);void ipoib_pkey_dev_check_presence (struct net_device *dev); -int ipoib_dev_init (struct net_device *dev,struct ib_device *ca,int port); -void ipoib_mcast_join_task (struct work_struct *work);void ipoib_mcast_carrier_on_task (struct work_struct *work);void ipoib_mcast_send (struct net_device *dev,u8 *daddr,struct sk_buff *skb);
@@ -597,7 +595,6 @@ void ipoib_pkey_open(struct ipoib_dev_priv *priv);void ipoib_drain_cq (struct net_device *dev);void ipoib_set_ethtool_ops (struct net_device *dev); -void ipoib_set_dev_features (struct ipoib_dev_priv *priv,struct ib_device *hca);#define IPOIB_FLAGS_RC 0x80#define IPOIB_FLAGS_UC 0x40
drivers/infiniband/ulp/ipoib/ipoib_main.c +111 -82
260 lines not on this page · raw message
drivers/infiniband/ulp/ipoib/ipoib_netlink.c +0 -6
@@ -125,12 +125,6 @@ static int ipoib_new_child_link(struct net *src_net, struct net_device *dev,if (child_pkey ==0 || child_pkey ==0x8000 )return -EINVAL ; -/* -* Set the full membership bit, so that we join the right -* broadcast group, etc. -*/ - child_pkey |=0x8000 ; - err =__ipoib_vlan_add (ppriv,ipoib_priv (dev), child_pkey,IPOIB_RTNL_CHILD );
drivers/infiniband/ulp/ipoib/ipoib_vlan.c +3 -28
@@ -55,35 +55,14 @@ int __ipoib_vlan_add(struct ipoib_dev_priv *ppriv, struct ipoib_dev_priv *priv, {int result; - priv->max_ib_mtu = ppriv->max_ib_mtu; -/* MTU will be reset when mcast join happens */ - priv->dev->mtu =IPOIB_UD_MTU (priv->max_ib_mtu); - priv->mcast_mtu = priv->admin_mtu = priv->dev->mtu; priv->parent = ppriv->dev; -set_bit (IPOIB_FLAG_SUBINTERFACE , &priv->flags); - -ipoib_set_dev_features (priv, ppriv->ca); - priv->pkey = pkey; - -memcpy (priv->dev->dev_addr, ppriv->dev->dev_addr,INFINIBAND_ALEN ); -memcpy (&priv->local_gid, &ppriv->local_gid,sizeof (priv->local_gid)); -set_bit (IPOIB_FLAG_DEV_ADDR_SET , &priv->flags); - priv->dev->broadcast[8 ] = pkey >>8 ; - priv->dev->broadcast[9 ] = pkey &0xff ; - - result =ipoib_dev_init (priv->dev, ppriv->ca, ppriv->port); -if (result <0 ) { -ipoib_warn (ppriv,"failed to initialize subinterface: " -"device %s, port %d" , - ppriv->ca->name, ppriv->port); - goto err; - } + priv->child_type = type; result =register_netdevice (priv->dev);if (result) {ipoib_warn (priv,"failed to initialize; error %i" , result); - goto err; +return result; }/* RTNL childs don't need proprietary sysfs entries */
@@ -99,17 +78,13 @@ int __ipoib_vlan_add(struct ipoib_dev_priv *ppriv, struct ipoib_dev_priv *priv, goto sysfs_failed; } - priv->child_type = type;list_add_tail (&priv->list, &ppriv->child_intfs);return 0 ; sysfs_failed: - result = -ENOMEM ;unregister_netdevice (priv->dev); - -err: -return result; +return -ENOMEM ; }int ipoib_vlan_add (struct net_device *pdev,unsigned short pkey)
Signature · 1 line
2.14.4
PATCH 6/10 RDMA/netdev: Use priv_destructor for netdev cleanup
In reply to Leon RomanovskyFrom: Jason Gunthorpe <[email protected]>
Now that the unregister_netdev flow for IPoIB no longer relies on external code we can now introduce the use of priv_destructor and needs_free_netdev.
The rdma_netdev flow is switched to use the netdev common priv_destructor instead of the special free_rdma_netdev and the IPOIB ULP adjusted:
- priv_destructor needs to switch to point to the ULP's destructor which will then call the rdma_ndev's in the right order
- We need to be careful around the error unwind of register_netdev as it sometimes calls priv_destructor on failure
- ULPs need to use ndo_init/uninit to ensure proper ordering of failures around register_netdev
Switching to priv_destructor is a necessary pre-requisite to using the rtnl new_link mechanism.
The VNIC user for rdma_netdev should also be revised, but that is left for another patch.
--- drivers/infiniband/hw/mlx5/main.c | 10 -- drivers/infiniband/ulp/ipoib/ipoib.h | 2 + drivers/infiniband/ulp/ipoib/ipoib_main.c | 101 +++++++++++++-------- drivers/infiniband/ulp/ipoib/ipoib_vlan.c | 68 ++++++++------ .../net/ethernet/mellanox/mlx5/core/ipoib/ipoib.c | 37 ++++---- include/linux/mlx5/driver.h | 3 - include/rdma/ib_verbs.h | 6 +- 7 files changed, 129 insertions(+), 98 deletions(-)
drivers/infiniband/hw/mlx5/main.c +0 -10
@@ -5157,11 +5157,6 @@ static int mlx5_ib_get_hw_stats(struct ib_device *ibdev,return num_counters; } -static void mlx5_ib_free_rdma_netdev (struct net_device *netdev) -{ -return mlx5_rdma_netdev_free (netdev); -} -static struct net_device * mlx5_ib_alloc_rdma_netdev(struct ib_device *hca, u8 port_num,
@@ -5171,17 +5166,12 @@ mlx5_ib_alloc_rdma_netdev(struct ib_device *hca, void (*setup)(struct net_device *)) {struct net_device *netdev; -struct rdma_netdev *rn;if (type !=RDMA_NETDEV_IPOIB )return ERR_PTR (-EOPNOTSUPP ); netdev =mlx5_rdma_netdev_alloc (to_mdev (hca)->mdev, hca, name, setup); -if (likely (!IS_ERR_OR_NULL (netdev))) { - rn =netdev_priv (netdev); - rn->free_rdma_netdev = mlx5_ib_free_rdma_netdev; - }return netdev; }
drivers/infiniband/ulp/ipoib/ipoib.h +2 -0
@@ -323,6 +323,7 @@ struct ipoib_dev_priv {spinlock_t lock;struct net_device *dev; +void (*next_priv_destructor)(struct net_device *dev);struct napi_struct send_napi;struct napi_struct recv_napi;
@@ -481,6 +482,7 @@ static inline void ipoib_put_ah(struct ipoib_ah *ah)kref_put (&ah->ref, ipoib_free_ah); }int ipoib_open (struct net_device *dev); +void ipoib_intf_free (struct net_device *dev);int ipoib_add_pkey_attr (struct net_device *dev);int ipoib_add_umcast_attr (struct net_device *dev);
drivers/infiniband/ulp/ipoib/ipoib_main.c +64 -37
175 lines not on this page · raw message
drivers/infiniband/ulp/ipoib/ipoib_vlan.c +39 -29
123 lines not on this page · raw message
drivers/net/ethernet/mellanox/mlx5/core/ipoib/ipoib.c +19 -18
@@ -580,6 +580,22 @@ static int mlx5i_check_required_hca_cap(struct mlx5_core_dev *mdev)return 0 ; } +static void mlx5_rdma_netdev_free (struct net_device *netdev) +{ +struct mlx5e_priv *priv =mlx5i_epriv (netdev); +struct mlx5i_priv *ipriv = priv->ppriv; +const struct mlx5e_profile *profile = priv->profile; + +mlx5e_detach_netdev (priv); + profile->cleanup (priv); +destroy_workqueue (priv->wq); + +if (!ipriv->sub_interface) { +mlx5i_pkey_qpn_ht_cleanup (netdev); +mlx5e_destroy_mdev_resources (priv->mdev); + } +} + struct net_device *mlx5_rdma_netdev_alloc(struct mlx5_core_dev *mdev, struct ib_device *ibdev, const char *name,
@@ -653,6 +669,9 @@ struct net_device *mlx5_rdma_netdev_alloc(struct mlx5_core_dev *mdev, rn->detach_mcast = mlx5i_detach_mcast; rn->set_id = mlx5i_set_pkey_index; + netdev->priv_destructor = mlx5_rdma_netdev_free; + netdev->needs_free_netdev =1 ; +return netdev; destroy_ht:
@@ -665,21 +684,3 @@ struct net_device *mlx5_rdma_netdev_alloc(struct mlx5_core_dev *mdev,return NULL ; }EXPORT_SYMBOL (mlx5_rdma_netdev_alloc); - -void mlx5_rdma_netdev_free (struct net_device *netdev) -{ -struct mlx5e_priv *priv =mlx5i_epriv (netdev); -struct mlx5i_priv *ipriv = priv->ppriv; -const struct mlx5e_profile *profile = priv->profile; - -mlx5e_detach_netdev (priv); - profile->cleanup (priv); -destroy_workqueue (priv->wq); - -if (!ipriv->sub_interface) { -mlx5i_pkey_qpn_ht_cleanup (netdev); -mlx5e_destroy_mdev_resources (priv->mdev); - } -free_netdev (netdev); -} -EXPORT_SYMBOL (mlx5_rdma_netdev_free);
include/linux/mlx5/driver.h +0 -3
@@ -1218,14 +1218,11 @@ struct net_device *mlx5_rdma_netdev_alloc(struct mlx5_core_dev *mdev, {return ERR_PTR (-EOPNOTSUPP ); } - -static inline void mlx5_rdma_netdev_free (struct net_device *netdev) {}#else struct net_device *mlx5_rdma_netdev_alloc (struct mlx5_core_dev *mdev,struct ib_device *ibdev,const char *name,void (*setup)(struct net_device *)); -void mlx5_rdma_netdev_free (struct net_device *netdev);#endif /* CONFIG_MLX5_CORE_IPOIB */ struct mlx5_profile {
include/rdma/ib_verbs.h +5 -1
@@ -2202,7 +2202,11 @@ struct rdma_netdev {struct ib_device *hca;u8 port_num; -/* cleanup function must be specified */ +/* +* cleanup function must be specified. +* FIXME: This is only used for OPA_VNIC and that usage should be +* removed too. +*/ void (*free_rdma_netdev)(struct net_device *netdev);/* control functions */
PATCH 7/10 IB/ipoib: Get rid of the sysfs_mutex
In reply to Leon RomanovskyFrom: Jason Gunthorpe <[email protected]>
This mutex was introduced to deal with the deadlock formed by calling unregister_netdev from within the sysfs callback of a netdev.
Now that we have priv_destructor and needs_free_netdev we can switch to the more targeted solution of running the unregister from a work queue. This avoids the deadlock and gets rid of the mutex.
The next patch in the series needs this mutex eliminated to create atomicity of unregisteration.
--- drivers/infiniband/ulp/ipoib/ipoib.h | 1 - drivers/infiniband/ulp/ipoib/ipoib_cm.c | 7 --- drivers/infiniband/ulp/ipoib/ipoib_main.c | 7 +-- drivers/infiniband/ulp/ipoib/ipoib_vlan.c | 98 ++++++++++++++++++++----------- 4 files changed, 65 insertions(+), 48 deletions(-)
drivers/infiniband/ulp/ipoib/ipoib.h +0 -1
@@ -332,7 +332,6 @@ struct ipoib_dev_priv {struct rw_semaphore vlan_rwsem;struct mutex mcast_mutex; -struct mutex sysfs_mutex;struct rb_root path_tree;struct list_head path_list;
drivers/infiniband/ulp/ipoib/ipoib_cm.c +0 -7
@@ -1514,19 +1514,13 @@ static ssize_t set_mode(struct device *d, struct device_attribute *attr, {struct net_device *dev =to_net_dev (d);int ret; -struct ipoib_dev_priv *priv =ipoib_priv (dev); - -if (!mutex_trylock (&priv->sysfs_mutex)) -return restart_syscall ();if (!rtnl_trylock ()) { -mutex_unlock (&priv->sysfs_mutex);return restart_syscall (); }if (dev->reg_state !=NETREG_REGISTERED ) {rtnl_unlock (); -mutex_unlock (&priv->sysfs_mutex);return -EPERM ; }
@@ -1538,7 +1532,6 @@ static ssize_t set_mode(struct device *d, struct device_attribute *attr,*/ if (ret != -EBUSY )rtnl_unlock (); -mutex_unlock (&priv->sysfs_mutex);return (!ret || ret == -EBUSY ) ? count : ret; }
drivers/infiniband/ulp/ipoib/ipoib_main.c +1 -6
@@ -2079,7 +2079,6 @@ static void ipoib_build_priv(struct net_device *dev)spin_lock_init (&priv->lock);init_rwsem (&priv->vlan_rwsem);mutex_init (&priv->mcast_mutex); -mutex_init (&priv->sysfs_mutex);INIT_LIST_HEAD (&priv->path_list);INIT_LIST_HEAD (&priv->child_intfs);
@@ -2476,10 +2475,7 @@ static void ipoib_remove_one(struct ib_device *device, void *client_data)list_for_each_entry_safe (priv, tmp, dev_list, list) {ipoib_parent_unregister_pre (priv->dev); -/* Wrap rtnl_lock/unlock with mutex to protect sysfs calls */ -mutex_lock (&priv->sysfs_mutex);unregister_netdev (priv->dev); -mutex_unlock (&priv->sysfs_mutex); }kfree (dev_list);
@@ -2527,8 +2523,7 @@ static int __init ipoib_init_module(void)* its private workqueue, and we only queue up flush events * on our global flush workqueue. This avoids the deadlocks. */ - ipoib_workqueue =alloc_ordered_workqueue ("ipoib_flush" , -WQ_MEM_RECLAIM ); + ipoib_workqueue =alloc_ordered_workqueue ("ipoib_flush" ,0 );if (!ipoib_workqueue) { ret = -ENOMEM ; goto err_fs;
drivers/infiniband/ulp/ipoib/ipoib_vlan.c +64 -34
145 lines not on this page · raw message
Signature · 1 line
2.14.4
PATCH 8/10 IB/ipoib: Do not remove child devices from within the ndo_uninit
In reply to Leon RomanovskyFrom: Jason Gunthorpe <[email protected]>
Switching to priv_destructor and needs_free_netdev created a subtle ordering problem in ipoib_remove_one.
Now that unregister_netdev frees the netdev and priv we must ensure that the children are unregistered before trying to unregister the parent, or child unregister will use after free.
The solution is to unregister the children, then parent, in the same batch all while holding the rtnl_lock. This closes all the races where a new child could have been added and ensures proper ordering.
--- drivers/infiniband/ulp/ipoib/ipoib.h | 7 +++++++ drivers/infiniband/ulp/ipoib/ipoib_main.c | 28 +++++++++++++++++----------- drivers/infiniband/ulp/ipoib/ipoib_vlan.c | 6 ++++++ 3 files changed, 30 insertions(+), 11 deletions(-)
drivers/infiniband/ulp/ipoib/ipoib.h +7 -0
@@ -330,6 +330,13 @@ struct ipoib_dev_priv {unsigned long flags; +/* +* This protects access to the child_intfs list. +* To READ from child_intfs the RTNL or vlan_rwsem read side must be +* held. To WRITE RTNL and the vlan_rwsem write side must be held (in +* that order) This lock exists because we have a few contexts where +* we need the child_intfs, but do not want to grab the RTNL. +*/ struct rw_semaphore vlan_rwsem;struct mutex mcast_mutex;
drivers/infiniband/ulp/ipoib/ipoib_main.c +17 -11
@@ -1939,18 +1939,15 @@ static int ipoib_ndo_init(struct net_device *ndev)static void ipoib_ndo_uninit (struct net_device *dev) { -struct ipoib_dev_priv *priv =ipoib_priv (dev), *cpriv, *tcpriv; -LIST_HEAD (head); +struct ipoib_dev_priv *priv =ipoib_priv (dev);ASSERT_RTNL (); -/* Delete any child interfaces first */ -list_for_each_entry_safe (cpriv, tcpriv, &priv->child_intfs, list) { -/* Stop GC on child */ -cancel_delayed_work_sync (&cpriv->neigh_reap_task); -unregister_netdevice_queue (cpriv->dev, &head); - } -unregister_netdevice_many (&head); +/* +* ipoib_remove_one guarantees the children are removed before the +* parent, and that is the only place where a parent can be removed. +*/ +WARN_ON (!list_empty (&priv->child_intfs));ipoib_neigh_hash_uninit (dev);
@@ -2466,16 +2463,25 @@ static void ipoib_add_one(struct ib_device *device)static void ipoib_remove_one (struct ib_device *device,void *client_data) { -struct ipoib_dev_priv *priv, *tmp; +struct ipoib_dev_priv *priv, *tmp, *cpriv, *tcpriv;struct list_head *dev_list = client_data;if (!dev_list)return ;list_for_each_entry_safe (priv, tmp, dev_list, list) { +LIST_HEAD (head);ipoib_parent_unregister_pre (priv->dev); -unregister_netdev (priv->dev); +rtnl_lock (); + +list_for_each_entry_safe (cpriv, tcpriv, &priv->child_intfs, + list) +unregister_netdevice_queue (cpriv->dev, &head); +unregister_netdevice_queue (priv->dev, &head); +unregister_netdevice_many (&head); + +rtnl_unlock (); }kfree (dev_list);
drivers/infiniband/ulp/ipoib/ipoib_vlan.c +6 -0
@@ -67,6 +67,12 @@ int __ipoib_vlan_add(struct ipoib_dev_priv *ppriv, struct ipoib_dev_priv *priv,ASSERT_RTNL (); +/* +* Racing with unregister of the parent must be prevented by the +* caller. +*/ +WARN_ON (ppriv->dev->reg_state !=NETREG_REGISTERED ); + priv->parent = ppriv->dev; priv->pkey = pkey; priv->child_type = type;
Signature · 1 line
2.14.4
PATCH 9/10 IB/ipoib: Maintain the child_intfs list from ndo_init/uninit
In reply to Leon RomanovskyFrom: Jason Gunthorpe <[email protected]>
This fixes a bug in the netlink path where the vlan_rwsem was not held around __ipoib_vlan_add causing the child_intfs to be manipulated unsafely.
In the process this greatly simplifies the vlan_rwsem write side locking to only cover a single non-sleeping statement.
This also further increases the safety of the removal ordering by holding the netdev of the parent while the child is active to ensure most bugs become either an oops on a NULL priv or a deadlock on the netdev refcount.
--- drivers/infiniband/ulp/ipoib/ipoib_main.c | 16 ++++++++++++++++ drivers/infiniband/ulp/ipoib/ipoib_netlink.c | 14 -------------- drivers/infiniband/ulp/ipoib/ipoib_vlan.c | 12 ------------ 3 files changed, 16 insertions(+), 26 deletions(-)
drivers/infiniband/ulp/ipoib/ipoib_main.c +16 -0
@@ -1890,6 +1890,12 @@ static void ipoib_child_init(struct net_device *ndev)struct ipoib_dev_priv *priv =ipoib_priv (ndev);struct ipoib_dev_priv *ppriv =ipoib_priv (priv->parent); +dev_hold (priv->parent); + +down_write (&ppriv->vlan_rwsem); +list_add_tail (&priv->list, &ppriv->child_intfs); +up_write (&ppriv->vlan_rwsem); + priv->max_ib_mtu = ppriv->max_ib_mtu;set_bit (IPOIB_FLAG_SUBINTERFACE , &priv->flags);memcpy (priv->dev->dev_addr, ppriv->dev->dev_addr,INFINIBAND_ALEN );
@@ -1959,6 +1965,16 @@ static void ipoib_ndo_uninit(struct net_device *dev)destroy_workqueue (priv->wq); priv->wq =NULL ; } + +if (priv->parent) { +struct ipoib_dev_priv *ppriv =ipoib_priv (priv->parent); + +down_write (&ppriv->vlan_rwsem); +list_del (&priv->list); +up_write (&ppriv->vlan_rwsem); + +dev_put (priv->parent); + } }static int ipoib_set_vf_link_state (struct net_device *dev,int vf,int link_state)
drivers/infiniband/ulp/ipoib/ipoib_netlink.c +0 -14
@@ -133,19 +133,6 @@ static int ipoib_new_child_link(struct net *src_net, struct net_device *dev,return err; } -static void ipoib_unregister_child_dev (struct net_device *dev,struct list_head *head) -{ -struct ipoib_dev_priv *priv, *ppriv; - - priv =ipoib_priv (dev); - ppriv =ipoib_priv (priv->parent); - -down_write (&ppriv->vlan_rwsem); -unregister_netdevice_queue (dev, head); -list_del (&priv->list); -up_write (&ppriv->vlan_rwsem); -} -static size_t ipoib_get_size (const struct net_device *dev) { return nla_total_size(2) + /* IFLA_IPOIB_PKEY */
@@ -161,7 +148,6 @@ static struct rtnl_link_ops ipoib_link_ops __read_mostly = { .setup = ipoib_setup_common, .newlink = ipoib_new_child_link, .changelink = ipoib_changelink, - .dellink = ipoib_unregister_child_dev, .get_size = ipoib_get_size, .fill_info = ipoib_fill_info, };
drivers/infiniband/ulp/ipoib/ipoib_vlan.c +0 -12
@@ -106,8 +106,6 @@ int __ipoib_vlan_add(struct ipoib_dev_priv *ppriv, struct ipoib_dev_priv *priv, goto sysfs_failed; } -list_add_tail (&priv->list, &ppriv->child_intfs); -return 0 ; sysfs_failed:
@@ -139,11 +137,6 @@ int ipoib_vlan_add(struct net_device *pdev, unsigned short pkey)return -EPERM ; } -if (!down_write_trylock (&ppriv->vlan_rwsem)) { -rtnl_unlock (); -return restart_syscall (); - } -/* * First ensure this isn't a duplicate. We check the parent device and * then all of the legacy child interfaces to make sure the Pkey
@@ -175,7 +168,6 @@ int ipoib_vlan_add(struct net_device *pdev, unsigned short pkey)free_netdev (ndev); out: -up_write (&ppriv->vlan_rwsem);rtnl_unlock ();return result;
@@ -209,10 +201,6 @@ static void ipoib_vlan_delete_task(struct work_struct *work)struct ipoib_dev_priv *priv =ipoib_priv (dev);struct ipoib_dev_priv *ppriv =ipoib_priv (priv->parent); -down_write (&ppriv->vlan_rwsem); -list_del (&priv->list); -up_write (&ppriv->vlan_rwsem); -ipoib_dbg (ppriv,"delete child vlan %s\n" , dev->name);unregister_netdevice (dev); }
Signature · 1 line
2.14.4
PATCH 10/10 IB/ipoib: Consolidate checking of the proposed child interface
In reply to Leon RomanovskyFrom: Jason Gunthorpe <[email protected]>
Move all the checking for pkey and other validity to the __ipoib_vlan_add function. This removes the last difference from the control flow of the __ipoib_vlan_add to make the overall design simpler to understand.
--- drivers/infiniband/ulp/ipoib/ipoib_netlink.c | 3 -- drivers/infiniband/ulp/ipoib/ipoib_vlan.c | 77 +++++++++++++++++++--------- 2 files changed, 52 insertions(+), 28 deletions(-)
drivers/infiniband/ulp/ipoib/ipoib_netlink.c +0 -3
@@ -122,9 +122,6 @@ static int ipoib_new_child_link(struct net *src_net, struct net_device *dev, }else child_pkey =nla_get_u16 (data[IFLA_IPOIB_PKEY ]); -if (child_pkey ==0 || child_pkey ==0x8000 ) -return -EINVAL ; - err =__ipoib_vlan_add (ppriv,ipoib_priv (dev), child_pkey,IPOIB_RTNL_CHILD );
drivers/infiniband/ulp/ipoib/ipoib_vlan.c +52 -25
123 lines not on this page · raw message
Signature · 1 line
2.14.4
Jason Gunthorpe
AppliedQuoted 16 lines · Leon Romanovsky
From: Leon Romanovsky <[email protected]>
IP link was broken due to the changes in IPoIB for the
rdma_netdevsupport after commitcd565b4b51e5("IB/IPoIB: Support acceleration options callbacks").This patchset restores IPoIB pkey creation and removal using rtnetlink.
It is completely rewritten variant of
https://marc.info/?l=linux-rdma&m=151553425830918&w=2 patch series.Thanks
Erez Shitrit (2):
IB/ipoib: Usecancel_delayed_work_syncfor neigh-clean task IB/ipoib: Makeipoib_neigh_hash_uninitfully fence its work
Quoted 9 lines
Jason Gunthorpe (8):
IB/ipoib: Get rid ofIPOIB_FLAG_GOING_DOWN
IB/ipoib: Move all uninit code intondo_uninitIB/ipoib: Move init code tondo_init
RDMA/netdev: Usepriv_destructorfor netdev cleanup IB/ipoib: Get rid of thesysfs_mutex
IB/ipoib: Do not remove child devices from within thendo_uninitIB/ipoib: Maintain thechild_intfslist from ndo_init/uninit IB/ipoib: Consolidate checking of the proposed child interface
Applied to for-next, finally this bug is fixed..
I squashed these two patches though:
IB/ipoib: Use cancel_delayed_work_sync for neigh-clean task IB/ipoib: Make ipoib_neigh_hash_uninit fully fence its work
Thanks,
Jason