I ran a parallel create/destroy/remove test overnight and something
deadlocked. Running with lockdep enabled gives a reproducible warning,
but I'm having trouble making sense of it. I'm not sure I understand
what the "events" lock is here.
Chris
=======================================================
[ INFO: possible circular locking dependency detected ]
2.6.31-rc2-cdl #112
-------------------------------------------------------
fcoeadm/4021 is trying to acquire lock:
(events){+.+.+.}, at: [<ffffffff8105c744>] flush_work+0x3a/0xf0
but task is already holding lock:
(rtnl_mutex){+.+.+.}, at: [<ffffffff813057e5>] rtnl_lock+0x12/0x14
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #2 (rtnl_mutex){+.+.+.}:
[<ffffffff8107039c>] __lock_acquire+0xa8c/0xc14
[<ffffffff810705eb>] lock_acquire+0xc7/0xf3
[<ffffffff813965e2>] __mutex_lock_common+0x48/0x39a
[<ffffffff813969dd>] mutex_lock_nested+0x35/0x3a
[<ffffffff813057e5>] rtnl_lock+0x12/0x14
[<ffffffff813067af>] linkwatch_event+0x9/0x27
[<ffffffff8105baee>] worker_thread+0x200/0x313
[<ffffffff8105ffd4>] kthread+0x88/0x90
[<ffffffff81012d2a>] child_rip+0xa/0x20
[<ffffffffffffffff>] 0xffffffffffffffff
-> #1 ((linkwatch_work).work){+.+.+.}:
[<ffffffff8107039c>] __lock_acquire+0xa8c/0xc14
[<ffffffff810705eb>] lock_acquire+0xc7/0xf3
[<ffffffff8105bae5>] worker_thread+0x1f7/0x313
[<ffffffff8105ffd4>] kthread+0x88/0x90
[<ffffffff81012d2a>] child_rip+0xa/0x20
[<ffffffffffffffff>] 0xffffffffffffffff
-> #0 (events){+.+.+.}:
[<ffffffff81070290>] __lock_acquire+0x980/0xc14
[<ffffffff810705eb>] lock_acquire+0xc7/0xf3
[<ffffffff8105c76f>] flush_work+0x65/0xf0
[<ffffffffa0054c01>] fcoe_ctlr_destroy+0x25/0xa6 [libfcoe]
[<ffffffffa011309f>] __fcoe_destroy+0xe7/0x203 [fcoe]
[<ffffffffa01131ec>] fcoe_destroy+0x31/0x57 [fcoe]
[<ffffffff8105e378>] param_attr_store+0x25/0x35
[<ffffffff8105e3cd>] module_attr_store+0x21/0x25
[<ffffffff81142511>] sysfs_write_file+0xe4/0x119
[<ffffffff810efe6e>] vfs_write+0xab/0x105
[<ffffffff810eff8c>] sys_write+0x47/0x6f
[<ffffffff81011b42>] system_call_fastpath+0x16/0x1b
[<ffffffffffffffff>] 0xffffffffffffffff
other info that might help us debug this:
2 locks held by fcoeadm/4021:
#0: (&buffer->mutex){+.+.+.}, at: [<ffffffff81142465>]
sysfs_write_file+0x38/0x119
#1: (rtnl_mutex){+.+.+.}, at: [<ffffffff813057e5>] rtnl_lock+0x12/0x14
stack backtrace:
Pid: 4021, comm: fcoeadm Not tainted 2.6.31-rc2-cdl #112
Call Trace:
[<ffffffff8106f59a>] print_circular_bug_tail+0x71/0x7c
[<ffffffff81070290>] __lock_acquire+0x980/0xc14
[<ffffffff810705eb>] lock_acquire+0xc7/0xf3
[<ffffffff8105c744>] ? flush_work+0x3a/0xf0
[<ffffffff8105c76f>] flush_work+0x65/0xf0
[<ffffffff8105c744>] ? flush_work+0x3a/0xf0
[<ffffffff8106e9f5>] ? mark_lock+0x22/0x227
[<ffffffff8106ec47>] ? mark_held_locks+0x4d/0x6b
[<ffffffff8139640f>] ? __mutex_unlock_slowpath+0x128/0x13b
[<ffffffff8106eeda>] ? trace_hardirqs_on_caller+0x110/0x134
[<ffffffff8106ef0b>] ? trace_hardirqs_on+0xd/0xf
[<ffffffffa0054c01>] fcoe_ctlr_destroy+0x25/0xa6 [libfcoe]
[<ffffffffa011309f>] __fcoe_destroy+0xe7/0x203 [fcoe]
[<ffffffffa01121a6>] ? fcoe_if_to_netdev+0x5c/0x63 [fcoe]
[<ffffffffa01131ec>] fcoe_destroy+0x31/0x57 [fcoe]
[<ffffffff8105e378>] param_attr_store+0x25/0x35
[<ffffffff8105e3cd>] module_attr_store+0x21/0x25
[<ffffffff81142511>] sysfs_write_file+0xe4/0x119
[<ffffffff810efe6e>] vfs_write+0xab/0x105
[<ffffffff810eff8c>] sys_write+0x47/0x6f
[<ffffffff81011b42>] system_call_fastpath+0x16/0x1b
On Wed, Jul 08, 2009 at 06:04:41PM -0700, Joe Eykholt wrote:
> Chris Leech wrote:
> > Rather than rely on the hostlist_lock to be held while creating exchange
> > managers, serialize fcoe instance creation and destruction with a mutex.
> > This will allow the hostlist addition to be moved out of fcoe_if_create(),
> > which will simplify NPIV support.
> >
> > RTNL mutex was used because it needs to be held in parts of both the create
> > and destroy path anyway, and because the netdevice notifier will be called
> > with RTNL held. If a new mutex was added and fine-grained use of RTNL was
> > maintained, then destroying the interface when the netdevice was
> > unregistered
> > would deadlock on RTNL unless it was deferred using a workqueue or similar.
> >
> > Signed-off-by: Chris Leech <[email protected]>
> > ---
> >
> > drivers/scsi/fcoe/fcoe.c | 68
> > ++++++++++++++++++++++++++++------------------
> > 1 files changed, 41 insertions(+), 27 deletions(-)
> >
> > diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
> > index 49a9d5b..a4b8991 100644
> > --- a/drivers/scsi/fcoe/fcoe.c
> > +++ b/drivers/scsi/fcoe/fcoe.c
> > @@ -49,9 +49,9 @@ MODULE_AUTHOR("Open-FCoE.org");
> > MODULE_DESCRIPTION("FCoE");
> > MODULE_LICENSE("GPL v2");
> >
> > -/* fcoe host list */
> > +/* fcoe host list
> > + * must be accessed under rtnl_lock */
> > LIST_HEAD(fcoe_hostlist);
> > -DEFINE_RWLOCK(fcoe_hostlist_lock);
> > DEFINE_PER_CPU(struct fcoe_percpu_s, fcoe_percpu);
> >
> > /* Function Prototypes */
> > @@ -186,13 +186,11 @@ static int fcoe_interface_setup(struct fcoe_interface
> > *fcoe,
> > * or enter promiscuous mode if not capable of listening
> > * for multiple unicast MACs.
> > */
> > - rtnl_lock();
> > memcpy(flogi_maddr, (u8[6]) FC_FCOE_FLOGI_MAC, ETH_ALEN);
> > dev_unicast_add(netdev, flogi_maddr);
> > if (fip->spma)
> > dev_unicast_add(netdev, fip->ctl_src_addr);
> > dev_mc_add(netdev, FIP_ALL_ENODE_MACS, ETH_ALEN, 0);
> > - rtnl_unlock();
> >
> > /*
> > * setup the receive function from ethernet driver
> > @@ -259,7 +257,6 @@ void fcoe_interface_cleanup(struct fcoe_interface *fcoe)
> > dev_remove_pack(&fcoe->fip_packet_type);
> >
> > /* Delete secondary MAC addresses */
> > - rtnl_lock();
> > memcpy(flogi_maddr, (u8[6]) FC_FCOE_FLOGI_MAC, ETH_ALEN);
> > dev_unicast_delete(netdev, flogi_maddr);
> > if (!is_zero_ether_addr(fip->data_src_addr))
> > @@ -267,7 +264,6 @@ void fcoe_interface_cleanup(struct fcoe_interface *fcoe)
> > if (fip->spma)
> > dev_unicast_delete(netdev, fip->ctl_src_addr);
> > dev_mc_delete(netdev, FIP_ALL_ENODE_MACS, ETH_ALEN, 0);
> > - rtnl_unlock();
> > }
> >
> > /**
> > @@ -516,8 +512,6 @@ bool fcoe_oem_match(struct fc_frame *fp)
> > * fcoe_em_config() - allocates em for this lport
> > * @lp: the fcoe that em is to allocated for
> > *
> > - * Called with write fcoe_hostlist_lock held.
> > - *
> > * Returns : 0 on success
> > */
> > static inline int fcoe_em_config(struct fc_lport *lp)
> > @@ -739,11 +733,14 @@ static struct fc_lport *fcoe_if_create(struct
> > fcoe_interface *fcoe,
> >
> > /*
> > * fcoe_em_alloc() and fcoe_hostlist_add() both
> > - * need to be atomic under fcoe_hostlist_lock
> > + * need to be atomic with respect to other changes to the hostlist
> > * since fcoe_em_alloc() looks for an existing EM
> > * instance on host list updated by fcoe_hostlist_add().
> > + *
> > + * This is OK because the entire create and destroy paths are called
> > + * with the RTNL mutex held.
> > */
> > - write_lock(&fcoe_hostlist_lock);
> > +
> > /* lport exch manager allocation */
> > rc = fcoe_em_config(lport);
> > if (rc) {
> > @@ -754,7 +751,6 @@ static struct fc_lport *fcoe_if_create(struct
> > fcoe_interface *fcoe,
> >
> > /* add to lports list */
> > fcoe_hostlist_add(lport);
> > - write_unlock(&fcoe_hostlist_lock);
> >
> > dev_hold(netdev);
> > fcoe_interface_get(fcoe);
> > @@ -795,6 +791,7 @@ static int __init fcoe_if_init(void)
> > int __exit fcoe_if_exit(void)
> > {
> > fc_release_transport(scsi_transport_fcoe_sw);
> > + scsi_transport_fcoe_sw = NULL;
> > return 0;
> > }
> >
> > @@ -1524,14 +1521,14 @@ static int fcoe_device_notification(struct
> > notifier_block *notifier,
> > u32 mfs;
> > int rc = NOTIFY_OK;
> >
> > - read_lock(&fcoe_hostlist_lock);
> > + ASSERT_RTNL();
> > +
> > list_for_each_entry(fcoe, &fcoe_hostlist, list) {
> > if (fcoe->real_dev == real_dev) {
> > lp = fcoe->ctlr.lp;
> > break;
> > }
> > }
> > - read_unlock(&fcoe_hostlist_lock);
> > if (lp == NULL) {
> > rc = NOTIFY_DONE;
> > goto out;
> > @@ -1674,6 +1671,18 @@ static int fcoe_destroy(const char *buffer, struct
> > kernel_param *kp)
> > struct fc_lport *lport;
> > int rc;
> >
> > + rtnl_lock();
> > +
> > + /*
> > + * Make sure the module has been initialized, and is not about to be
> > + * removed. Module paramter sysfs files are writable before the
> > + * module_init function is called and after module_exit.
> > + */
> > + if (!scsi_transport_fcoe_sw) {
> > + rc = -ENODEV;
> > + goto out_nodev;
> > + }
> > +
> > netdev = fcoe_if_to_netdev(buffer);
> > if (!netdev) {
> > rc = -ENODEV;
> > @@ -1693,6 +1702,7 @@ static int fcoe_destroy(const char *buffer, struct
> > kernel_param *kp)
> > out_putdev:
> > dev_put(netdev);
> > out_nodev:
> > + rtnl_unlock();
> > return rc;
> > }
> >
> > @@ -1710,6 +1720,18 @@ static int fcoe_create(const char *buffer, struct
> > kernel_param *kp)
> > struct fc_lport *lport;
> > struct net_device *netdev;
> >
> > + rtnl_lock();
> > +
> > + /*
> > + * Make sure the module has been initialized, and is not about to be
> > + * removed. Module paramter sysfs files are writable before the
> > + * module_init function is called and after module_exit.
> > + */
> > + if (!scsi_transport_fcoe_sw) {
> > + rc = -ENODEV;
> > + goto out_nodev;
> > + }
> > +
> > netdev = fcoe_if_to_netdev(buffer);
> > if (!netdev) {
> > rc = -ENODEV;
> > @@ -1746,14 +1768,15 @@ static int fcoe_create(const char *buffer, struct
> > kernel_param *kp)
> > if (!fcoe_link_ok(lport))
> > fcoe_ctlr_link_up(&fcoe->ctlr);
> >
> > - dev_put(netdev);
> > - return 0;
> > + rc = 0;
> > + goto out_putdev;
> >
> > out_free:
> > fcoe_interface_put(fcoe);
> > out_putdev:
> > dev_put(netdev);
> > out_nodev:
> > + rtnl_unlock();
> > return rc;
> > }
> >
> > @@ -1872,8 +1895,6 @@ int fcoe_reset(struct Scsi_Host *shost)
> > * fcoe_hostlist_lookup_port() - find the corresponding lport by a given
> > device
> > * @dev: this is currently ptr to net_device
> > *
> > - * Called with fcoe_hostlist_lock held.
> > - *
> > * Returns: NULL or the located fcoe_port
> > */
> > static struct fcoe_interface *
> > @@ -1898,10 +1919,7 @@ struct fc_lport *fcoe_hostlist_lookup(const struct
> > net_device *netdev)
> > {
> > struct fcoe_interface *fcoe;
> >
> > - read_lock(&fcoe_hostlist_lock);
> > fcoe = fcoe_hostlist_lookup_port(netdev);
> > - read_unlock(&fcoe_hostlist_lock);
> > -
> > return (fcoe) ? fcoe->ctlr.lp : NULL;
> > }
> >
> > @@ -1909,8 +1927,6 @@ struct fc_lport *fcoe_hostlist_lookup(const struct
> > net_device *netdev)
> > * fcoe_hostlist_add() - Add a lport to lports list
> > * @lp: ptr to the fc_lport to be added
> > *
> > - * Called with write fcoe_hostlist_lock held.
> > - *
> > * Returns: 0 for success
> > */
> > int fcoe_hostlist_add(const struct fc_lport *lport)
> > @@ -1937,11 +1953,9 @@ int fcoe_hostlist_remove(const struct fc_lport
> > *lport)
> > {
> > struct fcoe_interface *fcoe;
> >
> > - write_lock_bh(&fcoe_hostlist_lock);
> > fcoe = fcoe_hostlist_lookup_port(fcoe_netdev(lport));
> > BUG_ON(!fcoe);
> > list_del(&fcoe->list);
> > - write_unlock_bh(&fcoe_hostlist_lock);
> >
> > return 0;
> > }
> > @@ -1957,9 +1971,6 @@ static int __init fcoe_init(void)
> > int rc = 0;
> > struct fcoe_percpu_s *p;
> >
> > - INIT_LIST_HEAD(&fcoe_hostlist);
> > - rwlock_init(&fcoe_hostlist_lock);
> > -
> > for_each_possible_cpu(cpu) {
> > p = &per_cpu(fcoe_percpu, cpu);
> > skb_queue_head_init(&p->fcoe_rx_list);
> > @@ -2000,6 +2011,7 @@ static void __exit fcoe_exit(void)
> > struct fcoe_interface *fcoe, *tmp;
> >
> > fcoe_dev_cleanup();
> > + rtnl_lock();
> >
> > /* releases the associated fcoe hosts */
> > list_for_each_entry_safe(fcoe, tmp, &fcoe_hostlist, list)
> > @@ -2012,5 +2024,7 @@ static void __exit fcoe_exit(void)
> >
> > /* detach from scsi transport */
> > fcoe_if_exit();
> > +
> > + rtnl_unlock();
> > }
> > module_exit(fcoe_exit);
> >
> > _______________________________________________
> > devel mailing list
> > [email protected]
> > http://www.open-fcoe.org/mailman/listinfo/devel
>
> This looks good to me. Thanks for doing it.
>
> Joe
>
_______________________________________________
devel mailing list
[email protected]
http://www.open-fcoe.org/mailman/listinfo/devel
