From owner-freebsd-current@FreeBSD.ORG Fri Jan 27 16:34:42 2012 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 90B8A106564A for ; Fri, 27 Jan 2012 16:34:42 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from cyrus.watson.org (cyrus.watson.org [65.122.17.42]) by mx1.freebsd.org (Postfix) with ESMTP id 63FBC8FC13 for ; Fri, 27 Jan 2012 16:34:42 +0000 (UTC) Received: from bigwig.baldwin.cx (bigwig.baldwin.cx [96.47.65.170]) by cyrus.watson.org (Postfix) with ESMTPSA id 01BA246B37; Fri, 27 Jan 2012 11:34:42 -0500 (EST) Received: from jhbbsd.localnet (unknown [209.249.190.124]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 86F1EB96F; Fri, 27 Jan 2012 11:34:41 -0500 (EST) From: John Baldwin To: freebsd-current@freebsd.org Date: Fri, 27 Jan 2012 10:13:55 -0500 User-Agent: KMail/1.13.5 (FreeBSD/8.2-CBSD-20110714-p10; KDE/4.5.5; amd64; ; ) References: <201201262103.q0QL3QWr083496@ambrisko.com> <20120127085656.GY2726@deviant.kiev.zoral.com.ua> In-Reply-To: <20120127085656.GY2726@deviant.kiev.zoral.com.ua> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-15" Content-Transfer-Encoding: 7bit Message-Id: <201201271013.55474.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Fri, 27 Jan 2012 11:34:41 -0500 (EST) Cc: Kostik Belousov Subject: Re: knlist_empty locking fix X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 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: Fri, 27 Jan 2012 16:34:42 -0000 On Friday, January 27, 2012 3:56:56 am Kostik Belousov wrote: > On Thu, Jan 26, 2012 at 01:03:26PM -0800, Doug Ambrisko wrote: > > Ran into problems with running kqueue/aio with WITNESS etc. Sometimes > > things are locked sometimes not. knlist_remove is called telling it > > whether it is locked or not ie: > > extern void knlist_remove(struct knlist *knl, struct knote *kn, int islocked); > > so I changed: > > extern int knlist_empty(struct knlist *knl); > > to: > > extern int knlist_empty(struct knlist *knl, int islocked); > > > > and then updated things to reflect that following what that state of the > > lock for knlist_remove. If it is not locked, it gets a lock and > > frees it after. > > > > This now fixes a panic when a process using kqueue/aio is killed on > > shutdown with WITNESS. > > > > It changes an API/ABI so it probably can't merged back. If there are > > no objections then I'll commit it. > > > Change to knlist_init() does not make sense at all, the knlist shall > not be exposed to other consumers during initialization, so no need > to exclude the parallel access. > > Regarding the knlist_empty(), I propose to keep it as is. Locking > the knlist inside knlist_empty() does not make sense, because lock > is immediately dropped afterward, and relocked for remove. This way, > the entry could be removed from the list meantime (can it, really ?). > > I think that you should take a lock around the whole if() {} statement, > and call knlist_remove with locked == 1. Agreed, I think the missing locking should just be added to the aio code. -- John Baldwin