[Open-FCoE] deadlock in testing (Re: [RFC PATCH V3 11/13] fcoe: use RTNL mutex to serialize create/destroy and protect hostlist)

Joe Eykholt jeykholt at cisco.com
Thu Jul 9 22:47:57 UTC 2009


Chris Leech wrote:
> 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.

I'm not sure why it says "events" either.  I think it has something
to do with flush_work() calling lock_map_acquire/release to indicate
that the work items it will wait on may need locks.

I see one problem, though.  fcoe_ctlr_destroy() is doing a
flush_work and it uses the general work thread.  So does
linkwatch_event(), which needs rtnl_lock().  So the flush_work()
may hang forever if there's a linkwatch_event queued.  Shoot.

Ways to fix it:
1) have FIP use its own work queue.
2) separately flush the FIP work queue while not holding rtnl_lock.
3) go back to using a separate mutex for fcoe create/delete, but
    use rtnl_lock for the hostlist to protect the notification.
4) something better?

	Joe



> 
> 	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 <christopher.leech at intel.com>
>>> ---
>>>
>>>  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
>>> devel at open-fcoe.org
>>> http://www.open-fcoe.org/mailman/listinfo/devel
>> This looks good to me.  Thanks for doing it.
>>
>> 	Joe
>>




More information about the devel mailing list