On Sun, Aug 09, 2020 at 05:33:58PM +0200, Mark Kettenis wrote:
> > Date: Sun, 9 Aug 2020 10:02:38 -0500
> > From: Scott Cheloha <scottchel...@gmail.com>
> > 
> > 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.
> 
> 'T' is fine with me.  But I'm clearly not an authority here.  Anyway,
> renaming variables because you don't have a matching letter to
> annotate the lock doesn't feel right.

I'm also fine with T.

Reply via email to