From owner-freebsd-current@freebsd.org Mon Jul 13 18:48:14 2015 Return-Path: Delivered-To: freebsd-current@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 7D1E499C222 for ; Mon, 13 Jul 2015 18:48:14 +0000 (UTC) (envelope-from luigi@onelab2.iet.unipi.it) Received: from mailman.ysv.freebsd.org (mailman.ysv.freebsd.org [IPv6:2001:1900:2254:206a::50:5]) by mx1.freebsd.org (Postfix) with ESMTP id 68D6D1228 for ; Mon, 13 Jul 2015 18:48:14 +0000 (UTC) (envelope-from luigi@onelab2.iet.unipi.it) Received: by mailman.ysv.freebsd.org (Postfix) id 65BC099C221; Mon, 13 Jul 2015 18:48:14 +0000 (UTC) Delivered-To: current@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 654A399C220 for ; Mon, 13 Jul 2015 18:48:14 +0000 (UTC) (envelope-from luigi@onelab2.iet.unipi.it) Received: from onelab2.iet.unipi.it (onelab2.iet.unipi.it [131.114.59.238]) by mx1.freebsd.org (Postfix) with ESMTP id 2DC931227; Mon, 13 Jul 2015 18:48:13 +0000 (UTC) (envelope-from luigi@onelab2.iet.unipi.it) Received: by onelab2.iet.unipi.it (Postfix, from userid 275) id 912D37300A; Mon, 13 Jul 2015 20:56:36 +0200 (CEST) Date: Mon, 13 Jul 2015 20:56:36 +0200 From: Luigi Rizzo To: Konstantin Belousov , current@freebsd.org Cc: adrian@freebsd.org Subject: Re: protection against module unloading ? Message-ID: <20150713185636.GA28828@onelab2.iet.unipi.it> References: <20150713124603.GH2404@kib.kiev.ua> <20150713150030.GA25208@onelab2.iet.unipi.it> <20150713152912.GL2404@kib.kiev.ua> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150713152912.GL2404@kib.kiev.ua> User-Agent: Mutt/1.5.20 (2009-06-14) X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 13 Jul 2015 18:48:14 -0000 On Mon, Jul 13, 2015 at 06:29:12PM +0300, Konstantin Belousov wrote: > On Mon, Jul 13, 2015 at 05:00:30PM +0200, Luigi Rizzo wrote: ... > > thanks a lot for the clarification on the intent. > > I clearly need to understand more on the architecture of the module unload. > > > > In any case: the global contention on devmtx for I/O syscalls is > > really a showstopper for making effective use of modular drivers > > so we should really find a way to remove it. > What contention do you see ? Is it on the device node for a single > device ? If yes, then any modification of the below proposal would > not help. I explained this below. It was adrian that pointed it out to me the huge devmtx contention with multiple threads doing I/O on netmap file descriptor (4-8 threads each of them issuing around 200K syscalls/s) Now i see how even if my idea of per-dev lock was correct it would not remove contention at all. One final thing: > > Is there any other way to protect access to dev->si_threadcount ? > > > > Eg how about the following: > > - use a (leaf) lock into struct cdev to protect dev->si_threadcount, so that > > we could replace dev_lock() with mtx_lock(&dev->foo) in dev_refthread(dev) > > dev_relthread(dev) and other places that access si_threadcount > This would not work, you cannot protect a lifetime of the object by a lock > contained in the object. i thought so but then the current dev_refthread() is already unsafe, accessing dev->si_flags unprotected sys/kern/kern_conf.c: struct cdevsw * dev_refthread(struct cdev *dev, int *ref) { struct cdevsw *csw; struct cdev_priv *cdp; mtx_assert(&devmtx, MA_NOTOWNED); if ((dev->si_flags & SI_ETERNAL) != 0) { *ref = 0; return (dev->si_devsw); } dev_lock(); csw = dev->si_devsw; if (csw != NULL) { cdp = cdev2priv(dev); if ((cdp->cdp_flags & CDP_SCHED_DTR) == 0) atomic_add_long(&dev->si_threadcount, 1); else csw = NULL; } dev_unlock(); *ref = 1; return (csw); } that is particularly bad though, because it prevents from checking the SI_ETERNAL flag without holding the lock (short of encoding the flag in the pointer!) cheers luigi