From owner-svn-src-head@FreeBSD.ORG Thu Sep 19 18:17:07 2013 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 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTP id C2CA3B23; Thu, 19 Sep 2013 18:17:07 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from bigwig.baldwin.cx (bigwig.baldwin.cx [IPv6:2001:470:1f11:75::1]) (using TLSv1 with cipher ADH-CAMELLIA256-SHA (256/256 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id 970852F1D; Thu, 19 Sep 2013 18:17:07 +0000 (UTC) Received: from jhbbsd.localnet (unknown [209.249.190.124]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 9BF57B98A; Thu, 19 Sep 2013 14:17:06 -0400 (EDT) From: John Baldwin To: Davide Italiano Subject: Re: svn commit: r254703 - in head: share/man/man9 sys/sys Date: Thu, 19 Sep 2013 13:49:55 -0400 User-Agent: KMail/1.13.5 (FreeBSD/8.4-CBSD-20130906; KDE/4.5.5; amd64; ; ) References: <201308231412.r7NECdG7081565@svn.freebsd.org> <201309121008.01115.jhb@freebsd.org> In-Reply-To: MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201309191349.55574.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Thu, 19 Sep 2013 14:17:06 -0400 (EDT) Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.14 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: Thu, 19 Sep 2013 18:17:07 -0000 On Wednesday, September 18, 2013 11:06:33 am Davide Italiano wrote: > On Thu, Sep 12, 2013 at 4:08 PM, John Baldwin wrote: > > Hmm, I think I had envisioned something a bit simpler. Namely, I would > > change lc_lock/lc_unlock to return a uintptr_t instead of an int, and > > I would then change lc_unlock for an rm lock to return a pointer to the > > current thread's tracker as the 'how' and 0 for a write lock. Note > > that you have to use the existing tracker to make this work correctly > > for the sleep case where you unlock/lock. > > > > Well, your solution is indeed a lot simpler :) > Here's a patch that implements it: > http://people.freebsd.org/~davide/review/rmshared.diff > > Some (more or less relevant) observations: > -> I realized that before this change lc_unlock() for rmlocks, i.e. > unlock_rm(), returned 1 for exclusive lock and panic'ed otherwise, > while all the other primitives returned 0 for exclusive lock. Not sure > if this was intentional or just an oversight, but I changed it to > return what other primitive did mostly (0 for excl, > (uintptr_t)rm_tracker for shared). That's fine. > -> In order to get the rm_priotracker structure for curthread (which I > think is unique as long as we assert curthread is not recursively > acquiring this lock) I just traversed the pc->pc_rm_queue. Maybe it's > not the most efficient solution here, but I think is correct. Yes, it should be similar to rm_trackers_present(). > -> I think that only lc_unlock() return type need to be changed from > 'int' to 'uintptr_t'. lc_lock() type can stay void as it is right now. > But I think you were talking about "how" argument of lc_lock() and not > return type, probably. Yes, I meant the 'how' arg to lc_lock(). The patch looks good, just one minor nit: - 'rval' in _cv_wait_sig() in kern_condvar.c should stay an 'int' > > However, if you make my suggested change to make the 'how' a uintptr_t > > that passes the tracker you can actually do this in the callout case: > > > > struct rm_priotracker tracker; > > uintptr_t how; > > > > how = 0; > > if (flags & CALLOUT_SHAREDLOCK) > > how = 1; > > else if (flags & CALLOUT_SHAREDRM) > > how = (uintptr_t)&tracker; > > ... > > > > class->lc_lock(lock, how); > > > > Now, it would be even nicer to make prevent footshooting perhaps by > > checking the lock class directly: > > > > how = 0; > > if (flags & CALLOUT_SHAREDLOCK) { > > if (class == &lock_class_rm || class == &lock_class_rm_sleepable) > > how = (uintptr_t)&tracker; > > else > > how = 1; > > } > > > > -- > > John Baldwin > > This other patch just puts your code into kern_timeout.c > I also removed the check for lock_class_rm_sleepable as callout_init() > should catch this earlier. > http://people.freebsd.org/~davide/review/callout_sharedrm.diff This looks good. My only suggestion would be to rename 'sharedlock' to 'how' or maybe 'lock_state'. -- John Baldwin