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

[Davide Italiano]

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).
-> 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