From owner-freebsd-current@freebsd.org Mon Aug 24 08:10:25 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 377D09BF26E for ; Mon, 24 Aug 2015 08:10:25 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from kib.kiev.ua (kib.kiev.ua [IPv6:2001:470:d5e7:1::1]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id D0EF7F6E; Mon, 24 Aug 2015 08:10:24 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from tom.home (kostik@localhost [127.0.0.1]) by kib.kiev.ua (8.15.2/8.15.2) with ESMTPS id t7O8AHgY050060 (version=TLSv1 cipher=DHE-RSA-CAMELLIA256-SHA bits=256 verify=NO); Mon, 24 Aug 2015 11:10:17 +0300 (EEST) (envelope-from kostikbel@gmail.com) DKIM-Filter: OpenDKIM Filter v2.10.3 kib.kiev.ua t7O8AHgY050060 Received: (from kostik@localhost) by tom.home (8.15.2/8.15.2/Submit) id t7O8AFfh050045; Mon, 24 Aug 2015 11:10:15 +0300 (EEST) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: tom.home: kostik set sender to kostikbel@gmail.com using -f Date: Mon, 24 Aug 2015 11:10:15 +0300 From: Konstantin Belousov To: John-Mark Gurney Cc: Andriy Gapon , FreeBSD Current , Lawrence Stewart , Pawel Pekala , "K. Macy" Subject: Re: Instant panic while trying run ports-mgmt/poudriere Message-ID: <20150824081015.GT2072@kib.kiev.ua> References: <20150715174616.652d0aea@FreeBSD.org> <20150715180526.GM8523@funkthat.com> <20150715223703.78b9197c@FreeBSD.org> <20150806233328.47a02594@FreeBSD.org> <55CB5428.2090505@room52.net> <55D96E24.9060106@FreeBSD.org> <20150823090816.GJ2072@kib.kiev.ua> <20150823125442.GK2072@kib.kiev.ua> <20150824053543.GH33167@funkthat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150824053543.GH33167@funkthat.com> User-Agent: Mutt/1.5.23 (2014-03-12) X-Spam-Status: No, score=-2.0 required=5.0 tests=ALL_TRUSTED,BAYES_00, DKIM_ADSP_CUSTOM_MED,FREEMAIL_FROM,NML_ADSP_CUSTOM_MED autolearn=no autolearn_force=no version=3.4.1 X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on tom.home 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, 24 Aug 2015 08:10:25 -0000 On Sun, Aug 23, 2015 at 10:35:44PM -0700, John-Mark Gurney wrote: > Konstantin Belousov wrote this message on Sun, Aug 23, 2015 at 15:54 +0300: > > if (kev->flags & EV_ADD) > > - tkn = knote_alloc(waitok); /* prevent waiting with locks */ > > + /* > > + * Prevent waiting with locks. Non-sleepable > > + * allocation failures are handled in the loop, only > > + * if the spare knote appears to be actually required. > > + */ > > + tkn = knote_alloc(waitok); > > if you add this comment, please add curly braces around the block... Ok. > > > else > > tkn = NULL; > > > > @@ -1310,8 +1315,7 @@ done: > > FILEDESC_XUNLOCK(td->td_proc->p_fd); > > if (fp != NULL) > > fdrop(fp, td); > > - if (tkn != NULL) > > - knote_free(tkn); > > + knote_free(tkn); > > Probably should just change knote_free to a static inline that does > a uma_zfree as uma_zfree also does nothing is the input is NULL... This was already done in the patch (the removal of the NULL check in knote_free()). I usually do not add excessive inline keywords. Compilers are good, sometimes even too good, at figuring out the possibilities for inlining. knote_free() is inlined automatically. > > @@ -1948,7 +1948,7 @@ knote(struct knlist *list, long hint, int lockflags) > > * only safe if you want to remove the current item, which we are > > * not doing. > > */ > > - SLIST_FOREACH(kn, &list->kl_list, kn_selnext) { > > + SLIST_FOREACH_SAFE(kn, &list->kl_list, kn_selnext, tkn) { > > Clearly you didn't read the comment that preceeds this line, or at > least didn't update it: > * SLIST_FOREACH, SLIST_FOREACH_SAFE is not safe in our case, it is > * only safe if you want to remove the current item, which we are > * not doing. > > So, you'll need to be more specific in why this needs to change... > When I wrote this code, I spent a lot of time looking at this, and > reasoned as to why SLIST_FOREACH_SAFE was NOT correct usage here... I explained what happens in the message. The knote list is modified by the filter, see knlist_remove_inevent() call in filt_proc(). > > > kq = kn->kn_kq; > > KQ_LOCK(kq); > > if ((kn->kn_status & (KN_INFLUX | KN_SCAN)) == KN_INFLUX) { > > @@ -2385,15 +2385,16 @@ SYSINIT(knote, SI_SUB_PSEUDO, SI_ORDER_ANY, knote_init, NULL); > > static struct knote * > > knote_alloc(int waitok) > > { > > - return ((struct knote *)uma_zalloc(knote_zone, > > - (waitok ? M_WAITOK : M_NOWAIT)|M_ZERO)); > > + > > + return (uma_zalloc(knote_zone, (waitok ? M_WAITOK : M_NOWAIT) | > > + M_ZERO)); > > } > > > > static void > > per above, we should add inline here... > > > knote_free(struct knote *kn) > > { > > - if (kn != NULL) > > - uma_zfree(knote_zone, kn); > > + > > + uma_zfree(knote_zone, kn); > > } > > > > /* > > I agree w/ the all the non-SLIST changes, but I disagree w/ the SLIST > change as I don't believe that all cases was considered... What cases do you mean ? The patch does not unlock knlist lock in the iteration. As such, the only thread which could remove elements from the knlist, or rearrange the list, while loop is active, is the current thread. So I claim that the only the current iterating element can be removed, and the next list element stays valid. This is enough for _SAFE loop to work. Why do you think that _SAFE is incorrect ? Comment talks about very different case, where the knlist lock is dropped. Then indeed, other thread may iterate in parallel, and invalidate the memoized next element while KN_INFLUX is set for the current element and knlist is dropped. But _SAFE in sys/queue.h never means 'safe for parallel mutators', it only means 'safe for the current iterator removing current element'. I preferred not to touch the comment until it is confirmed that the change help. I reformulated it now, trying to keep the note about unlock (but is it useful ?). > > Anyways, the other changes shouldn't be committed w/ the SLIST change > as they are unrelated... Sure, I posted the diff against the WIP branch. The commits will be split. diff --git a/sys/kern/kern_event.c b/sys/kern/kern_event.c index a4388aa..0e26a78 100644 --- a/sys/kern/kern_event.c +++ b/sys/kern/kern_event.c @@ -1105,10 +1105,16 @@ kqueue_register(struct kqueue *kq, struct kevent *kev, struct thread *td, int wa if (fops == NULL) return EINVAL; - if (kev->flags & EV_ADD) - tkn = knote_alloc(waitok); /* prevent waiting with locks */ - else + if (kev->flags & EV_ADD) { + /* + * Prevent waiting with locks. Non-sleepable + * allocation failures are handled in the loop, only + * if the spare knote appears to be actually required. + */ + tkn = knote_alloc(waitok); + } else { tkn = NULL; + } findkn: if (fops->f_isfd) { @@ -1310,8 +1316,7 @@ done: FILEDESC_XUNLOCK(td->td_proc->p_fd); if (fp != NULL) fdrop(fp, td); - if (tkn != NULL) - knote_free(tkn); + knote_free(tkn); if (fops != NULL) kqueue_fo_release(filt); return (error); @@ -1507,10 +1512,6 @@ kqueue_scan(struct kqueue *kq, int maxevents, struct kevent_copyops *k_ops, } else asbt = 0; marker = knote_alloc(1); - if (marker == NULL) { - error = ENOMEM; - goto done_nl; - } marker->kn_status = KN_MARKER; KQ_LOCK(kq); @@ -1929,7 +1930,7 @@ void knote(struct knlist *list, long hint, int lockflags) { struct kqueue *kq; - struct knote *kn; + struct knote *kn, *tkn; int error; if (list == NULL) @@ -1941,14 +1942,13 @@ knote(struct knlist *list, long hint, int lockflags) list->kl_lock(list->kl_lockarg); /* - * If we unlock the list lock (and set KN_INFLUX), we can eliminate - * the kqueue scheduling, but this will introduce four - * lock/unlock's for each knote to test. If we do, continue to use - * SLIST_FOREACH, SLIST_FOREACH_SAFE is not safe in our case, it is - * only safe if you want to remove the current item, which we are - * not doing. + * If we unlock the list lock (and set KN_INFLUX), we can + * eliminate the kqueue scheduling, but this will introduce + * four lock/unlock's for each knote to test. Also, marker + * would be needed to keep iteration position, since filters + * or other threads could remove events. */ - SLIST_FOREACH(kn, &list->kl_list, kn_selnext) { + SLIST_FOREACH_SAFE(kn, &list->kl_list, kn_selnext, tkn) { kq = kn->kn_kq; KQ_LOCK(kq); if ((kn->kn_status & (KN_INFLUX | KN_SCAN)) == KN_INFLUX) { @@ -2385,15 +2385,16 @@ SYSINIT(knote, SI_SUB_PSEUDO, SI_ORDER_ANY, knote_init, NULL); static struct knote * knote_alloc(int waitok) { - return ((struct knote *)uma_zalloc(knote_zone, - (waitok ? M_WAITOK : M_NOWAIT)|M_ZERO)); + + return (uma_zalloc(knote_zone, (waitok ? M_WAITOK : M_NOWAIT) | + M_ZERO)); } static void knote_free(struct knote *kn) { - if (kn != NULL) - uma_zfree(knote_zone, kn); + + uma_zfree(knote_zone, kn); } /*