Message-Id: <20140627121752.21c559c3404d665adbaa5b23@linux-foundation.org>
Date:	Fri, 27 Jun 2014 12:17:52 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Dan Streetman <ddstreet@...e.org>
Cc:	Seth Jennings <sjennings@...iantweb.net>,
	Minchan Kim <minchan@...nel.org>,
	Weijie Yang <weijie.yang@...sung.com>,
	Nitin Gupta <ngupta@...are.org>, Bob Liu <bob.liu@...cle.com>,
	Hugh Dickins <hughd@...gle.com>, Mel Gorman <mgorman@...e.de>,
	Rik van Riel <riel@...hat.com>,
	Johannes Weiner <hannes@...xchg.org>,
	Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
	Linux-MM <linux-mm@...ck.org>,
	linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCHv4 3/6] mm/zpool: implement common zpool api to
 zbud/zsmalloc

On Fri, 27 Jun 2014 13:11:15 -0400 Dan Streetman <ddstreet@...e.org> wrote:

> >> >> +struct zpool *zpool_create_pool(char *type, gfp_t flags,
> >> >> +                     struct zpool_ops *ops)
> >> >> +{
> >> >> +     struct zpool_driver *driver;
> >> >> +     struct zpool *zpool;
> >> >> +
> >> >> +     pr_info("creating pool type %s\n", type);
> >> >> +
> >> >> +     spin_lock(&drivers_lock);
> >> >> +     driver = zpool_get_driver(type);
> >> >> +     spin_unlock(&drivers_lock);
> >> >
> >> > Racy against unregister.  Can be solved with a standard get/put
> >> > refcounting implementation.  Or perhaps a big fat mutex.
> >
> > Was there a decision here?
> 
> What I tried to do, with the final patch in the set, was use module
> usage counting combined with function documentation - in
> zpool_create_pool() the zpool_get_driver() does try_module_get()
> before releasing the spinlock, so if the driver *only* calls
> unregister from its module exit function, I think we should be good -
> once zpool_create_pool() gets the driver module, the driver won't
> enter its exit function and thus won't unregister; and if the driver
> module has started its exit function, try_module_get() will return
> failure and zpool_create_pool() will return failure.
> 
> Now, if we remove the restriction that the driver module can only
> unregister from its module exit function, then we would need an
> additional refcount (we could use module_refcount() but the module may
> have refcounts unrelated to us) and unregister would need a return
> value, to indicate failure.  I think the problem I had with that is,
> in the driver module's exit function it can't abort if unregister
> fails; but with the module refcounting, unregister shouldn't ever fail
> in the driver's exit function...
> 
> So should I remove the unregister function doc asking to only call
> unregister from the module exit function, and add a separate refcount
> to the driver get/put functions?  I don't think we need to use a kref,
> since we don't want to free the driver once kref == 0, we want to be
> able to check in the unregister function if there are any refs, so
> just an atomic_t should work.  And we would still need to keep the
> module get/put, too, so it would be something like:

I'm not sure I understood all that.  But I don't want to understand it
in this context!  Readers should be able to gather all this from
looking at the code.

>   spin_lock(&drivers_lock);
> ...
>   bool got = try_module_get(driver->owner);
>   if (got)
>     atomic_inc(driver->refs);
>   spin_unlock(&drivers_lock);
>   return got ? driver : NULL;
> 
> with the appropriate atomic_dec in zpool_put_driver(), and unregister
> would change to:
> 
> int zpool_unregister_driver(struct zpool_driver *driver)
> {
>   spin_lock(&drivers_lock);
>   if (atomic_read(driver->refs) > 0) {
>     spin_unlock(&drivers_lock);
>     return -EBUSY;
>   }
>   list_del(&driver->list);
>   spin_unlock(&drivers_lock);
>   return 0;
> }

It sounds like that will work.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists