Hi,

Before merging the critical sections for setitimer(2) and getitimer(2)
we need to make their critical sections as similar as possible.

In getitimer(2) we read the timer value in one place in the code:

        /* getitimer(2) pseudocode */
        if (which != ITIMER_REAL)
                mtx_enter(&itimer_mtx);

        /* read the timer */

        if (which != ITIMER_REAL)
                mtx_leave(&itimer_mtx);

The corresponding write logic in setitimer(2) is laid out differently.
The code that actually performs the write is duplicated:

        /* setitimer(2) pseudocode (as-is) */
        if (which == ITIMER_REAL) {
                /* ITIMER_REAL-specific stuff */
>>>             pr->ps_timer[ITIMER_REAL] = aits;
        } else {
                mtx_enter(&itimer_mtx);
>>>             pr->ps_timer[which] = aits;
                mtx_leave(&itimer_mtx);
        }

If we rearrange the setitimer(2) code to do the write in one place...

        /* setitimer(2) pseudocode (rearranged) */
        if (which != ITIMER_REAL)
                mtx_enter(&itimer_mtx);

        if (which == ITIMER_REAL) {
                /* ITIMER_REAL-specific stuff */
        }
>>>     pr->ps_timer[which] = aits;

        if (which != ITIMER_REAL)
                mtx_leave(&itimer_mtx);

... merging the critical sections in a subsequent patch will be
easier.

ok?

Index: kern_time.c
===================================================================
RCS file: /cvs/src/sys/kern/kern_time.c,v
retrieving revision 1.137
diff -u -p -r1.137 kern_time.c
--- kern_time.c 11 Aug 2020 18:29:58 -0000      1.137
+++ kern_time.c 11 Aug 2020 20:43:32 -0000
@@ -597,6 +597,10 @@ sys_setitimer(struct proc *p, void *v, r
        }
        if (itvp == 0)
                return (0);
+
+       if (which != ITIMER_REAL)
+               mtx_enter(&itimer_mtx);
+
        if (which == ITIMER_REAL) {
                struct timespec cts;
 
@@ -607,12 +611,11 @@ sys_setitimer(struct proc *p, void *v, r
                        timeout_add(&pr->ps_realit_to, timo);
                        timespecadd(&aits.it_value, &cts, &aits.it_value);
                }
-               pr->ps_timer[ITIMER_REAL] = aits;
-       } else {
-               mtx_enter(&itimer_mtx);
-               pr->ps_timer[which] = aits;
-               mtx_leave(&itimer_mtx);
        }
+       pr->ps_timer[which] = aits;
+
+       if (which != ITIMER_REAL)
+               mtx_leave(&itimer_mtx);
 
        return (0);
 }

Reply via email to