On Thu, Sep 12, 2013 at 4:08 PM, John Baldwin <j...@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 _______________________________________________ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
