Re: process: annotate locking for setitimer(2) state
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
> >
> > 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
> > > >
> > > > 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(_mtx);
> > ps.ps_foo = 10;
> > mtx_leave(_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.
Re: process: annotate locking for setitimer(2) state
> Date: Sun, 9 Aug 2020 10:02:38 -0500
> From: Scott Cheloha
>
> 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
> > >
> > > 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(_mtx);
> ps.ps_foo = 10;
> mtx_leave(_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.
Re: process: annotate locking for setitimer(2) state
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
> >
> > 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(_mtx);
ps.ps_foo = 10;
mtx_leave(_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
Re: process: annotate locking for setitimer(2) state
> Date: Sat, 8 Aug 2020 19:46:14 -0500
> From: Scott Cheloha
>
> 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?
> Preferences? ok?
>
> Index: kern/kern_time.c
> ===
> RCS file: /cvs/src/sys/kern/kern_time.c,v
> retrieving revision 1.134
> diff -u -p -r1.134 kern_time.c
> --- kern/kern_time.c 8 Aug 2020 01:01:26 - 1.134
> +++ kern/kern_time.c 9 Aug 2020 00:41:10 -
> @@ -488,7 +488,13 @@ out:
> }
>
>
> -struct mutex itimer_mtx = MUTEX_INITIALIZER(IPL_CLOCK);
> +/*
> + * Global virtual interval timer mutex.
> + *
> + * Protects state for the per-process ITIMER_VIRTUAL and ITIMER_PROF
> + * interval timers.
> + */
> +struct mutex virtual_itimer_mtx = MUTEX_INITIALIZER(IPL_CLOCK);
>
> /*
> * Get value of an interval timer. The process virtual and
> @@ -526,10 +532,10 @@ sys_getitimer(struct proc *p, void *v, r
> return (EINVAL);
> itimer = >p_p->ps_timer[which];
> memset(, 0, sizeof(aitv));
> - mtx_enter(_mtx);
> + mtx_enter(_itimer_mtx);
> TIMESPEC_TO_TIMEVAL(_interval, >it_interval);
> TIMESPEC_TO_TIMEVAL(_value, >it_value);
> - mtx_leave(_mtx);
> + mtx_leave(_itimer_mtx);
>
> if (which == ITIMER_REAL) {
> struct timeval now;
> @@ -604,9 +610,9 @@ sys_setitimer(struct proc *p, void *v, r
> }
> pr->ps_timer[ITIMER_REAL] = aits;
> } else {
> - mtx_enter(_mtx);
> + mtx_enter(_itimer_mtx);
> pr->ps_timer[which] = aits;
> - mtx_leave(_mtx);
> + mtx_leave(_itimer_mtx);
> }
>
> return (0);
> @@ -681,20 +687,20 @@ itimerdecr(struct itimerspec *itp, long
>
> NSEC_TO_TIMESPEC(nsec, );
>
> - mtx_enter(_mtx);
> + mtx_enter(_itimer_mtx);
> timespecsub(>it_value, , >it_value);
> if (itp->it_value.tv_sec >= 0 && timespecisset(>it_value)) {
> - mtx_leave(_mtx);
> + mtx_leave(_itimer_mtx);
> return (1);
> }
> if (!timespecisset(>it_interval)) {
> timespecclear(>it_value);
> - mtx_leave(_mtx);
> + mtx_leave(_itimer_mtx);
> return (0);
> }
> while (itp->it_value.tv_sec < 0 || !timespecisset(>it_value))
> timespecadd(>it_value, >it_interval, >it_value);
> - mtx_leave(_mtx);
> + mtx_leave(_itimer_mtx);
> return (0);
> }
>
> Index: sys/proc.h
> ===
> RCS file: /cvs/src/sys/sys/proc.h,v
> retrieving revision 1.297
> diff -u -p -r1.297 proc.h
> --- sys/proc.h6 Jul 2020 13:33:09 - 1.297
> +++ sys/proc.h9 Aug 2020 00:41:11 -
> @@ -150,9 +150,11 @@ struct unveil;
> /*
> * Locks used to protect struct members in this file:
> * a atomic operations
> + * K kernel lock
> * m this process' `ps_mtx'
> * p this process' `ps_lock'
> * R rlimit_lock
> + * V virtual_itimer_mtx
> */
> struct process {
> /*
> @@ -216,7 +218,8 @@ struct process {
> struct rusage *ps_ru; /* sum of stats for dead threads. */
> struct tusage ps_tu; /* accumulated times. */
> struct rusage ps_cru; /* sum of stats for reaped children */
> - struct itimerspec ps_timer[3]; /* timers, indexed by ITIMER_* */
> + struct itimerspec ps_timer[3]; /* [K] ITIMER_REAL timer */
> + /* [V] ITIMER_{PROF,VIRTUAL} timers */
> struct timeout ps_rucheck_to; /* [] resource limit check timer */
> time_t ps_nextxcpu;/* when to send next SIGXCPU, */
> /* in seconds of process runtime */
> @@ -269,7 +272,7 @@ struct process {
> int ps_refcnt; /* Number of references. */
>
> struct timespec ps_start; /* starting uptime. */
> - struct timeout ps_realit_to; /* real-time itimer trampoline. */
> + struct timeout ps_realit_to; /* [K] ITIMER_REAL
process: annotate locking for setitimer(2) state
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.
Preferences? ok?
Index: kern/kern_time.c
===
RCS file: /cvs/src/sys/kern/kern_time.c,v
retrieving revision 1.134
diff -u -p -r1.134 kern_time.c
--- kern/kern_time.c8 Aug 2020 01:01:26 - 1.134
+++ kern/kern_time.c9 Aug 2020 00:41:10 -
@@ -488,7 +488,13 @@ out:
}
-struct mutex itimer_mtx = MUTEX_INITIALIZER(IPL_CLOCK);
+/*
+ * Global virtual interval timer mutex.
+ *
+ * Protects state for the per-process ITIMER_VIRTUAL and ITIMER_PROF
+ * interval timers.
+ */
+struct mutex virtual_itimer_mtx = MUTEX_INITIALIZER(IPL_CLOCK);
/*
* Get value of an interval timer. The process virtual and
@@ -526,10 +532,10 @@ sys_getitimer(struct proc *p, void *v, r
return (EINVAL);
itimer = >p_p->ps_timer[which];
memset(, 0, sizeof(aitv));
- mtx_enter(_mtx);
+ mtx_enter(_itimer_mtx);
TIMESPEC_TO_TIMEVAL(_interval, >it_interval);
TIMESPEC_TO_TIMEVAL(_value, >it_value);
- mtx_leave(_mtx);
+ mtx_leave(_itimer_mtx);
if (which == ITIMER_REAL) {
struct timeval now;
@@ -604,9 +610,9 @@ sys_setitimer(struct proc *p, void *v, r
}
pr->ps_timer[ITIMER_REAL] = aits;
} else {
- mtx_enter(_mtx);
+ mtx_enter(_itimer_mtx);
pr->ps_timer[which] = aits;
- mtx_leave(_mtx);
+ mtx_leave(_itimer_mtx);
}
return (0);
@@ -681,20 +687,20 @@ itimerdecr(struct itimerspec *itp, long
NSEC_TO_TIMESPEC(nsec, );
- mtx_enter(_mtx);
+ mtx_enter(_itimer_mtx);
timespecsub(>it_value, , >it_value);
if (itp->it_value.tv_sec >= 0 && timespecisset(>it_value)) {
- mtx_leave(_mtx);
+ mtx_leave(_itimer_mtx);
return (1);
}
if (!timespecisset(>it_interval)) {
timespecclear(>it_value);
- mtx_leave(_mtx);
+ mtx_leave(_itimer_mtx);
return (0);
}
while (itp->it_value.tv_sec < 0 || !timespecisset(>it_value))
timespecadd(>it_value, >it_interval, >it_value);
- mtx_leave(_mtx);
+ mtx_leave(_itimer_mtx);
return (0);
}
Index: sys/proc.h
===
RCS file: /cvs/src/sys/sys/proc.h,v
retrieving revision 1.297
diff -u -p -r1.297 proc.h
--- sys/proc.h 6 Jul 2020 13:33:09 - 1.297
+++ sys/proc.h 9 Aug 2020 00:41:11 -
@@ -150,9 +150,11 @@ struct unveil;
/*
* Locks used to protect struct members in this file:
* a atomic operations
+ * K kernel lock
* m this process' `ps_mtx'
* p this process' `ps_lock'
* R rlimit_lock
+ * V virtual_itimer_mtx
*/
struct process {
/*
@@ -216,7 +218,8 @@ struct process {
struct rusage *ps_ru; /* sum of stats for dead threads. */
struct tusage ps_tu; /* accumulated times. */
struct rusage ps_cru; /* sum of stats for reaped children */
- struct itimerspec ps_timer[3]; /* timers, indexed by ITIMER_* */
+ struct itimerspec ps_timer[3]; /* [K] ITIMER_REAL timer */
+ /* [V] ITIMER_{PROF,VIRTUAL} timers */
struct timeout ps_rucheck_to; /* [] resource limit check timer */
time_t ps_nextxcpu;/* when to send next SIGXCPU, */
/* in seconds of process runtime */
@@ -269,7 +272,7 @@ struct process {
int ps_refcnt; /* Number of references. */
struct timespec ps_start; /* starting uptime. */
- struct timeout ps_realit_to; /* real-time itimer trampoline. */
+ struct timeout ps_realit_to; /* [K] ITIMER_REAL timeout */
};
#defineps_session ps_pgrp->pg_session
