Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754196AbaF0TRz (ORCPT ); Fri, 27 Jun 2014 15:17:55 -0400 Received: from mail.linuxfoundation.org ([140.211.169.12]:43856 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752561AbaF0TRy (ORCPT ); Fri, 27 Jun 2014 15:17:54 -0400 Date: Fri, 27 Jun 2014 12:17:52 -0700 From: Andrew Morton To: Dan Streetman Cc: Seth Jennings , Minchan Kim , Weijie Yang , Nitin Gupta , Bob Liu , Hugh Dickins , Mel Gorman , Rik van Riel , Johannes Weiner , Sergey Senozhatsky , Linux-MM , linux-kernel Subject: Re: [PATCHv4 3/6] mm/zpool: implement common zpool api to zbud/zsmalloc Message-Id: <20140627121752.21c559c3404d665adbaa5b23@linux-foundation.org> In-Reply-To: References: <1400958369-3588-1-git-send-email-ddstreet@ieee.org> <1401747586-11861-1-git-send-email-ddstreet@ieee.org> <1401747586-11861-4-git-send-email-ddstreet@ieee.org> <20140623144614.d4549fa0aecb03b7b8044bc7@linux-foundation.org> <20140624160857.ebb638c4c69c1d290f64d01f@linux-foundation.org> X-Mailer: Sylpheed 3.2.0beta5 (GTK+ 2.24.10; x86_64-pc-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, 27 Jun 2014 13:11:15 -0400 Dan Streetman 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/