On Sun, Aug 09, 2020 at 04:43:24PM +0200, Mark Kettenis wrote:
> > Date: Sat, 8 Aug 2020 19:46:14 -0500
> > From: Scott Cheloha <scottchel...@gmail.com>
> >
> > Hi,
> >
> > I want to annotate the locking for the per-process interval timers.
> >
> > In the process struct, the ITIMER_REAL itimerspec and the ps_itimer_to
> > timeout are protected by the kernel lock. These should be annotated
> > with "K", right?
> >
> > Also in the process struct, the ITIMER_VIRTUAL and ITIMER_PROF
> > itimerspecs are protected by the global itimer_mtx.
> >
> > However, I don't think "itimer_mtx" isn't the best name for it, as it
> > doesn't protect state for *all* per-process interval timers. Just the
> > virtual ones.
> >
> > Could I rename the mutex to "virtual_itimer_mtx"? Then I can annotate
> > the state protected by it with "V", as shown here in this patch.
>
> That's quite a long variable name though. And it also protects
> ITIMER_PROF. So I'd say the name would be at least as misleading as
> the current one and perhaps even more so. You can just use "I" as the
> annotation perhaps?
The convention is to use "I" for immutable variables. We do it
everywhere. I don't think we should buck convention here.
I also proposed using "i" in a prior patch to annotate these
variables, but mpi@ said it was too close to "I". Also, it's a global
lock, and we have settled on only annotate global locks with capital
letters.
If you don't want to rename the mutex I guess we could use "T" for
"timer". We use "T" for other global locks (tc_lock, timeout_mutex)
but not in this context.
However, there are only so many letters. Eventually this scheme will
run afoul of that limitation. An idea I had re. the letter shortage
was to use two letters where necessary. So instead of "I" you could
use "It" for "itimer". We annotate locking hierarchies with commas so
there isn't an ambiguity when reading it.
For example, if the code for writing a hypothetical "ps_foo" process
struct member was:
KERNEL_LOCK();
mtx_enter(&itimer_mtx);
ps.ps_foo = 10;
mtx_leave(&itimer_mtx);
KERNEL_UNLOCK();
You could annotate it like this:
/*
* Locks used to protect process struct members:
*
* It itimer_mtx
* K kernel lock
*/
struct process {
/* [...] */
int ps_foo; /* [K,It] per-process foobar */
/* [...] */
};
anton@, mpi@: is that too radical or easily misread?
Sorry if this all seems fussy, but I'd like to get this right the
first time.
-Scott
