[Open-FCoE] [PATCH V2 11/13] fcoe: add mutex to protect create and destroy
christopher.leech at intel.com
Wed Jul 8 22:41:38 UTC 2009
On Wed, Jul 08, 2009 at 03:14:54PM -0700, Joe Eykholt wrote:
> 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?
I have new versions of the last 3 patches changed to work this way. Let
me do a little testing and then I'll send them out for review.
More information about the devel