sched: make struct task_struct::state 32-bit

32-bit accesses are shorter than 64-bit accesses on x86_64. Nothing uses 64-bitness of ->state.

Space savings are ~2KB on F30 kernel config.

Signed-off-by Alexey Dobriyan <[email protected]>

---

struct task_struct {

	struct thread_info         thread_info;          /*     0    24 */
	volatile int               state;                /*    24     4 */

	/* XXX 4 bytes hole, try to pack */

	void *                     stack;                /*    32     8 */

Though seeing as this is also the boundary of the randomized layout we can't really do much better without changing the boundary itself. So much for cacheline use :/

Quoted 6 lines · Valentin Schneider

struct task_struct {

	struct thread_info         thread_info;          /*     0    24 */
	volatile int               state;                /*    24     4 */

	/* XXX 4 bytes hole, try to pack */

Quoted 5 lines

void * stack; /* 32 8 */

Though seeing as this is also the boundary of the randomized layout we can't really do much better without changing the boundary itself. So much for cacheline use :/

Cacheline use of task_struct is pretty hopeless because of all the ifdefs.

32-bit accesses are shorter than 64-bit accesses on x86_64. Nothing uses 64-bitness of ->state.

Space savings are ~2KB on F30 kernel config.

Quoted 4 lines · Alexey Dobriyan

32-bit accesses are shorter than 64-bit accesses on x86_64. Nothing uses 64-bitness of ->state.

Space savings are ~2KB on F30 kernel config.

I guess we'd save even more when moving from a volatile to WRITE_ONCE/READ_ONCE..

32-bit accesses are shorter than 64-bit accesses on x86_64. Nothing uses 64-bitness of ->state.

Space savings are ~2KB on F30 kernel config.

Signed-off-by Alexey Dobriyan <[email protected]>

---

 arch/ia64/kernel/perfmon.c   |    4 ++--
 block/blk-mq.c               |    2 +-
 drivers/md/dm.c              |    4 ++--
 fs/userfaultfd.c             |    2 +-
 include/linux/sched.h        |    6 +++---
 include/linux/sched/debug.h  |    2 +-
 include/linux/sched/signal.h |    2 +-
 kernel/freezer.c             |    2 +-
 kernel/kthread.c             |    4 ++--
 kernel/locking/mutex.c       |    6 +++---
 kernel/locking/semaphore.c   |    2 +-
 kernel/rcu/rcutorture.c      |    4 ++--
 kernel/rcu/tree_stall.h      |    6 +++---
 kernel/sched/core.c          |    8 ++++----
 lib/syscall.c                |    2 +-
 15 files changed, 28 insertions(+), 28 deletions(-)

Quoted 2 lines · Alexey Dobriyan

32-bit accesses are shorter than 64-bit accesses on x86_64. Nothing uses 64-bitness of ->state.

It looks like you missed a few places. There's a long prev_state in sched/core.c::finish_task_switch() for instance.

I suppose that's where coccinelle oughta help but I'm really not fluent in that. Is there a way to make it match p.state accesses with p task_struct? And if so, can we make it change the type of the variable being read from / written to?

How did you come up with this changeset, did you pickaxe for some regexp?

Quoted 11 lines · Valentin Schneider

Quoted 2 lines · Alexey Dobriyan

32-bit accesses are shorter than 64-bit accesses on x86_64. Nothing uses 64-bitness of ->state.

It looks like you missed a few places. There's a long prev_state in sched/core.c::finish_task_switch() for instance.

I suppose that's where coccinelle oughta help but I'm really not fluent in that. Is there a way to make it match p.state accesses with p task_struct? And if so, can we make it change the type of the variable being read from / written to?

Coccinelle is interesting: basic

- foo
+ bar

doesn't find "foo" in function arguments.

I'm scared of coccinelle.>

Quoted 1 line

How did you come up with this changeset, did you pickaxe for some regexp?

No, manually, backtracking up to the call chain.
Maybe I missed a few places.

I tried something for function parameters, which seems to be feasible according to [1], but couldn't get it to work (yet). Here's what I have so far:

[...]
Baby steps...

@funcmatch@
identifier func;
identifier p;
identifier state_var;
@@

func(..., struct task_struct *p, ...

(quoted diff) +1 -1

-      , long state_var
+      , int state_var
       ,...)

{
	...
}

Which seems to be doing roughly what I want.

Can a transformation approach like the following work also for your software?

@replacement@

identifier func, p, state_var;

@@

func(...,
struct task_struct *p,
...
,

(quoted diff) +1 -1

-     long
+     int
      state_var

,

      ...)

{

...

}

Regards,
Markus

I actually got rid of the task_struct* parameter and now just match against task_struct.p accesses in the function body, which has the added bonus of not caring about the order of the parameters.

Still not there yet but making progress in the background, hope it's passable entertainment to see me struggle my way there :)

Quoted 7 lines · Valentin Schneider

I actually got rid of the task_struct* parameter and now just match against task_struct.p accesses in the function body, which has the added bonus of not caring about the order of the parameters.

Still not there yet but making progress in the background, hope it's passable entertainment to see me struggle my way there :)

Bit of hiatus on my end there. I did play around some more with Coccinelle on the way to/from Plumbers. The main problems I'm facing ATM is "current" not being recognized as a task_struct* expression, and the need to "recursively" match task_struct.state modifiers, i.e. catch both functions for something like:

foo(long state)

{
	__foo(state);
}

__foo(long state)

{
	current->state = state;
}

Here's where I'm at:
---
virtual patch
virtual report

// Match variables that represent task states
// They can be read from / written to task_struct.state, or be compared // to TASK_* values
@state_access@
struct task_struct *p;
// FIXME: current not recognized as task_struct*, fixhack with regexp identifier current =~ "^current$";

identifier task_state =~ "^TASK_";

identifier state_var;
position pos;
@@

(
p->state & state_var@pos
|
current->state & state_var@pos
|
p->state | state_var@pos
|
current->state | state_var@pos
|
p->state < state_var@pos
|
current->state < state_var@pos
|
p->state > state_var@pos
|
current->state > state_var@pos
|
state_var@pos = p->state
|
state_var@pos = current->state
|
p->state == state_var@pos
|

current->state == state_var@pos
|
p->state != state_var@pos
|
current->state != state_var@pos
|
// FIXME: match functions that do something with state_var underneath? // How to do recursive rules?

set_current_state(state_var@pos)
|
set_special_state(state_var@pos)
|
signal_pending_state(state_var@pos, p)
|
signal_pending_state(state_var@pos, current)
|
state_var@pos & task_state
|
state_var@pos | task_state
)

// Fixup local variables
@depends on patch && state_access@
identifier state_var = state_access.state_var;
@@
(
- long
+ int
|
- unsigned long
+ unsigned int
)
state_var;

// Fixup function parameters
@depends on patch && state_access@
identifier fn;
identifier state_var = state_access.state_var;
@@

fn(...,
- long state_var
+ int state_var
,...)

{
	...
}

// FIXME: find a way to squash that with the above?

// Fixup function parameters
@depends on patch && state_access@
identifier fn;
identifier state_var = state_access.state_var;
@@

fn(...,
- unsigned long
+ unsigned int
state_var
,...)

{
	...
}

---

This gives me the following diff on kernel/:

---
diff -u -p a/locking/mutex.c b/locking/mutex.c

locking/mutex.c +3 -3

@@ -923,7 +923,7 @@ __ww_mutex_add_waiter(struct mutex_waite
  * Lock a mutex (possibly interruptible), slowpath:
  */
 static __always_inline int __sched
-__mutex_lock_common(struct mutex *lock, long state, unsigned int subclass,
+__mutex_lock_common(struct mutex *lock, int state, unsigned int subclass,
 		    struct lockdep_map *nest_lock, unsigned long ip,
 		    struct ww_acquire_ctx *ww_ctx, const bool use_ww_ctx)
 {

@@ -1097,14 +1097,14 @@ err_early_kill:
 }
 
 static int __sched
-__mutex_lock(struct mutex *lock, long state, unsigned int subclass,
+__mutex_lock(struct mutex *lock, int state, unsigned int subclass,
 	     struct lockdep_map *nest_lock, unsigned long ip)
 {
 	return __mutex_lock_common(lock, state, subclass, nest_lock, ip, NULL, false);
 }
 
 static int __sched
-__ww_mutex_lock(struct mutex *lock, long state, unsigned int subclass,
+__ww_mutex_lock(struct mutex *lock, int state, unsigned int subclass,
 		struct lockdep_map *nest_lock, unsigned long ip,
 		struct ww_acquire_ctx *ww_ctx)
 {

diff -u -p a/locking/semaphore.c b/locking/semaphore.c

locking/semaphore.c +1 -1

@@ -201,7 +201,7 @@ struct semaphore_waiter {
  * constant, and thus optimised away by the compiler.  Likewise the
  * 'timeout' parameter for the cases without timeouts.
  */
-static inline int __sched __down_common(struct semaphore *sem, long state,
+static inline int __sched __down_common(struct semaphore *sem, int state,
 								long timeout)
 {
 	struct semaphore_waiter waiter;

diff -u -p a/freezer.c b/freezer.c

freezer.c +1 -1

@@ -64,7 +64,7 @@ bool __refrigerator(bool check_kthr_stop
 	/* Hmm, should we be allowed to suspend when there are realtime
 	   processes around? */
 	bool was_frozen = false;
-	long save = current->state;
+	int save = current->state;
 
 	pr_debug("%s entered refrigerator\n", current->comm);
 

diff -u -p a/sched/core.c b/sched/core.c

sched/core.c +4 -5

@@ -1888,7 +1888,7 @@ out:
  * smp_call_function() if an IPI is sent by the same process we are
  * waiting to become inactive.
  */
-unsigned long wait_task_inactive(struct task_struct *p, long match_state)
+unsigned long wait_task_inactive(struct task_struct *p, int match_state)
 {
 	int running, queued;
 	struct rq_flags rf;

@@ -3185,7 +3185,7 @@ static struct rq *finish_task_switch(str
 {
 	struct rq *rq = this_rq();
 	struct mm_struct *mm = rq->prev_mm;
-	long prev_state;
+	int prev_state;
 
 	/*
 	 * The previous task will have left us with a preempt_count of 2

@@ -5964,7 +5964,7 @@ void sched_show_task(struct task_struct
 EXPORT_SYMBOL_GPL(sched_show_task);
 
 static inline bool
-state_filter_match(unsigned long state_filter, struct task_struct *p)
+state_filter_match(unsigned int state_filter, struct task_struct *p)
 {
 	/* no filter, everything matches */
 	if (!state_filter)

@@ -5985,7 +5985,7 @@ state_filter_match(unsigned long state_f
 }
 
 
-void show_state_filter(unsigned long state_filter)
+void show_state_filter(unsigned int state_filter)
 {
 	struct task_struct *g, *p;
 
---

Quoted 2 lines

// FIXME: current not recognized as task_struct*, fixhack with regexp identifier current =~ "^current$";

Please don't do this. Just use the word current. It doesn't have to be a metavariable. You will though get a warning about it. To eliminate the warning, you can say symbol current;

Quoted 1 line

identifier task_state =~ "^TASK_";

Are there a lot of options? You can also enumerate them in {}, ie

identifier task_state = {TASK_BLAH, TASK_BLAHBLAH};

Quoted 27 lines

identifier state_var;
position pos;
@@

(
p->state & state_var@pos
|
current->state & state_var@pos
|
p->state | state_var@pos
|
current->state | state_var@pos
|
p->state < state_var@pos
|
current->state < state_var@pos
|
p->state > state_var@pos
|
current->state > state_var@pos
|
state_var@pos = p->state
|
state_var@pos = current->state
|
p->state == state_var@pos
|

Quoted 8 lines

current->state == state_var@pos
|
p->state != state_var@pos
|
current->state != state_var@pos
|
// FIXME: match functions that do something with state_var underneath? // How to do recursive rules?

You want to look at the definitions of called functions? Coccinelle doesn't really support that, but there are hackish ways to add that. How many function calls would you expect have to be unrolled?

Quoted 36 lines

set_current_state(state_var@pos)
|
set_special_state(state_var@pos)
|
signal_pending_state(state_var@pos, p)
|
signal_pending_state(state_var@pos, current)
|
state_var@pos & task_state
|
state_var@pos | task_state
)

// Fixup local variables
@depends on patch && state_access@
identifier state_var = state_access.state_var;
@@
(
- long
+ int
|
- unsigned long
+ unsigned int
)
state_var;

// Fixup function parameters
@depends on patch && state_access@
identifier fn;
identifier state_var = state_access.state_var;
@@

fn(...,
- long state_var
+ int state_var
,...)

Quoted 5 lines

{
	...
}

// FIXME: find a way to squash that with the above?

I think that you can make a disjunction on a function parameter

fn(...,
(
- T1 x1
+ T2 x2
|
- T3 x3
+ T4 x4
)
, ...) { ... }

julia

Quoted 7 lines

Quoted 2 lines

// FIXME: match functions that do something with state_var underneath? // How to do recursive rules?

You want to look at the definitions of called functions? Coccinelle doesn't really support that, but there are hackish ways to add that. How many function calls would you expect have to be unrolled?

I wouldn't expect more than a handful (~5). I suppose this has to do with injecting some Python/Ocaml code? I have some examples bookmarked but haven't gotten to stare at them long enough.

Quoted 31 lines

Quoted 23 lines

// Fixup local variables
@depends on patch && state_access@
identifier state_var = state_access.state_var;
@@
(
- long
+ int
|
- unsigned long
+ unsigned int
)
state_var;

// Fixup function parameters
@depends on patch && state_access@
identifier fn;
identifier state_var = state_access.state_var;
@@

fn(...,
- long state_var
+ int state_var
,...)

Quoted 5 lines

{
	...
}

// FIXME: find a way to squash that with the above?

I think that you can make a disjunction on a function parameter

Quoted 10 lines

fn(...,
(
- T1 x1
+ T2 x2
|
- T3 x3
+ T4 x4
)
, ...) { ... }

My attempt at this gives me "minus: parse error", which is why I went with the split.

Something simple like this works:
---
virtual patch
virtual report

@@
identifier fn;
identifier p;
@@

fn(...,
- long
+ int
p
,...)

{
	...
}

---

but this doesn't:
---
virtual patch
virtual report

@@
identifier fn;
identifier p;
@@

fn(...,
(
- long p
+ int p
|
- unsigned long p
+ unsigned int p
)
,...)

{
	...
}

---

Quoted 2 lines

julia

Quoted 11 lines

Quoted 7 lines

Quoted 2 lines

// FIXME: match functions that do something with state_var underneath? // How to do recursive rules?

You want to look at the definitions of called functions? Coccinelle doesn't really support that, but there are hackish ways to add that. How many function calls would you expect have to be unrolled?

I wouldn't expect more than a handful (~5). I suppose this has to do with injecting some Python/Ocaml code? I have some examples bookmarked but haven't gotten to stare at them long enough.

You can look at iteration.cocci, but it's a bit complex.

You could match definitions of functions that do what you are interested in, then store the names of these functions in a list (python/ocaml), and then look for calls to those functions. Something like

identifier fn : script:ocaml() { in_my_list fn };

Quoted 5 lines

Quoted 1 line

identifier task_state =~ "^TASK_";

Are there a lot of options? You can also enumerate them in {}, ie

identifier task_state = {TASK_BLAH, TASK_BLAHBLAH};

Around a dozen, can be enumerated easily and is indeed probably better than a regexp.

Quoted 2 lines

You want to look at the definitions of called functions? Coccinelle doesn't really support that,

Quoted 1 line

but there are hackish ways to add that.

// FIXME: current not recognized as task_struct*, fixhack with regexp identifier current =~ "^current$";

identifier state_var;
position pos;
@@

(
p->state & state_var@pos
|
current->state & state_var@pos
|

set_current_state(state_var@pos)
|
set_special_state(state_var@pos)

|
signal_pending_state(state_var@pos, p)
|
signal_pending_state(state_var@pos, current)

Sent: 03 September 2019 19:19

Quoted 1 line

How did you come up with this changeset, did you pickaxe for some regexp?

No, manually, backtracking up to the call chain.
Maybe I missed a few places.

From: Alexey Dobriyan

Quoted 1 line

Sent: 03 September 2019 19:19

...

Quoted 4 lines

Quoted 1 line

How did you come up with this changeset, did you pickaxe for some regexp?

No, manually, backtracking up to the call chain.
Maybe I missed a few places.

Renaming the structure field and getting the compiler to find all the uses can help.

David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)