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
