Date: Fri, 26 Aug 2011 21:28:40 +0300 From: Andriy Gapon <avg@FreeBSD.org> To: freebsd-arch@FreeBSD.org Cc: Hans Petter Selasky <hselasky@FreeBSD.org>, John Baldwin <jhb@FreeBSD.org> Subject: Re: skipping locks, mutex_owned, usb Message-ID: <4E57E5D8.3010606@FreeBSD.org> In-Reply-To: <201108250945.24606.jhb@freebsd.org> References: <4E53986B.5000804@FreeBSD.org> <201108230911.09021.jhb@freebsd.org> <4E564F15.3010809@FreeBSD.org> <201108250945.24606.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
I looked a little bit through the code trying to get some facts about the
locking in tty/syscons/kbd area (with respect to input, obviously). I would
like to ask you to please double-check my observations and also to answer some
questions that follow from the observations, if possible.
So, first of all, I see almost no explicit Giant locking in the code related to
tty/syscons/kbd subsystems/drivers/layers. By kbd I mean kbd, kbdmux, vkbd and
atkbdc, but not ukbd, which I consider as a special case. The code seems
to get the locking via the following means:
- kbd/syscons cdevs are marked as needing the Giant
- Giant-protected variant of callouts is used
- wherever taskqueues are used they are used with the Giant
- console vtys are allocated using tty_alloc_mutex(Giant)
The only case where the Giant is acquired explicitly is sckbdevent() in the
syscons driver.
Also, and maybe this is beyond the topic at hand, I do not see any other locking
that would protect internal states of syscons and atkbd* drivers. Of course,
they are littered with spl* calls, but those are NOPs now.
Another observation is that when the kernel code calls into syscons code
(kern_cons.c) it doesn't acquire the Giant. It seems that the kernel calls
syscons input routines only in some very special situations like very early boot
(pause after each line option), late shutdown stage ("press any key to...") and
ddb command prompt.
Based on the above I make some further conclusions.
The Giant is held when upper levels call into syscons and kbd drivers and the
calls are originating from userland.
The Giant is held when the kbd drivers call into syscons via sckbdevent (as
kb_callback).
Thus, I think that the internal state of syscons is protected by the Giant alone.
The only case where it is not protected and where the protection would probably
be unneeded or even harmful is when the kernel calls into the syscons in the
special situations.
I do not see how internal state of kbd drivers is protected from races between
interrupts and calls from upper layers. The only possibility that I see is that
kbd interrupt handlers do not directly change the driver state but rather "call
back" into the driver via sckbdevent().
In this case the state is always protected by the Giant in normal situations.
And it should not need to be protected in the special situations, because in
those there should be neither concurrent access nor interrupts.
Again, I think that when the kernel calls the syscons inputs routines, sc_cngetc
actually, then the syscons puts a kbd driver into polling mode.
So further calls should be safe from races with a kbd interrupt handler.
Hmm, but it's actually the kbd poll(TRUE) call itself that can be racy-ish...
As a sidenote, I think that ukbd is not used during early boot, because we do
not have any special code for early USB keyboard discovery and typically ukbd
attaches quite late during boot (sometimes even too late, at least in the past).
And now, provided that no other kbd driver explicitly obtains the Giant and thus
depends on upper levels doing the right thing - either locking the Giant or
knowing that it needs not to be locked, I have to ask why ukbd needs to
explicitly take the Giant.
Does ukbd interact with other layers/subsystems substantially differently from
other kbd drivers?
Does ukbd needs the Giant because of the rest of the USB subsystem?
Does it need the Giant inherently because of how it accesses its internal state?
Thank you very much in advance!
P.S. I will greatly appreciate if each of my statements about the FreeBSD code
could be annotated with the correct/incorrect comment. I do realize how much
effort that is.
--
Andriy Gapon
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4E57E5D8.3010606>