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;
Date:   Mon, 24 Feb 2020 11:25:16 -0500
From:   Johannes Weiner <hannes@cmpxchg.org>
To:     Yafang Shao <laoar.shao@gmail.com>
Cc:     mingo@redhat.com, peterz@infradead.org, juri.lelli@redhat.com,
        vincent.guittot@linaro.org, dietmar.eggemann@arm.com,
        rostedt@goodmis.org, bsegall@google.com, mgorman@suse.de,
        akpm@linux-foundation.org, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] psi: move PF_MEMSTALL into psi specific psi_flags
Message-ID: <20200224162516.GA1674@cmpxchg.org>
References: <20200222144647.10120-1-laoar.shao@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200222144647.10120-1-laoar.shao@gmail.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.

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:

> @@ -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