Date: Thu, 19 Sep 2013 13:49:55 -0400 From: John Baldwin <jhb@freebsd.org> To: Davide Italiano <davide@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r254703 - in head: share/man/man9 sys/sys Message-ID: <201309191349.55574.jhb@freebsd.org> In-Reply-To: <CACYV=-EE0RiaFYDk56910rye-fZV6NStYA4nAZMSEcJ-bk96yg@mail.gmail.com> References: <201308231412.r7NECdG7081565@svn.freebsd.org> <201309121008.01115.jhb@freebsd.org> <CACYV=-EE0RiaFYDk56910rye-fZV6NStYA4nAZMSEcJ-bk96yg@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wednesday, September 18, 2013 11:06:33 am Davide Italiano wrote: > On Thu, Sep 12, 2013 at 4:08 PM, John Baldwin <jhb@freebsd.org> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201309191349.55574.jhb>