| Message ID | 1549416277-8732-1-git-send-email-pkaustub@cisco.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | [for-next,v2] IB/usnic: fix deadlock | expand |
On Tue, Feb 05, 2019 at 05:24:37PM -0800, Parvi Kaustubhi wrote: > There is a dead lock in usnic ib_register and netdev_notify > path. > > usnic_ib_discover_pf() > | mutex_lock(&usnic_ib_ibdev_list_lock); > | usnic_ib_device_add(); > | ib_register_device() > | usnic_ib_query_port() > | mutex_lock(&us_ibdev->usdev_lock); > | ib_get_eth_speed() > | rtnl_lock() > order of lock: &usnic_ib_ibdev_list_lock -> usdev_lock -> rtnl_lock > > rtnl_lock() > | usnic_ib_netdevice_event() > | mutex_lock(&usnic_ib_ibdev_list_lock); > > order of lock: rtnl_lock -> &usnic_ib_ibdev_list_lock > > Solution is to add notifier blocks (netdev_nb and inet_nb) for each pf > and use container_of to lookup usnic_ib_dev while handling netdev/ inet > events. This eliminates the need to acquire usnic_ib_ibdev_list_lock to > obtain the corresponding usnic_ib_dev pointer. What you should do is use this series: https://github.com/jgunthorpe/linux/commits/device_locking_cleanup And make a patch to convert usnic to use ib_device_set_netdev() and ib_device_get_by_netdev() and try to eliminate as many uses of the usnic_ib_ibdev_list as you can. Also, drivers should not hold any locks while calling ib_unregister_device() Jason
Incase of usnic, only the first vf for each enic pf (and not all vfs) registers with ib. However, each vf that registers with ib has it’s own netdevice. usnic_ib_ibdev_list_lock protects a list of the those “first” vfs that have registered with ib. This series : https://github.com/jgunthorpe/linux/commits/device_locking_cleanup , will not allow us to get rid of this list (and hence usnic_ib_ibdev_list_lock) completely. We can modify the current patch such that driver doesn’t hold any lock while doing ib_unregsiter_device() Thanks, Parvi > On Feb 5, 2019, at 7:22 PM, Jason Gunthorpe <jgg@ziepe.ca> wrote: > > On Tue, Feb 05, 2019 at 05:24:37PM -0800, Parvi Kaustubhi wrote: >> There is a dead lock in usnic ib_register and netdev_notify >> path. >> >> usnic_ib_discover_pf() >> | mutex_lock(&usnic_ib_ibdev_list_lock); >> | usnic_ib_device_add(); >> | ib_register_device() >> | usnic_ib_query_port() >> | mutex_lock(&us_ibdev->usdev_lock); >> | ib_get_eth_speed() >> | rtnl_lock() > >> order of lock: &usnic_ib_ibdev_list_lock -> usdev_lock -> rtnl_lock >> >> rtnl_lock() >> | usnic_ib_netdevice_event() >> | mutex_lock(&usnic_ib_ibdev_list_lock); >> >> order of lock: rtnl_lock -> &usnic_ib_ibdev_list_lock >> >> Solution is to add notifier blocks (netdev_nb and inet_nb) for each pf >> and use container_of to lookup usnic_ib_dev while handling netdev/ inet >> events. This eliminates the need to acquire usnic_ib_ibdev_list_lock to >> obtain the corresponding usnic_ib_dev pointer. > > What you should do is use this series: > > https://github.com/jgunthorpe/linux/commits/device_locking_cleanup > > And make a patch to convert usnic to use ib_device_set_netdev() and > ib_device_get_by_netdev() and try to eliminate as many uses of the > usnic_ib_ibdev_list as you can. > > Also, drivers should not hold any locks while calling > ib_unregister_device() > > Jason
On Thu, Feb 07, 2019 at 03:04:58AM +0000, Parvi Kaustubhi (pkaustub) wrote: > Incase of usnic, only the first vf for each enic pf (and not all > vfs) registers with ib. However, each vf that registers with ib has > it’s own netdevice. usnic_ib_ibdev_list_lock protects a list of the > those “first” vfs that have registered with ib. This series : > https://github.com/jgunthorpe/linux/commits/device_locking_cleanup , > will not allow us to get rid of this list (and hence > usnic_ib_ibdev_list_lock) completely. I wasn't thinking you could get rid of all of them, I did say 'many of them'. > We can modify the current patch such that driver doesn’t hold any > lock while doing ib_unregsiter_device() You should use the ib_device_set_netdev()/etc to eliminate the list from the netdev notifier path. There is also now an ib_unregister_device_queued() which might be useful here. Jason
diff --git a/drivers/infiniband/hw/usnic/usnic_ib.h b/drivers/infiniband/hw/usnic/usnic_ib.h index 525bf27..1461be0 100644 --- a/drivers/infiniband/hw/usnic/usnic_ib.h +++ b/drivers/infiniband/hw/usnic/usnic_ib.h @@ -82,6 +82,8 @@ struct usnic_ib_dev { /* sysfs vars for QPN reporting */ struct kobject *qpn_kobj; + struct notifier_block netdev_nb; + struct notifier_block inet_nb; }; struct usnic_ib_vf { diff --git a/drivers/infiniband/hw/usnic/usnic_ib_main.c b/drivers/infiniband/hw/usnic/usnic_ib_main.c index c4a4cfe..a6d54d7 100644 --- a/drivers/infiniband/hw/usnic/usnic_ib_main.c +++ b/drivers/infiniband/hw/usnic/usnic_ib_main.c @@ -219,21 +219,13 @@ static int usnic_ib_netdevice_event(struct notifier_block *notifier, struct net_device *netdev = netdev_notifier_info_to_dev(ptr); - mutex_lock(&usnic_ib_ibdev_list_lock); - list_for_each_entry(us_ibdev, &usnic_ib_ibdev_list, ib_dev_link) { - if (us_ibdev->netdev == netdev) { - usnic_ib_handle_usdev_event(us_ibdev, event); - break; - } - } - mutex_unlock(&usnic_ib_ibdev_list_lock); + us_ibdev = container_of(notifier, struct usnic_ib_dev, netdev_nb); + if (us_ibdev->netdev == netdev) + usnic_ib_handle_usdev_event(us_ibdev, event); return NOTIFY_DONE; } -static struct notifier_block usnic_ib_netdevice_notifier = { - .notifier_call = usnic_ib_netdevice_event -}; /* End of netdev section */ /* Start of inet section */ @@ -283,20 +275,12 @@ static int usnic_ib_inetaddr_event(struct notifier_block *notifier, struct in_ifaddr *ifa = ptr; struct net_device *netdev = ifa->ifa_dev->dev; - mutex_lock(&usnic_ib_ibdev_list_lock); - list_for_each_entry(us_ibdev, &usnic_ib_ibdev_list, ib_dev_link) { - if (us_ibdev->netdev == netdev) { - usnic_ib_handle_inet_event(us_ibdev, event, ptr); - break; - } - } - mutex_unlock(&usnic_ib_ibdev_list_lock); + us_ibdev = container_of(notifier, struct usnic_ib_dev, inet_nb); + if (us_ibdev->netdev == netdev) + usnic_ib_handle_inet_event(us_ibdev, event, ptr); return NOTIFY_DONE; } -static struct notifier_block usnic_ib_inetaddr_notifier = { - .notifier_call = usnic_ib_inetaddr_event -}; /* End of inet section*/ static int usnic_port_immutable(struct ib_device *ibdev, u8 port_num, @@ -361,6 +345,7 @@ static void *usnic_ib_device_add(struct pci_dev *dev) union ib_gid gid; struct in_device *ind; struct net_device *netdev; + int err; usnic_dbg("\n"); netdev = pci_get_drvdata(dev); @@ -418,6 +403,18 @@ static void *usnic_ib_device_add(struct pci_dev *dev) if (ib_register_device(&us_ibdev->ib_dev, "usnic_%d")) goto err_fwd_dealloc; + us_ibdev->netdev_nb.notifier_call = usnic_ib_netdevice_event; + err = register_netdevice_notifier(&us_ibdev->netdev_nb); + if (err) { + usnic_err("Failed to register netdev notifier\n"); + goto err_fwd_dealloc; + } + us_ibdev->inet_nb.notifier_call = usnic_ib_inetaddr_event; + err = register_inetaddr_notifier(&us_ibdev->inet_nb); + if (err) { + usnic_err("Failed to register inet addr notifier\n"); + goto err_netdev_notifier; + } usnic_fwd_set_mtu(us_ibdev->ufdev, us_ibdev->netdev->mtu); usnic_fwd_set_mac(us_ibdev->ufdev, us_ibdev->netdev->dev_addr); if (netif_carrier_ok(us_ibdev->netdev)) @@ -441,6 +438,8 @@ static void *usnic_ib_device_add(struct pci_dev *dev) us_ibdev->ufdev->link_up, us_ibdev->ufdev->mtu); return us_ibdev; +err_netdev_notifier: + unregister_netdevice_notifier(&us_ibdev->netdev_nb); err_fwd_dealloc: usnic_fwd_dev_free(us_ibdev->ufdev); err_dealloc: @@ -453,6 +452,8 @@ static void usnic_ib_device_remove(struct usnic_ib_dev *us_ibdev) { usnic_info("Unregistering %s\n", dev_name(&us_ibdev->ib_dev.dev)); usnic_ib_sysfs_unregister_usdev(us_ibdev); + unregister_inetaddr_notifier(&us_ibdev->inet_nb); + unregister_netdevice_notifier(&us_ibdev->netdev_nb); usnic_fwd_dev_free(us_ibdev->ufdev); ib_unregister_device(&us_ibdev->ib_dev); ib_dealloc_device(&us_ibdev->ib_dev); @@ -655,32 +656,16 @@ static int __init usnic_ib_init(void) goto out_umem_fini; } - err = register_netdevice_notifier(&usnic_ib_netdevice_notifier); - if (err) { - usnic_err("Failed to register netdev notifier\n"); - goto out_pci_unreg; - } - - err = register_inetaddr_notifier(&usnic_ib_inetaddr_notifier); - if (err) { - usnic_err("Failed to register inet addr notifier\n"); - goto out_unreg_netdev_notifier; - } - err = usnic_transport_init(); if (err) { usnic_err("Failed to initialize transport\n"); - goto out_unreg_inetaddr_notifier; + goto out_pci_unreg; } usnic_debugfs_init(); return 0; -out_unreg_inetaddr_notifier: - unregister_inetaddr_notifier(&usnic_ib_inetaddr_notifier); -out_unreg_netdev_notifier: - unregister_netdevice_notifier(&usnic_ib_netdevice_notifier); out_pci_unreg: pci_unregister_driver(&usnic_ib_pci_driver); out_umem_fini: @@ -694,8 +679,6 @@ static void __exit usnic_ib_destroy(void) usnic_dbg("\n"); usnic_debugfs_exit(); usnic_transport_fini(); - unregister_inetaddr_notifier(&usnic_ib_inetaddr_notifier); - unregister_netdevice_notifier(&usnic_ib_netdevice_notifier); pci_unregister_driver(&usnic_ib_pci_driver); usnic_uiom_fini(); }
There is a dead lock in usnic ib_register and netdev_notify path. usnic_ib_discover_pf() | mutex_lock(&usnic_ib_ibdev_list_lock); | usnic_ib_device_add(); | ib_register_device() | usnic_ib_query_port() | mutex_lock(&us_ibdev->usdev_lock); | ib_get_eth_speed() | rtnl_lock() order of lock: &usnic_ib_ibdev_list_lock -> usdev_lock -> rtnl_lock rtnl_lock() | usnic_ib_netdevice_event() | mutex_lock(&usnic_ib_ibdev_list_lock); order of lock: rtnl_lock -> &usnic_ib_ibdev_list_lock Solution is to add notifier blocks (netdev_nb and inet_nb) for each pf and use container_of to lookup usnic_ib_dev while handling netdev/ inet events. This eliminates the need to acquire usnic_ib_ibdev_list_lock to obtain the corresponding usnic_ib_dev pointer. Signed-off-by: Parvi Kaustubhi <pkaustub@cisco.com> Reviewed-by: Govindarajulu Varadarajan <gvaradar@cisco.com> Reviewed-by: Tanmay Inamdar <tinamdar@cisco.com> --- Changelog: v1->v2: * Have notifier blocks in usnic_ib_dev instead of using workqueue to defer event handling. --- drivers/infiniband/hw/usnic/usnic_ib.h | 2 + drivers/infiniband/hw/usnic/usnic_ib_main.c | 65 +++++++++++------------------ 2 files changed, 26 insertions(+), 41 deletions(-)