On Mon, Jul 25, 2022 at 06:44:31PM -0500, Scott Cheloha wrote:
> On Mon, Jul 25, 2022 at 01:52:36PM +0200, Mark Kettenis wrote:
> > > Date: Sun, 24 Jul 2022 19:33:57 -0500
> > > From: Scott Cheloha <scottchel...@gmail.com>
> > > 
> > > On Sat, Jul 23, 2022 at 08:14:32PM -0500, Scott Cheloha wrote:
> > > > 
> > > > [...]
> > > > 
> > > > I don't have a powerpc64 machine, so this is untested.  [...]
> > > 
> > > gkoehler@ has pointed out two dumb typos in the prior patch.  My bad.
> > > 
> > > Here is a corrected patch that, according to gkoehler@, actually
> > > compiles.
> > 
> > Thanks.  I already figured that bit out myself.  Did some limited
> > testing, but it seems to work correctly.  No noticable effect on the
> > timekeeping even when building clang on all the (4) cores.
> 
> I wouldn't expect this patch to impact timekeeping.  All we're doing
> is calling hardclock(9) a bit sooner than we normally would a few
> times every second.
> 
> I would expect to see slightly more distinct interrupts (uvmexp.intrs)
> per second because we aren't actively batching hardclock(9) and
> statclock calls.
> 
> ... by the way, uvmexp.intrs should probably be incremented
> atomically, no?
> 
> > Regarding the diff, I think it would be better to avoid changing
> > trap.c.  That function is complicated enough and splitting the logic
> > for this over three files makes it a bit harder to understand.  So you
> > could have:
> > 
> > void
> > decr_intr(struct trapframe *frame)
> > {
> >     struct cpu_info *ci = curcpu();
> >     ...
> >     int s;
> > 
> >     if (ci->ci_cpl >= IPL_CLOCK) {
> >             ci->ci_dec_deferred = 1;
> >             mtdec(UINT32_MAX >> 1); /* clear DEC exception */
> >             return;
> >     }
> > 
> >     ci->ci_dec_deferred = 0;
> > 
> >     ...
> > }
> > 
> > That has the downside of course that it will be slightly less
> > efficient if we're at IPL_CLOCK or above, but that really shouldn't
> > happen often enough for it to matter.
> 
> Yep.  It's an extra function call, the overhead is small.
> 
> Updated patch below.

At what point do we consider the patch safe?  Have you seen any hangs?

Wanna run with it another week?

Index: include/cpu.h
===================================================================
RCS file: /cvs/src/sys/arch/powerpc64/include/cpu.h,v
retrieving revision 1.31
diff -u -p -r1.31 cpu.h
--- include/cpu.h       6 Jul 2021 09:34:07 -0000       1.31
+++ include/cpu.h       25 Jul 2022 23:43:47 -0000
@@ -74,9 +74,9 @@ struct cpu_info {
        uint64_t        ci_lasttb;
        uint64_t        ci_nexttimerevent;
        uint64_t        ci_nextstatevent;
-       int             ci_statspending;
        
        volatile int    ci_cpl;
+       volatile int    ci_dec_deferred;
        uint32_t        ci_ipending;
        uint32_t        ci_idepth;
 #ifdef DIAGNOSTIC
Index: powerpc64/clock.c
===================================================================
RCS file: /cvs/src/sys/arch/powerpc64/powerpc64/clock.c,v
retrieving revision 1.3
diff -u -p -r1.3 clock.c
--- powerpc64/clock.c   23 Feb 2021 04:44:31 -0000      1.3
+++ powerpc64/clock.c   25 Jul 2022 23:43:47 -0000
@@ -98,6 +98,17 @@ decr_intr(struct trapframe *frame)
        int s;
 
        /*
+        * If the clock interrupt is masked, postpone all work until
+        * it is unmasked in splx(9).
+        */
+       if (ci->ci_cpl >= IPL_CLOCK) {
+               ci->ci_dec_deferred = 1;
+               mtdec(UINT32_MAX >> 1);         /* clear DEC exception */
+               return;
+       }
+       ci->ci_dec_deferred = 0;
+
+       /*
         * Based on the actual time delay since the last decrementer reload,
         * we arrange for earlier interrupt next time.
         */
@@ -130,30 +141,23 @@ decr_intr(struct trapframe *frame)
        mtdec(nextevent - tb);
        mtdec(nextevent - mftb());
 
-       if (ci->ci_cpl >= IPL_CLOCK) {
-               ci->ci_statspending += nstats;
-       } else {
-               nstats += ci->ci_statspending;
-               ci->ci_statspending = 0;
-
-               s = splclock();
-               intr_enable();
-
-               /*
-                * Do standard timer interrupt stuff.
-                */
-               while (ci->ci_lasttb < prevtb) {
-                       ci->ci_lasttb += tick_increment;
-                       clock_count.ec_count++;
-                       hardclock((struct clockframe *)frame);
-               }
+       s = splclock();
+       intr_enable();
 
-               while (nstats-- > 0)
-                       statclock((struct clockframe *)frame);
-
-               intr_disable();
-               splx(s);
+       /*
+        * Do standard timer interrupt stuff.
+        */
+       while (ci->ci_lasttb < prevtb) {
+               ci->ci_lasttb += tick_increment;
+               clock_count.ec_count++;
+               hardclock((struct clockframe *)frame);
        }
+
+       while (nstats-- > 0)
+               statclock((struct clockframe *)frame);
+
+       intr_disable();
+       splx(s);
 }
 
 void
Index: powerpc64/intr.c
===================================================================
RCS file: /cvs/src/sys/arch/powerpc64/powerpc64/intr.c,v
retrieving revision 1.9
diff -u -p -r1.9 intr.c
--- powerpc64/intr.c    26 Sep 2020 17:56:54 -0000      1.9
+++ powerpc64/intr.c    25 Jul 2022 23:43:47 -0000
@@ -139,6 +139,11 @@ splx(int new)
 {
        struct cpu_info *ci = curcpu();
 
+       if (ci->ci_dec_deferred && new < IPL_CLOCK) {
+               mtdec(0);
+               mtdec(UINT32_MAX);      /* raise DEC exception */
+       }
+
        if (ci->ci_ipending & intr_smask[new])
                intr_do_pending(new);
 

Reply via email to