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 <[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
