On Tue, Feb 25, 2020 at 12:25 AM Johannes Weiner <hannes@xxxxxxxxxxx> 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
