From owner-svn-src-head@FreeBSD.ORG Fri Jun 5 16:54:23 2015 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 70BAEFB8; Fri, 5 Jun 2015 16:54:23 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: from mail-wi0-x233.google.com (mail-wi0-x233.google.com [IPv6:2a00:1450:400c:c05::233]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 193B11294; Fri, 5 Jun 2015 16:54:23 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: by wibut5 with SMTP id ut5so27105851wib.1; Fri, 05 Jun 2015 09:54:21 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=yyvSAky6sWyXI8u2rDljowf8b2qZyHGen9vwIIX1AF0=; b=T8hfcbRISy3Ict3rxnKFVXAeLlHrymrIcFCy76Ry1lJjU06oBWLNbIp2PnCXcNneAC GKuhoEC3tca1X84Hky0FpEIukq/FrAXQt2IIiy7Ec/EnU+qZdk6NJ3Ln078g2LcNXntD exrh58oWhB4rXt++leoLssiRGBU9PjA/QOKhhicr+nCKcWbja/H6gjWeNoqgV7gTOymf PsHBpLaWUSYIIGFPvg27JypmTEbAaN2w0/YLp8oYa5bZpz5ihyDKxxCkGVv/9F6UZt63 KsQf0y1+VTbyMXMIMusutDa89UOCdBQifKaR1xkhIf0IvJYF9GxV83UZO6LoMkxW2o6o +pCQ== X-Received: by 10.180.105.227 with SMTP id gp3mr19956587wib.56.1433523261452; Fri, 05 Jun 2015 09:54:21 -0700 (PDT) Received: from dft-labs.eu (n1x0n-1-pt.tunnel.tserv5.lon1.ipv6.he.net. [2001:470:1f08:1f7::2]) by mx.google.com with ESMTPSA id r6sm4226612wiy.13.2015.06.05.09.54.19 (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Fri, 05 Jun 2015 09:54:20 -0700 (PDT) Date: Fri, 5 Jun 2015 18:54:18 +0200 From: Mateusz Guzik To: Sean Bruno Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r284029 - head/sys/kern Message-ID: <20150605165417.GA13205@dft-labs.eu> References: <201506051621.t55GLiKp095005@svn.freebsd.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <201506051621.t55GLiKp095005@svn.freebsd.org> User-Agent: Mutt/1.5.21 (2010-09-15) X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 05 Jun 2015 16:54:23 -0000 On Fri, Jun 05, 2015 at 04:21:44PM +0000, Sean Bruno wrote: > Author: sbruno > Date: Fri Jun 5 16:21:43 2015 > New Revision: 284029 > URL: https://svnweb.freebsd.org/changeset/base/284029 > > Log: > This change uses a reader count instead of holding the mutex for the > interpreter list to avoid the problem of holding a non-sleep lock during > a page fault as reported by witness. In addition, it consistently uses > memset()/memcpy() instead of bzero()/bcopy() except in the case where > bcopy() is required (i.e. overlapping copy). What lock was that? If it has to be held, why not use an sx lock? The change looks quite buggy. > > Differential Revision: https://reviews.freebsd.org/D2123 > Submitted by: sson > MFC after: 2 weeks > Relnotes: Yes > > Modified: > head/sys/kern/imgact_binmisc.c > > Modified: head/sys/kern/imgact_binmisc.c > ============================================================================== > --- head/sys/kern/imgact_binmisc.c Fri Jun 5 16:02:24 2015 (r284028) > +++ head/sys/kern/imgact_binmisc.c Fri Jun 5 16:21:43 2015 (r284029) > @@ -1,5 +1,5 @@ > /*- > - * Copyright (c) 2013, Stacey D. Son > + * Copyright (c) 2013-15, Stacey D. Son > * All rights reserved. > * > * Redistribution and use in source and binary forms, with or without > @@ -41,6 +41,9 @@ __FBSDID("$FreeBSD$"); > #include > #include > #include > +#include > + > +#include > > /** > * Miscellaneous binary interpreter image activator. > @@ -95,11 +98,68 @@ static SLIST_HEAD(, imgact_binmisc_entry > SLIST_HEAD_INITIALIZER(interpreter_list); > > static int interp_list_entry_count = 0; > - > +static int interp_list_rcount = 0; > static struct mtx interp_list_mtx; > > int imgact_binmisc_exec(struct image_params *imgp); > > +static inline void > +interp_list_rlock() > +{ > + > + /* Don't use atomic_add() here. We must block if the mutex is held. */ > + mtx_lock(&interp_list_mtx); > + interp_list_rcount++; > + mtx_unlock(&interp_list_mtx); > +} > + > +static inline void > +interp_list_runlock() > +{ > + > + /* Decrement the reader count and wake up one writer, if any. */ > + atomic_subtract_int(&interp_list_rcount, 1); > + if (interp_list_rcount == 0) > + wakeup_one(&interp_list_rcount); > +} > + interp_list_rcount manipulation between these 2 is not safe. This effectively is: A B interp_list_rcount++ atomic_subtract_int(&interp_list_rcount, 1); As such updates can be lost. There is also potential for bonus wakeup. > +static inline void > +interp_list_wlock() > +{ > + > + /* Wait until there are no readers. */ > + mtx_lock(&interp_list_mtx); > + while (interp_list_rcount) > + mtx_sleep(&interp_list_rcount, &interp_list_mtx, 0, "IABM", 0); > +} > + Given that runlock does not take the lock, this func can sleep after wakeup_one() thus blocking the thread with nobody to wake it up. -- Mateusz Guzik