Re: [PATCH v3 6/6] freezer,sched: Rewrite core freezer logic

Ingo Molnar
2022-09-04T10:09:50Z

* 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