svn commit: r254703 - in head: share/man/man9 sys/sys

[John Baldwin]

On Wednesday, September 18, 2013 11:06:33 am Davide Italiano wrote:
> On Thu, Sep 12, 2013 at 4:08 PM, John Baldwin <jhb at 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