Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20200222144647.10120-1-laoar.shao@gmail.com> <20200224162516.GA1674@cmpxchg.org>
In-Reply-To: <20200224162516.GA1674@cmpxchg.org>
From:   Yafang Shao <laoar.shao@gmail.com>
Date:   Wed, 26 Feb 2020 22:23:16 +0800
Message-ID: <CALOAHbBOx7moLeVNVDOkstR9QWyaKOmJXmUxtiJL0Uhyg7CDgg@mail.gmail.com>
Subject: Re: [PATCH] psi: move PF_MEMSTALL into psi specific psi_flags
To:     Johannes Weiner <hannes@cmpxchg.org>
Cc:     mingo@redhat.com, Peter Zijlstra <peterz@infradead.org>,
        juri.lelli@redhat.com,
        Vincent Guittot <vincent.guittot@linaro.org>,
        dietmar.eggemann@arm.com, Steven Rostedt <rostedt@goodmis.org>,
        Benjamin Segall <bsegall@google.com>, mgorman@suse.de,
        Andrew Morton <akpm@linux-foundation.org>,
        Linux MM <linux-mm@kvack.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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