Re: [PATCH] psi: move PF_MEMSTALL into psi specific psi_flags

[Yafang Shao <laoar.shao@gmail.com>]

On Tue, Feb 25, 2020 at 12:25 AM Johannes Weiner <hannes@cmpxchg.org> wrote:
>
> Hello Yafang,
>
> On Sat, Feb 22, 2020 at 09:46:47AM -0500, Yafang Shao wrote:
> > The task->flags is a 32-bits flag, in which 31 bits have already been
> > consumed. So it is hardly to introduce other new per process flag.
> > As there's a psi specific flag psi_flags, we'd better move the psi specific
> > per process flag PF_MEMSTALL into it.
>
> Currently, psi_flags is used only for debugging:
>
>         if (((task->psi_flags & set) ||
>              (task->psi_flags & clear) != clear) &&
>             !psi_bug) {
>                 printk_deferred(KERN_ERR "psi: inconsistent task state! task=%d:%s cpu=%d psi_flags=%x clear=%x set=%x\n",
>                                 task->pid, task->comm, cpu,
>                                 task->psi_flags, clear, set);
>                 psi_bug = 1;
>         }
>
>         task->psi_flags &= ~clear;
>         task->psi_flags |= set;
>
> While this has caught a few bugs while the code was new, I'm planning
> on moving it to a CONFIG option that is only enabled in debug builds.
>

Got it. Many thanks for you explanation.

> If you need the room in task->flags, can you please make the memstall
> state a single bit in task_struct instead? AFAICS there is still space
> in this section:
>
>         /* Force alignment to the next boundary: */
>         unsigned                        :0;
>
>         /* Unserialized, strictly 'current' */
>
>         ...
>
> #ifdef CONFIG_PSI
>         unsigned                        in_memstall:1;
> #endif
>
> It would also avoid the mixed-bit masking headache:
>

Seems that's a better solution. I will update with it.
Thanks for your suggestion.

> > @@ -17,11 +17,21 @@ enum psi_task_count {
> >       NR_PSI_TASK_COUNTS = 3,
> >  };
> >
> > -/* Task state bitmasks */
> > +/*
> > + * Task state bitmasks:
> > + * These flags are stored in the lower PSI_TSK_BITS bits of
> > + * task->psi_flags, and the higher bits are set with per process flag which
> > + * persists across sleeps.
> > + */
> > +#define PSI_TSK_STATE_BITS 16
> > +#define PSI_TSK_STATE_MASK ((1 << PSI_TSK_STATE_BITS) - 1)
> >  #define TSK_IOWAIT   (1 << NR_IOWAIT)
> >  #define TSK_MEMSTALL (1 << NR_MEMSTALL)
> >  #define TSK_RUNNING  (1 << NR_RUNNING)
> >
> > +/* Stalled due to lack of memory, that's per process flag. */
> > +#define PSI_PF_MEMSTALL (1 << PSI_TSK_STATE_BITS)
> > +
> >  /* Resources that workloads could be stalled on */
> >  enum psi_res {
> >       PSI_IO,
> > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > index f314790cb527..2d4c04d35d9b 100644
> > --- a/include/linux/sched.h
> > +++ b/include/linux/sched.h
> > @@ -1025,7 +1025,11 @@ struct task_struct {
> >
> >       struct task_io_accounting       ioac;
> >  #ifdef CONFIG_PSI
> > -     /* Pressure stall state */
> > +     /*
> > +      * Pressure stall state:
> > +      * Bits 0 ~ PSI_TSK_STATE_BITS-1: PSI task states
> > +      * Bits PSI_TSK_STATE_BITS ~ 31: Per process flags
> > +      */
> >       unsigned int                    psi_flags;
> >  #endif
> >  #ifdef CONFIG_TASK_XACCT
>
> Thanks



-- 
Yafang Shao
DiDi