From owner-freebsd-current@FreeBSD.ORG Thu Aug 27 00:34:12 2009 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 28F8A1065695 for ; Thu, 27 Aug 2009 00:34:12 +0000 (UTC) (envelope-from asmrookie@gmail.com) Received: from mail-fx0-f210.google.com (mail-fx0-f210.google.com [209.85.220.210]) by mx1.freebsd.org (Postfix) with ESMTP id A5CB48FC22 for ; Thu, 27 Aug 2009 00:34:11 +0000 (UTC) Received: by fxm6 with SMTP id 6so519746fxm.43 for ; Wed, 26 Aug 2009 17:34:10 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:sender:received:in-reply-to :references:date:x-google-sender-auth:message-id:subject:from:to:cc :content-type:content-transfer-encoding; bh=Binoo4qDTDnDRLVhUL7gc34nX383/rE5zD6YjXCMtaM=; b=GiwP5nL7xN6EqUzJFUzusFUsjkh7x+OCOpM2sL+PwiYMYii3dBMIPmdi6w/3lZBR6N WWpezfixz8mvktkmCMtShMJt5uPfhJbHLHTyDHGU9/UyzZZPqZkzOkIWAXNUQwpZib8v ObTBteGTz+jSbUkwB4N/70/r/M5Xu+75JhcmA= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type :content-transfer-encoding; b=i/V8tVKhAwLW8it5FnzQrwyC7ItMH2AK2xcDE3Z6EVojU9dHnIEMaSOQXaITMgOaP5 K1vle4cE7ayTC79NfSD+p22GHofR1EEGygTDKJ8PPgWcg3TG5R8t6dDhHF/XqH+d+ebF vKXah6M7TQRlnc9mmyvcE04Lh47tq91KKm/bc= MIME-Version: 1.0 Sender: asmrookie@gmail.com Received: by 10.223.97.132 with SMTP id l4mr6314445fan.100.1251333250508; Wed, 26 Aug 2009 17:34:10 -0700 (PDT) In-Reply-To: References: <3bbf2fe10908261557i286f5ec5q58eb4fec1abbfc7b@mail.gmail.com> Date: Thu, 27 Aug 2009 02:34:10 +0200 X-Google-Sender-Auth: d6934012693dda5a Message-ID: <3bbf2fe10908261734k541cc5f1kc13ffd1e581d3f13@mail.gmail.com> From: Attilio Rao To: Andrew Brampton Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Cc: freebsd-current@freebsd.org Subject: Re: [patch] rw_try_wlock does not set recursive bit when recursing 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: Thu, 27 Aug 2009 00:34:12 -0000 2009/8/27 Andrew Brampton : > 2009/8/26 Attilio Rao : >> 2009/8/27 Andrew Brampton : >>> Hi, >>> The following sequence of commands fails on line 4 with an assertion >>> that the lock is not currently held: >>> >>> 1: rw_wlock(&rw); >>> 2: if ( rw_try_wlock(&rw) ) >>> 3: rw_wunlock(&rw); >>> 4: rw_wunlock(&rw); >>> >>> This is because after line 3 is executed the rw lock is no longer >>> held. I tracked this bug down to _rw_try_wlock which correctly >>> increments rw_recurse, but does not set the RW_LOCK_RECURSED bit. >>> Without this bit the third line unlocks the lock and leaves it in a >>> unlocked state (when it should still be locked). Adding a line to set >>> this bit makes _rw_try_wlock match the code in _rw_wlock_hard. >> >> Sorry, but I really don't understand how that can be a bug. >> On STABLE_7, RW_LOCK_RECURSED is not used for checking if the lock is >> recursed or not. >> it just got set for improving debugging and eventually we decided to >> drop it for 8.0. >> >> However, for deciding if a lock is recursed or not in both STABLE_7 >> and HEAD we used just checking against the recursion count which is >> correctly handled by the function. >> >> What you describe can't be the problem. >> >> Attilio >> > > Ok, so I have had a better look at the code in CURRENT, and compared > it to the code in STABLE_7. I apologise but I think I mixed up my > sources somewhere and the problem does not appear in CURRENT. The > problem does however occur in STABLE_7, and I can explain that below. > > 1: rw_wlock(&rw); > 2: if ( rw_try_wlock(&rw) ) > 3: rw_wunlock(&rw); > 4: rw_wunlock(&rw); > > Line 1, _rw_wlock gets called which calls __rw_wlock. This in turn > calls _rw_write_lock which changes rw->rw_lock to tid > Line 2, _rw_try_wlock gets called, which check if the lock is already > held (which it is), if so it then rw->rw_recurse++ > Line 3, _rw_wunlock gets called which calls __rw_wunlock, which then > calls _rw_write_unlock. Now _rw_write_unlock trys to unlock the lock > by checking if rw->rw_lock is tid, if so it sets the lock to > RW_UNLOCKED. This is where the problem occurs, there is no check on > rx->rw_recurse. > > Now, if we used this code instead: > 1: rw_wlock(&rw); > 2: if ( rw_try_wlock(&rw) ) > 3: rw_wunlock(&rw); > 4: rw_wunlock(&rw); > > The order goes: > Line 1, _rw_wlock gets called which calls __rw_wlock. This in turn > calls _rw_write_lock which changes rw->rw_lock to tid > Line 2, _rw_wlock gets called which calls __rw_wlock. __rw_wlock > checks if rw->rw_lock is RW_UNLOCKED, otherwise it ends up calling > _rw_wlock_hard. Inside _rw_wlock_hard it checks if the lock is already > held, if so it increments rw_recurse, and changes rw->rw_lock to > (rw->rw_lock | RW_LOCK_RECURSED) by calling atomic_set_ptr. > Line 3, _rw_wunlock gets called which calls __rw_wunlock, which then > calls _rw_write_unlock. Now _rw_write_unlock trys to unlock the lock > by checking if rw->rw_lock is tid. This is where things differ from > the previous code, rw->rw_lock is NOT tid, it is now (tid | > RW_LOCK_RECURSED), and thus the code drops into _rw_wunlock_hard which > decrements rx->rw_recurse and removes the RW_LOCK_RECURSED flag if > rw_recurse equals zero. > > Hopefully you see how not setting the RW_LOCK_RECURSED flag causes a > problem in rw_wunlock. So this can be fixed by either back porting the > CURRENT code to STABLE_7, or using the one line fix in my patch. As I > already stated, I am using my patch and the sample code I have given > no longer panics. I see the problem now. I think the simple patch of your is good and I will commit it. Thanks, Attilio -- Peace can only be achieved by understanding - A. Einstein