| Message ID | 20240621102750.oH9p1FQq@linutronix.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | sched/task_struct: Move alloc_tag to the end of the struct. | expand |
On Fri, Jun 21, 2024 at 12:27:50PM +0200, Sebastian Andrzej Siewior wrote: > The alloc_tag member has been added to task_struct at the very > beginning. This is a pointer and on 64bit architectures it forces 4 byte > padding after `ptrace' and then forcing another another 4 byte padding > after `on_cpu'. A few members later, `se' requires a cacheline aligned > due to struct sched_avg resulting in 52 hole before `se'. > > This is the case on 64bit-SMP architectures. > The 52 byte hole can be avoided by moving alloc_tag away where it > currently resides. > > Move alloc_tag to the end of task_struct. There is likely a hole before > `thread' due to its alignment requirement and the previous members are > likely to be already pointer-aligned. We sure we want it at the end? we do want it on a hot cacheline
On 2024-06-21 10:20:58 [-0400], Kent Overstreet wrote: > On Fri, Jun 21, 2024 at 12:27:50PM +0200, Sebastian Andrzej Siewior wrote: > > The alloc_tag member has been added to task_struct at the very > > beginning. This is a pointer and on 64bit architectures it forces 4 byte > > padding after `ptrace' and then forcing another another 4 byte padding > > after `on_cpu'. A few members later, `se' requires a cacheline aligned > > due to struct sched_avg resulting in 52 hole before `se'. > > > > This is the case on 64bit-SMP architectures. > > The 52 byte hole can be avoided by moving alloc_tag away where it > > currently resides. > > > > Move alloc_tag to the end of task_struct. There is likely a hole before > > `thread' due to its alignment requirement and the previous members are > > likely to be already pointer-aligned. > > We sure we want it at the end? we do want it on a hot cacheline Well, the front is bad. Looking at pgalloc_tag_add() and its callers, there is no task_struct touching. alloc_tag_save()/restore might be the critical one. This is random code… Puh. So if the end is too cold, what about around the mm pointer? Other suggestions? Sebastian
On Fri, Jun 21, 2024 at 08:29:15PM +0200, Sebastian Andrzej Siewior wrote: > On 2024-06-21 10:20:58 [-0400], Kent Overstreet wrote: > > On Fri, Jun 21, 2024 at 12:27:50PM +0200, Sebastian Andrzej Siewior wrote: > > > The alloc_tag member has been added to task_struct at the very > > > beginning. This is a pointer and on 64bit architectures it forces 4 byte > > > padding after `ptrace' and then forcing another another 4 byte padding > > > after `on_cpu'. A few members later, `se' requires a cacheline aligned > > > due to struct sched_avg resulting in 52 hole before `se'. > > > > > > This is the case on 64bit-SMP architectures. > > > The 52 byte hole can be avoided by moving alloc_tag away where it > > > currently resides. > > > > > > Move alloc_tag to the end of task_struct. There is likely a hole before > > > `thread' due to its alignment requirement and the previous members are > > > likely to be already pointer-aligned. > > > > We sure we want it at the end? we do want it on a hot cacheline > > Well, the front is bad. > Looking at pgalloc_tag_add() and its callers, there is no task_struct > touching. alloc_tag_save()/restore might be the critical one. This is > random code… Puh. So if the end is too cold, what about around the mm > pointer? Not there, that's not actually that hot. It needs to be by task_struct->flags; that's used in the same paths.
On 2024-06-21 14:49:23 [-0400], Kent Overstreet wrote: > On Fri, Jun 21, 2024 at 08:29:15PM +0200, Sebastian Andrzej Siewior wrote: > > On 2024-06-21 10:20:58 [-0400], Kent Overstreet wrote: > > > On Fri, Jun 21, 2024 at 12:27:50PM +0200, Sebastian Andrzej Siewior wrote: > > > > The alloc_tag member has been added to task_struct at the very > > > > beginning. This is a pointer and on 64bit architectures it forces 4 byte > > > > padding after `ptrace' and then forcing another another 4 byte padding > > > > after `on_cpu'. A few members later, `se' requires a cacheline aligned > > > > due to struct sched_avg resulting in 52 hole before `se'. > > > > > > > > This is the case on 64bit-SMP architectures. > > > > The 52 byte hole can be avoided by moving alloc_tag away where it > > > > currently resides. > > > > > > > > Move alloc_tag to the end of task_struct. There is likely a hole before > > > > `thread' due to its alignment requirement and the previous members are > > > > likely to be already pointer-aligned. > > > > > > We sure we want it at the end? we do want it on a hot cacheline > > > > Well, the front is bad. > > Looking at pgalloc_tag_add() and its callers, there is no task_struct > > touching. alloc_tag_save()/restore might be the critical one. This is > > random code… Puh. So if the end is too cold, what about around the mm > > pointer? > > Not there, that's not actually that hot. It needs to be by > task_struct->flags; that's used in the same paths. But there is no space without the additional 52 bytes. Was this by any chance on purpose? Sebastian
On Fri, Jun 21, 2024 at 09:07:19PM +0200, Sebastian Andrzej Siewior wrote: > On 2024-06-21 14:49:23 [-0400], Kent Overstreet wrote: > > On Fri, Jun 21, 2024 at 08:29:15PM +0200, Sebastian Andrzej Siewior wrote: > > > On 2024-06-21 10:20:58 [-0400], Kent Overstreet wrote: > > > > On Fri, Jun 21, 2024 at 12:27:50PM +0200, Sebastian Andrzej Siewior wrote: > > > > > The alloc_tag member has been added to task_struct at the very > > > > > beginning. This is a pointer and on 64bit architectures it forces 4 byte > > > > > padding after `ptrace' and then forcing another another 4 byte padding > > > > > after `on_cpu'. A few members later, `se' requires a cacheline aligned > > > > > due to struct sched_avg resulting in 52 hole before `se'. > > > > > > > > > > This is the case on 64bit-SMP architectures. > > > > > The 52 byte hole can be avoided by moving alloc_tag away where it > > > > > currently resides. > > > > > > > > > > Move alloc_tag to the end of task_struct. There is likely a hole before > > > > > `thread' due to its alignment requirement and the previous members are > > > > > likely to be already pointer-aligned. > > > > > > > > We sure we want it at the end? we do want it on a hot cacheline > > > > > > Well, the front is bad. > > > Looking at pgalloc_tag_add() and its callers, there is no task_struct > > > touching. alloc_tag_save()/restore might be the critical one. This is > > > random code… Puh. So if the end is too cold, what about around the mm > > > pointer? > > > > Not there, that's not actually that hot. It needs to be by > > task_struct->flags; that's used in the same paths. > > But there is no space without the additional 52 bytes. Was this by any > chance on purpose? No, that wasn't, and it doesn't have to be the exact same cacheline, but we do want it near current->flags; we store PF_MEMALLOC flags there that are converted to gfp flags and used exactly where we're using current->alloc_tag.
On 2024-06-21 15:13:19 [-0400], Kent Overstreet wrote: > > > > random code… Puh. So if the end is too cold, what about around the mm > > > > pointer? > > > > > > Not there, that's not actually that hot. It needs to be by > > > task_struct->flags; that's used in the same paths. > > > > But there is no space without the additional 52 bytes. Was this by any > > chance on purpose? > > No, that wasn't, and it doesn't have to be the exact same cacheline, but > we do want it near current->flags; we store PF_MEMALLOC flags there that > are converted to gfp flags and used exactly where we're using > current->alloc_tag. Hmm. `stack' and `usage' are the only two member that you would have to move (away) in order the stash the conditional variable there. The `ptrace' one uses the same flags as `flags' so it wouldn't make sense to move that one. Sebastian
diff --git a/include/linux/sched.h b/include/linux/sched.h index 61591ac6eab6d..d76c61510ef1d 100644 --- a/include/linux/sched.h +++ b/include/linux/sched.h @@ -770,10 +770,6 @@ struct task_struct { unsigned int flags; unsigned int ptrace; -#ifdef CONFIG_MEM_ALLOC_PROFILING - struct alloc_tag *alloc_tag; -#endif - #ifdef CONFIG_SMP int on_cpu; struct __call_single_node wake_entry; @@ -1553,6 +1549,9 @@ struct task_struct { #ifdef CONFIG_USER_EVENTS struct user_event_mm *user_event_mm; #endif +#ifdef CONFIG_MEM_ALLOC_PROFILING + struct alloc_tag *alloc_tag; +#endif /* * New fields for task_struct should be added above here, so that
The alloc_tag member has been added to task_struct at the very beginning. This is a pointer and on 64bit architectures it forces 4 byte padding after `ptrace' and then forcing another another 4 byte padding after `on_cpu'. A few members later, `se' requires a cacheline aligned due to struct sched_avg resulting in 52 hole before `se'. This is the case on 64bit-SMP architectures. The 52 byte hole can be avoided by moving alloc_tag away where it currently resides. Move alloc_tag to the end of task_struct. There is likely a hole before `thread' due to its alignment requirement and the previous members are likely to be already pointer-aligned. Fixes: 22d407b164ff7 ("lib: add allocation tagging support for memory allocation profiling") Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> --- include/linux/sched.h | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)