[Open-FCoE] [PATCH V2 11/13] fcoe: add mutex to protect create and destroy

Joe Eykholt jeykholt at cisco.com
Wed Jul 8 22:14:54 UTC 2009


Chris Leech wrote:
> On Wed, Jul 08, 2009 at 02:24:16PM -0700, Joe Eykholt wrote:
>> Robert Love wrote:
>>> On Wed, 2009-07-08 at 10:40 -0700, 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.
>>>>
>>> Is there a reason that we don't use the rtnl_lock for protection? It's
>>> already held when we get the NETDEV_UNREGISTER event (added in patch
>>> 13/13). I believe that doing so would remove the need to defer the
>>> fcoe_destroy() call since the deferral is only needed so that we don't
>>> grab the rtnl_lock twice.
>> I considered that, too.  It seems a bit like a layering violation to use
>> it to protect create/delete/exit, but maybe it's OK to do since we
>> need to grab rtnl_lock anyway.
>>
>> In fcoe_exit() we would have to be careful because
>> unregister_netdevice_notifier() does rtnl_lock(),
>> but if we did the unregister before grabbing rtnl_lock,
>> that would be OK.
>>
>> It could completely eliminate the need for hostlist_lock and create_lock,
>> so it seems like a nice idea.
> 
> I thought there was somewhere outside of create/destroy that the
> host-list was being read, but it might just have been the module exit
> case.  Removing the lock and only having one mutex for all this would be
> cool.

The only other usage besides create/destroy is the netdev notification,
and that is called with rntl_lock held already.  Nice.

This also takes care of a worry I had that the notification would
find the fcoe_softc and be using it while a delete goes on.  That
can't happen currently because rtnl_lock is held in the notification
and would have to be taken at least once in fcoe_destroy().  Since
we found the softc on the list at the beginning of the notification,
we're sure fcoe_destroy() hasn't removed it yet, and won't completely
free it until it gets rtnl_lock.   So, to a small degree,
create/delete are already protected by rtnl_lock.

I think this would be a safe and clean change.  So, Chris, will
you work that into your patch set?

	Thanks,
	Joe





More information about the devel mailing list