Re: Critical section

[Masao Uebayashi <uebayasi%gmail.com@localhost>]

Critical section must stop soft interrupt which may block & sleep
(using the preempted lwp).  Thus critical sections must be at least
IPL_SOFTSERIAL.

On Wed, Nov 26, 2014 at 4:41 PM, Masao Uebayashi <uebayasi%gmail.com@localhost> wrote:
> The problem of kpreempt_*() API is that its meaning is overriden by kernel
> internal (scheduler, sync primitives, ...).  This change separates the internal
> use (scheduler disables preeemption) and others (kernel subsystem code executes
> critical section).  Detect sleep from within critical section in mi_switch().
>
> The only problem I've seen is, cprng_fast.c calling percpu_getref() in
> KASSERT(); it's kind of re-entrance.
>
> Index: sys/crypto/cprng_fast/cprng_fast.c
> ===================================================================
> RCS file: /cvsroot/src/sys/crypto/cprng_fast/cprng_fast.c,v
> retrieving revision 1.11
> diff -p -u -r1.11 cprng_fast.c
> --- sys/crypto/cprng_fast/cprng_fast.c  11 Aug 2014 22:36:49 -0000      1.11
> +++ sys/crypto/cprng_fast/cprng_fast.c  26 Nov 2014 07:35:51 -0000
> @@ -258,8 +258,10 @@ static inline void
>  cprng_fast_put(struct cprng_fast *cprng, int s)
>  {
>
> +#if 0
>         KASSERT((cprng == percpu_getref(cprng_fast_percpu)) &&
>             (percpu_putref(cprng_fast_percpu), true));
> +#endif
>         splx(s);
>         percpu_putref(cprng_fast_percpu);
>  }
> Index: sys/kern/kern_synch.c
> ===================================================================
> RCS file: /cvsroot/src/sys/kern/kern_synch.c,v
> retrieving revision 1.308
> diff -p -u -r1.308 kern_synch.c
> --- sys/kern/kern_synch.c       28 Feb 2014 10:16:51 -0000      1.308
> +++ sys/kern/kern_synch.c       26 Nov 2014 07:35:51 -0000
> @@ -441,6 +441,54 @@ kpreempt_enable(void)
>  }
>
>  /*
> + * Critical section.
> + *
> + * - Kernel subsystems can declare critical sections.
> + *   - Kernel core (scheduler and synchronization implementation) use
> + *     KPREEMPT_DISABLE()/KPREEMPT_ENABLE().
> + * - Not re-entrant.
> + *   - If re-entered, panic is triggered.
> + * - Can't sleep.
> + *   - If calling threads sleep (enter scheduler), panic is triggered.
> + * - Kernel preemption is disabled.
> + * - Callers ensure appropriate IPL.
> + *   - If there's no strong reason, IPL_SOFT* is recommended, because
> + *     setting H/W interrupt level is expensive itself.
> + */
> +
> +#define        CRIT_BIT        0x10000 /* set in l_nopreempt */
> +
> +void
> +crit_enter(void)
> +{
> +       lwp_t * const l = curlwp;
> +
> +       KASSERTMSG((l->l_nopreempt & CRIT_BIT) == 0, "l_nopreempt=%x",
> +           l->l_nopreempt);
> +       l->l_nopreempt |= CRIT_BIT;
> +}
> +
> +void
> +crit_exit(void)
> +{
> +       lwp_t * const l = curlwp;
> +
> +       KASSERTMSG((l->l_nopreempt & CRIT_BIT) != 0, "l_nopreempt=%x",
> +           l->l_nopreempt);
> +       l->l_nopreempt &= ~CRIT_BIT;
> +       if (__predict_false(l->l_dopreempt))
> +               kpreempt(0);
> +}
> +
> +bool
> +crit_entered(void)
> +{
> +       const lwp_t * const l = curlwp;
> +
> +       return (l->l_nopreempt & CRIT_BIT) != 0;
> +}
> +
> +/*
>   * Compute the amount of time during which the current lwp was running.
>   *
>   * - update l_rtime unless it's an idle lwp.
> @@ -514,6 +562,7 @@ mi_switch(lwp_t *l)
>         bool returning;
>
>         KASSERT(lwp_locked(l, NULL));
> +       KASSERT(!crit_entered());
>         KASSERT(kpreempt_disabled());
>         LOCKDEBUG_BARRIER(l->l_mutex, 1);
>
> Index: sys/kern/subr_percpu.c
> ===================================================================
> RCS file: /cvsroot/src/sys/kern/subr_percpu.c,v
> retrieving revision 1.16
> diff -p -u -r1.16 subr_percpu.c
> --- sys/kern/subr_percpu.c      27 Jan 2012 19:48:40 -0000      1.16
> +++ sys/kern/subr_percpu.c      26 Nov 2014 07:35:51 -0000
> @@ -291,7 +291,7 @@ void *
>  percpu_getref(percpu_t *pc)
>  {
>
> -       KPREEMPT_DISABLE(curlwp);
> +       crit_enter();
>         return percpu_getptr_remote(pc, curcpu());
>  }
>
> @@ -306,7 +306,7 @@ void
>  percpu_putref(percpu_t *pc)
>  {
>
> -       KPREEMPT_ENABLE(curlwp);
> +       crit_exit();
>  }
>
>  /*
> Index: sys/kern/subr_pserialize.c
> ===================================================================
> RCS file: /cvsroot/src/sys/kern/subr_pserialize.c,v
> retrieving revision 1.7
> diff -p -u -r1.7 subr_pserialize.c
> --- sys/kern/subr_pserialize.c  7 Feb 2013 23:37:58 -0000       1.7
> +++ sys/kern/subr_pserialize.c  26 Nov 2014 07:35:51 -0000
> @@ -187,6 +187,7 @@ pserialize_read_enter(void)
>  {
>
>         KASSERT(!cpu_intr_p());
> +       crit_enter();
>         return splsoftserial();
>  }
>
> @@ -195,6 +196,7 @@ pserialize_read_exit(int s)
>  {
>
>         splx(s);
> +       crit_exit();
>  }
>
>  /*
> Index: sys/sys/systm.h
> ===================================================================
> RCS file: /cvsroot/src/sys/sys/systm.h,v
> retrieving revision 1.266
> diff -p -u -r1.266 systm.h
> --- sys/sys/systm.h     3 Aug 2014 12:49:32 -0000       1.266
> +++ sys/sys/systm.h     26 Nov 2014 07:35:51 -0000
> @@ -522,6 +522,9 @@ do {                                                \
>  void   kpreempt_disable(void);
>  void   kpreempt_enable(void);
>  bool   kpreempt_disabled(void);
> +void   crit_enter(void);
> +void   crit_exit(void);
> +bool   crit_entered(void);
>  #endif
>
>  void assert_sleepable(void);