Date: Thu, 25 Aug 2011 16:33:09 +0300 From: Andriy Gapon <avg@FreeBSD.org> To: John Baldwin <jhb@FreeBSD.org> Cc: freebsd-arch@FreeBSD.org Subject: Re: skipping locks, mutex_owned, usb Message-ID: <4E564F15.3010809@FreeBSD.org> In-Reply-To: <201108230911.09021.jhb@freebsd.org> References: <4E53986B.5000804@FreeBSD.org> <201108230911.09021.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
on 23/08/2011 16:11 John Baldwin said the following:
> On Tuesday, August 23, 2011 8:09:15 am Andriy Gapon wrote:
>>
>> Yes, the subject sounds quite hairy, so please let me try to explain it.
>> First, let's consider one concrete function:
>>
>> static int
>> ukbd_poll(keyboard_t *kbd, int on)
>> {
>> struct ukbd_softc *sc = kbd->kb_data;
>>
>> if (!mtx_owned(&Giant)) {
>> /* XXX cludge */
>> int retval;
>> mtx_lock(&Giant);
>> retval = ukbd_poll(kbd, on);
>> mtx_unlock(&Giant);
>> return (retval);
>> }
>>
>> 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 */
>> }
>> return (0);
>> }
>>
>> This "XXX cludge" [sic] pattern is scattered around a few functions in the ukbd
>> code and perhaps other usb code:
>> func()
>> {
>> if (!mtx_owned(&Giant)) {
>> mtx_lock(&Giant);
>> func();
>> mtx_unlock(&Giant);
>> return;
>> }
>>
>> // etc ...
>> }
>>
>> I have a few question directly and indirectly related to this pattern.
>>
>> I. [Why] do we need this pattern?
>> Can the code be re-written in a smarter (if not to say proper) way?
>
> 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:
>
> 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);
> }
>
Thank you for clarifying this, I agree this is simpler than the original code and
should work exactly the same.
There is more trouble with a few other functions that actually behave differently
(even if slightly) depending on what mtx_owned(&Giant) returns. Not sure if
that's a legal use or an antipattern.
For example: ukbd_ioctl, ukbd_check, ukbd_check_char, ukbd_read, ukbd_read_char.
--
Andriy Gapon
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4E564F15.3010809>