Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Sender: Ingo Molnar <mingo.kernel.org@gmail.com>
Date:   Sun, 4 Sep 2022 12:09:37 +0200
From:   Ingo Molnar <mingo@kernel.org>
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     rjw@rjwysocki.net, oleg@redhat.com, vincent.guittot@linaro.org,
        dietmar.eggemann@arm.com, rostedt@goodmis.org, mgorman@suse.de,
        ebiederm@xmission.com, bigeasy@linutronix.de,
        Will Deacon <will@kernel.org>, linux-kernel@vger.kernel.org,
        tj@kernel.org, linux-pm@vger.kernel.org
Subject: Re: [PATCH v3 6/6] freezer,sched: Rewrite core freezer logic
Message-ID: <YxR5Yauhd90WN/AY@gmail.com>
References: <20220822111816.760285417@infradead.org>
 <20220822114649.055452969@infradead.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220822114649.055452969@infradead.org>
Precedence: bulk


* Peter Zijlstra <peterz@infradead.org> wrote:

> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -81,25 +81,32 @@ struct task_group;
>   */
>  
>  /* Used in tsk->state: */
> -#define TASK_RUNNING			0x0000
> -#define TASK_INTERRUPTIBLE		0x0001
> -#define TASK_UNINTERRUPTIBLE		0x0002
> -#define __TASK_STOPPED			0x0004
> -#define __TASK_TRACED			0x0008
> +#define TASK_RUNNING			0x000000
> +#define TASK_INTERRUPTIBLE		0x000001
> +#define TASK_UNINTERRUPTIBLE		0x000002
> +#define __TASK_STOPPED			0x000004
> +#define __TASK_TRACED			0x000008
>  /* Used in tsk->exit_state: */
> -#define EXIT_DEAD			0x0010
> -#define EXIT_ZOMBIE			0x0020
> +#define EXIT_DEAD			0x000010
> +#define EXIT_ZOMBIE			0x000020
>  #define EXIT_TRACE			(EXIT_ZOMBIE | EXIT_DEAD)
>  /* Used in tsk->state again: */
> -#define TASK_PARKED			0x0040
> -#define TASK_DEAD			0x0080
> -#define TASK_WAKEKILL			0x0100
> -#define TASK_WAKING			0x0200
> -#define TASK_NOLOAD			0x0400
> -#define TASK_NEW			0x0800
> -/* RT specific auxilliary flag to mark RT lock waiters */
> -#define TASK_RTLOCK_WAIT		0x1000
> -#define TASK_STATE_MAX			0x2000
> +#define TASK_PARKED			0x000040
> +#define TASK_DEAD			0x000080
> +#define TASK_WAKEKILL			0x000100
> +#define TASK_WAKING			0x000200
> +#define TASK_NOLOAD			0x000400
> +#define TASK_NEW			0x000800
> +#define TASK_FREEZABLE			0x001000
> +#define __TASK_FREEZABLE_UNSAFE	       (0x002000 * IS_ENABLED(CONFIG_LOCKDEP))
> +#define TASK_FROZEN			0x004000
> +#define TASK_RTLOCK_WAIT		0x008000
> +#define TASK_STATE_MAX			0x010000

Patch ordering suggestion: would be really nice to first do the width 
adjustment as a preparatory patch, then the real changes. The mixing 
obscures what the patch is doing here, that we leave all bits before 
TASK_NEW unchanged, add in TASK_FREEZABLE, __TASK_FREEZABLE_UNSAFE & 
TASK_FROZEN to before TASK_RTLOCK_WAIT.

Btw., wouldn't it be better to just add the new bits right before 
TASK_STATE_MAX, and leave the existing ones unchanged? I don't think the 
order of TASK_RTLOCK_WAIT is relevant, right?

>  /* Convenience macros for the sake of set_current_state: */
>  #define TASK_KILLABLE			(TASK_WAKEKILL | TASK_UNINTERRUPTIBLE)
> @@ -1714,7 +1721,6 @@ extern struct pid *cad_pid;
>  #define PF_NPROC_EXCEEDED	0x00001000	/* set_user() noticed that RLIMIT_NPROC was exceeded */
>  #define PF_USED_MATH		0x00002000	/* If unset the fpu must be initialized before use */
>  #define PF_NOFREEZE		0x00008000	/* This thread should not be frozen */
> -#define PF_FROZEN		0x00010000	/* Frozen for system suspend */
>  #define PF_KSWAPD		0x00020000	/* I am kswapd */
>  #define PF_MEMALLOC_NOFS	0x00040000	/* All allocation requests will inherit GFP_NOFS */
>  #define PF_MEMALLOC_NOIO	0x00080000	/* All allocation requests will inherit GFP_NOIO */

yay.

BTW., we should probably mark/document all PF_ holes with a PF__RESERVED 
kind of scheme? Something simple, like:

   #define PF_NPROC_EXCEEDED	0x00001000	/* set_user() noticed that RLIMIT_NPROC was exceeded */
   #define PF_USED_MATH		0x00002000	/* If unset the fpu must be initialized before use */
 + #define PF__RESERVED_04000	0x00004000	/* Unused */
   #define PF_NOFREEZE		0x00008000	/* This thread should not be frozen */
 + #define PF__RESERVED_10000	0x00010000	/* Unused */
   #define PF_KSWAPD		0x00020000	/* I am kswapd */
   #define PF_MEMALLOC_NOFS	0x00040000	/* All allocation requests will inherit GFP_NOFS */
   #define PF_MEMALLOC_NOIO	0x00080000	/* All allocation requests will inherit GFP_NOIO */

?

Thanks,

	Ingo