Date: Thu, 25 Aug 2011 14:34:44 +0300 From: Andriy Gapon <avg@FreeBSD.org> To: Hans Petter Selasky <hselasky@c2i.net>, John Baldwin <jhb@FreeBSD.org> Cc: freebsd-arch@FreeBSD.org Subject: Re: skipping locks, mutex_owned, usb Message-ID: <4E563354.5020106@FreeBSD.org> In-Reply-To: <201108251235.15853.hselasky@c2i.net> References: <4E53986B.5000804@FreeBSD.org> <201108230911.09021.jhb@freebsd.org> <201108251235.15853.hselasky@c2i.net>
next in thread | previous in thread | raw e-mail | index | archive | help
on 25/08/2011 13:35 Hans Petter Selasky said the following:
> On Tuesday 23 August 2011 15:11:08 John Baldwin wrote:
>>> I. [Why] do we need this pattern?
>>> Can the code be re-written in a smarter (if not to say proper) way?
>>
>
> Hi,
>
>> Giant is recursive, it should just be always acquired. Also, this
>> recursive call pattern is very non-obvious. A far more straightforward
>> approach would be to just have:
>
> Unless Witness is changed, that won't work. It will lead to LOR warnings I
> think.
>
> Imagine that the root function locks Giant, then another lock is locked and
> then ukbd_poll() is called. Won't the second lock of Giant cause a LOR
> warning?
I think no.
At least that's how I interpret the following snippet in witness_checkorder():
/*
* Check to see if we are recursing on a lock we already own. If
* so, make sure that we don't mismatch exclusive and shared lock
* acquires.
*/
lock1 = find_instance(lock_list, lock);
if (lock1 != NULL) {
if ((lock1->li_flags & LI_EXCLUSIVE) != 0 &&
(flags & LOP_EXCLUSIVE) == 0) {
printf("shared lock of (%s) %s @ %s:%d\n",
class->lc_name, lock->lo_name, file, line);
printf("while exclusively locked from %s:%d\n",
lock1->li_file, lock1->li_line);
panic("share->excl");
}
if ((lock1->li_flags & LI_EXCLUSIVE) == 0 &&
(flags & LOP_EXCLUSIVE) != 0) {
printf("exclusive lock of (%s) %s @ %s:%d\n",
class->lc_name, lock->lo_name, file, line);
printf("while share locked from %s:%d\n",
lock1->li_file, lock1->li_line);
panic("excl->share");
}
return;
}
Because of the return statement we do not seem to be doing any additional order
checking in the case of recursion.
>> static int
>> ukbd_poll(keyboard_t *kbd, int on)
>> {
>> struct ukbd_softc *sc = kbd->kb_data;
>>
>> mtx_lock(&Giant);
>> if (on) {
>> sc->sc_flags |= UKBD_FLAG_POLLING;
>> sc->sc_poll_thread = curthread;
>> } else {
>> sc->sc_flags &= ~UKBD_FLAG_POLLING;
>> ukbd_start_timer(sc); /* start timer */
>> }
>> mtx_unlock(&Giant);
>> return (0);
>> }
>>
>
>> Most code should not be abusing mtx_owned() in this fashion. For Giant you
>> should just follow a simple pattern like above that lets it recurse. For
>> your own locks you generally should use things like mtx_assert() to
>> require all callers of a given routine to hold the lock rather than
>> recursively acquiring the lock. Very few legitimate cases of mtx_owned()
>> exist IMO. It is debatable if we should even have a mtx_owned() routine
>> since we have mtx_assert().
>
> How would you solve the LOR case?
>
> --HPS
--
Andriy Gapon
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4E563354.5020106>