Date: Wed, 18 Sep 2013 17:06:33 +0200 From: Davide Italiano <davide@freebsd.org> To: John Baldwin <jhb@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: <CACYV=-EE0RiaFYDk56910rye-fZV6NStYA4nAZMSEcJ-bk96yg@mail.gmail.com> In-Reply-To: <201309121008.01115.jhb@freebsd.org> References: <201308231412.r7NECdG7081565@svn.freebsd.org> <201308261502.13277.jhb@freebsd.org> <CACYV=-FiOyVsjbnPA5qLXK2OvN_Y9AgWpqOyYzEuJ7a2o8dp1g@mail.gmail.com> <201309121008.01115.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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). -> 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. -> 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. > 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 Thanks for the guidance on this. I probably commit this in the next days if you don't have objections. -- Davide "There are no solved problems; there are only problems that are more or less solved" -- Henri Poincare
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CACYV=-EE0RiaFYDk56910rye-fZV6NStYA4nAZMSEcJ-bk96yg>