Re: [PATCH 4/5] Expand the INIT_SIGNALS and INIT_SIGHAND macros and remove

[Thomas Gleixner]

On Tue, 2 Jan 2018, David Howells wrote:

> Thomas Gleixner  wrote:
> 
> > >  #define INIT_CPU_TIMERS(s)   
> > > \
> > ...
> > That macro is only used in init_task.c Why not moving it there and get rid
> > of the whole macro maze in the header file?
> 
> The reason I didn't get rid of it is that it's got more than one expansion
> point.  Same for INIT_PREV_CPUTIME and INIT_PID_LINK.

Ah, sorry I missed that. So yes, the current patch is fine.

Acked-by: Thomas Gleixner 

Re: [PATCH 4/5] Expand the INIT_SIGNALS and INIT_SIGHAND macros and remove

[Thomas Gleixner]

On Tue, 2 Jan 2018, David Howells wrote:

> Thomas Gleixner  wrote:
> 
> > >  #define INIT_CPU_TIMERS(s)   
> > > \
> > ...
> > That macro is only used in init_task.c Why not moving it there and get rid
> > of the whole macro maze in the header file?
> 
> The reason I didn't get rid of it is that it's got more than one expansion
> point.  Same for INIT_PREV_CPUTIME and INIT_PID_LINK.

Ah, sorry I missed that. So yes, the current patch is fine.

Acked-by: Thomas Gleixner 

Re: [PATCH 4/5] Expand the INIT_SIGNALS and INIT_SIGHAND macros and remove

[David Howells]

Thomas Gleixner  wrote:

> >  #define INIT_CPU_TIMERS(s) \
> ...
> That macro is only used in init_task.c Why not moving it there and get rid
> of the whole macro maze in the header file?

The reason I didn't get rid of it is that it's got more than one expansion
point.  Same for INIT_PREV_CPUTIME and INIT_PID_LINK.

How about the attached patch?  Or do you think it's going a bit too far?

Possibly I could just move some of the #defines from the .h file to the .c
file, but otherwise leave them unexpanded.

David
---
commit ce4d0010bfc3b83f88322afa3cd9013b9c341c4d
Author: David Howells 
Date:   Tue Jan 2 14:20:17 2018 +

Expand some multi-use INIT_ macros and remove

Expand INIT_CPU_TIMERS, INIT_PREV_CPUTIME and INIT_PID_LINK in place and
remove the definitions.

I'm not entirely sure we want to do this since each of these macros has
multiple points of expansion - and not necessarily in the same structure in
each case.

Suggested-by: Thomas Gleixner 
Signed-off-by: David Howells 

diff --git a/include/linux/init_task.h b/include/linux/init_task.h
index a454b8aeb938..f4d002b329cc 100644
--- a/include/linux/init_task.h
+++ b/include/linux/init_task.h
@@ -27,34 +27,6 @@ extern struct nsproxy init_nsproxy;
 extern struct group_info init_groups;
 extern struct cred init_cred;
 
-#ifndef CONFIG_VIRT_CPU_ACCOUNTING_NATIVE
-#define INIT_PREV_CPUTIME(x)   .prev_cputime = {   \
-   .lock = __RAW_SPIN_LOCK_UNLOCKED(x.prev_cputime.lock),  \
-},
-#else
-#define INIT_PREV_CPUTIME(x)
-#endif
-
-#ifdef CONFIG_POSIX_TIMERS
-#define INIT_CPU_TIMERS(s) \
-   .cpu_timers = { \
-   LIST_HEAD_INIT(s.cpu_timers[0]),\
-   LIST_HEAD_INIT(s.cpu_timers[1]),\
-   LIST_HEAD_INIT(s.cpu_timers[2]),\
-   },
-#else
-#define INIT_CPU_TIMERS(s)
-#endif
-
-#define INIT_PID_LINK(type)\
-{  \
-   .node = {   \
-   .next = NULL,   \
-   .pprev = NULL,  \
-   },  \
-   .pid = _struct_pid,\
-}
-
 #define INIT_TASK_COMM "swapper"
 
 /* Attach to the init_task data structure for proper alignment */
diff --git a/init/init_task.c b/init/init_task.c
index 3ac6e754cf64..136d9896acdb 100644
--- a/init/init_task.c
+++ b/init/init_task.c
@@ -30,9 +30,15 @@ static struct signal_struct init_signals = {
.running= false,
.checking_timer = false,
},
+   .cpu_timers = {
+   LIST_HEAD_INIT(init_signals.cpu_timers[0]),
+   LIST_HEAD_INIT(init_signals.cpu_timers[1]),
+   LIST_HEAD_INIT(init_signals.cpu_timers[2]),
+   },
+#endif
+#ifndef CONFIG_VIRT_CPU_ACCOUNTING_NATIVE
+   .prev_cputime.lock = 
__RAW_SPIN_LOCK_UNLOCKED(init_signals.prev_cputime.lock),
 #endif
-   INIT_CPU_TIMERS(init_signals)
-   INIT_PREV_CPUTIME(init_signals)
 };
 
 static struct sighand_struct init_sighand = {
@@ -107,13 +113,19 @@ struct task_struct init_task
.blocked= {{0}},
.alloc_lock = __SPIN_LOCK_UNLOCKED(init_task.alloc_lock),
.journal_info   = NULL,
-   INIT_CPU_TIMERS(init_task)
+#ifdef CONFIG_POSIX_TIMERS
+   .cpu_timers = {
+   LIST_HEAD_INIT(init_task.cpu_timers[0]),
+   LIST_HEAD_INIT(init_task.cpu_timers[1]),
+   LIST_HEAD_INIT(init_task.cpu_timers[2]),
+   },
+#endif
.pi_lock= __RAW_SPIN_LOCK_UNLOCKED(init_task.pi_lock),
.timer_slack_ns = 5, /* 50 usec default slack */
.pids = {
-   [PIDTYPE_PID]  = INIT_PID_LINK(PIDTYPE_PID),
-   [PIDTYPE_PGID] = INIT_PID_LINK(PIDTYPE_PGID),
-   [PIDTYPE_SID]  = INIT_PID_LINK(PIDTYPE_SID),
+   [PIDTYPE_PID ].pid = _struct_pid,
+   [PIDTYPE_PGID].pid = _struct_pid,
+   [PIDTYPE_SID ].pid = _struct_pid,
},
.thread_group   = LIST_HEAD_INIT(init_task.thread_group),
.thread_node= LIST_HEAD_INIT(init_signals.thread_head),
@@ -143,7 +155,9 @@ struct task_struct init_task
.pi_waiters = RB_ROOT_CACHED,
.pi_top_task= NULL,
 #endif
-   INIT_PREV_CPUTIME(init_task)
+#ifndef CONFIG_VIRT_CPU_ACCOUNTING_NATIVE
+   .prev_cputime.lock = 
__RAW_SPIN_LOCK_UNLOCKED(init_task.prev_cputime.lock),
+#endif
 #ifdef CONFIG_VIRT_CPU_ACCOUNTING_GEN
.vtime.seqcount = 

Re: [PATCH 4/5] Expand the INIT_SIGNALS and INIT_SIGHAND macros and remove

[David Howells]

Thomas Gleixner  wrote:

> >  #define INIT_CPU_TIMERS(s) \
> ...
> That macro is only used in init_task.c Why not moving it there and get rid
> of the whole macro maze in the header file?

The reason I didn't get rid of it is that it's got more than one expansion
point.  Same for INIT_PREV_CPUTIME and INIT_PID_LINK.

How about the attached patch?  Or do you think it's going a bit too far?

Possibly I could just move some of the #defines from the .h file to the .c
file, but otherwise leave them unexpanded.

David
---
commit ce4d0010bfc3b83f88322afa3cd9013b9c341c4d
Author: David Howells 
Date:   Tue Jan 2 14:20:17 2018 +

Expand some multi-use INIT_ macros and remove

Expand INIT_CPU_TIMERS, INIT_PREV_CPUTIME and INIT_PID_LINK in place and
remove the definitions.

I'm not entirely sure we want to do this since each of these macros has
multiple points of expansion - and not necessarily in the same structure in
each case.

Suggested-by: Thomas Gleixner 
Signed-off-by: David Howells 

diff --git a/include/linux/init_task.h b/include/linux/init_task.h
index a454b8aeb938..f4d002b329cc 100644
--- a/include/linux/init_task.h
+++ b/include/linux/init_task.h
@@ -27,34 +27,6 @@ extern struct nsproxy init_nsproxy;
 extern struct group_info init_groups;
 extern struct cred init_cred;
 
-#ifndef CONFIG_VIRT_CPU_ACCOUNTING_NATIVE
-#define INIT_PREV_CPUTIME(x)   .prev_cputime = {   \
-   .lock = __RAW_SPIN_LOCK_UNLOCKED(x.prev_cputime.lock),  \
-},
-#else
-#define INIT_PREV_CPUTIME(x)
-#endif
-
-#ifdef CONFIG_POSIX_TIMERS
-#define INIT_CPU_TIMERS(s) \
-   .cpu_timers = { \
-   LIST_HEAD_INIT(s.cpu_timers[0]),\
-   LIST_HEAD_INIT(s.cpu_timers[1]),\
-   LIST_HEAD_INIT(s.cpu_timers[2]),\
-   },
-#else
-#define INIT_CPU_TIMERS(s)
-#endif
-
-#define INIT_PID_LINK(type)\
-{  \
-   .node = {   \
-   .next = NULL,   \
-   .pprev = NULL,  \
-   },  \
-   .pid = _struct_pid,\
-}
-
 #define INIT_TASK_COMM "swapper"
 
 /* Attach to the init_task data structure for proper alignment */
diff --git a/init/init_task.c b/init/init_task.c
index 3ac6e754cf64..136d9896acdb 100644
--- a/init/init_task.c
+++ b/init/init_task.c
@@ -30,9 +30,15 @@ static struct signal_struct init_signals = {
.running= false,
.checking_timer = false,
},
+   .cpu_timers = {
+   LIST_HEAD_INIT(init_signals.cpu_timers[0]),
+   LIST_HEAD_INIT(init_signals.cpu_timers[1]),
+   LIST_HEAD_INIT(init_signals.cpu_timers[2]),
+   },
+#endif
+#ifndef CONFIG_VIRT_CPU_ACCOUNTING_NATIVE
+   .prev_cputime.lock = 
__RAW_SPIN_LOCK_UNLOCKED(init_signals.prev_cputime.lock),
 #endif
-   INIT_CPU_TIMERS(init_signals)
-   INIT_PREV_CPUTIME(init_signals)
 };
 
 static struct sighand_struct init_sighand = {
@@ -107,13 +113,19 @@ struct task_struct init_task
.blocked= {{0}},
.alloc_lock = __SPIN_LOCK_UNLOCKED(init_task.alloc_lock),
.journal_info   = NULL,
-   INIT_CPU_TIMERS(init_task)
+#ifdef CONFIG_POSIX_TIMERS
+   .cpu_timers = {
+   LIST_HEAD_INIT(init_task.cpu_timers[0]),
+   LIST_HEAD_INIT(init_task.cpu_timers[1]),
+   LIST_HEAD_INIT(init_task.cpu_timers[2]),
+   },
+#endif
.pi_lock= __RAW_SPIN_LOCK_UNLOCKED(init_task.pi_lock),
.timer_slack_ns = 5, /* 50 usec default slack */
.pids = {
-   [PIDTYPE_PID]  = INIT_PID_LINK(PIDTYPE_PID),
-   [PIDTYPE_PGID] = INIT_PID_LINK(PIDTYPE_PGID),
-   [PIDTYPE_SID]  = INIT_PID_LINK(PIDTYPE_SID),
+   [PIDTYPE_PID ].pid = _struct_pid,
+   [PIDTYPE_PGID].pid = _struct_pid,
+   [PIDTYPE_SID ].pid = _struct_pid,
},
.thread_group   = LIST_HEAD_INIT(init_task.thread_group),
.thread_node= LIST_HEAD_INIT(init_signals.thread_head),
@@ -143,7 +155,9 @@ struct task_struct init_task
.pi_waiters = RB_ROOT_CACHED,
.pi_top_task= NULL,
 #endif
-   INIT_PREV_CPUTIME(init_task)
+#ifndef CONFIG_VIRT_CPU_ACCOUNTING_NATIVE
+   .prev_cputime.lock = 
__RAW_SPIN_LOCK_UNLOCKED(init_task.prev_cputime.lock),
+#endif
 #ifdef CONFIG_VIRT_CPU_ACCOUNTING_GEN
.vtime.seqcount = SEQCNT_ZERO(init_task.vtime_seqcount),
.vtime.starttime = 0,

Re: [PATCH 4/5] Expand the INIT_SIGNALS and INIT_SIGHAND macros and remove

[Thomas Gleixner]

On Fri, 8 Dec 2017, David Howells wrote:
>  #define INIT_CPU_TIMERS(s)   \
>   .cpu_timers = { \
>   LIST_HEAD_INIT(s.cpu_timers[0]),\
>   LIST_HEAD_INIT(s.cpu_timers[1]),\

That macro is only used in init_task.c Why not moving it there and get rid
of the whole macro maze in the header file?

Thanks,

tglx

Re: [PATCH 4/5] Expand the INIT_SIGNALS and INIT_SIGHAND macros and remove

[Thomas Gleixner]

On Fri, 8 Dec 2017, David Howells wrote:
>  #define INIT_CPU_TIMERS(s)   \
>   .cpu_timers = { \
>   LIST_HEAD_INIT(s.cpu_timers[0]),\
>   LIST_HEAD_INIT(s.cpu_timers[1]),\

That macro is only used in init_task.c Why not moving it there and get rid
of the whole macro maze in the header file?

Thanks,

tglx

[PATCH 4/5] Expand the INIT_SIGNALS and INIT_SIGHAND macros and remove

[David Howells]

There doesn't seem to be any need to have the INIT_SIGNALS and INIT_SIGHAND
macros, so expand them in their single places of use and remove them.

Signed-off-by: David Howells 
---

 include/linux/init_task.h |   43 ---
 init/init_task.c  |   30 --
 2 files changed, 32 insertions(+), 41 deletions(-)

diff --git a/include/linux/init_task.h b/include/linux/init_task.h
index b1385e1dca63..5b5f41328115 100644
--- a/include/linux/init_task.h
+++ b/include/linux/init_task.h
@@ -23,6 +23,9 @@
 
 extern struct files_struct init_files;
 extern struct fs_struct init_fs;
+extern struct nsproxy init_nsproxy;
+extern struct group_info init_groups;
+extern struct cred init_cred;
 
 #ifndef CONFIG_VIRT_CPU_ACCOUNTING_NATIVE
 #define INIT_PREV_CPUTIME(x)   .prev_cputime = {   \
@@ -33,52 +36,16 @@ extern struct fs_struct init_fs;
 #endif
 
 #ifdef CONFIG_POSIX_TIMERS
-#define INIT_POSIX_TIMERS(s)   \
-   .posix_timers = LIST_HEAD_INIT(s.posix_timers),
 #define INIT_CPU_TIMERS(s) \
.cpu_timers = { \
LIST_HEAD_INIT(s.cpu_timers[0]),\
LIST_HEAD_INIT(s.cpu_timers[1]),\
-   LIST_HEAD_INIT(s.cpu_timers[2]),
\
-   },
-#define INIT_CPUTIMER(s)   \
-   .cputimer   = { \
-   .cputime_atomic = INIT_CPUTIME_ATOMIC,  \
-   .running= false,\
-   .checking_timer = false,\
+   LIST_HEAD_INIT(s.cpu_timers[2]),\
},
 #else
-#define INIT_POSIX_TIMERS(s)
 #define INIT_CPU_TIMERS(s)
-#define INIT_CPUTIMER(s)
 #endif
 
-#define INIT_SIGNALS(sig) {\
-   .nr_threads = 1,\
-   .thread_head= LIST_HEAD_INIT(init_task.thread_node),\
-   .wait_chldexit  = __WAIT_QUEUE_HEAD_INITIALIZER(sig.wait_chldexit),\
-   .shared_pending = { \
-   .list = LIST_HEAD_INIT(sig.shared_pending.list),\
-   .signal =  {{0}}},  \
-   INIT_POSIX_TIMERS(sig)  \
-   INIT_CPU_TIMERS(sig)\
-   .rlim   = INIT_RLIMITS, \
-   INIT_CPUTIMER(sig)  \
-   INIT_PREV_CPUTIME(sig)  \
-   .cred_guard_mutex = \
-__MUTEX_INITIALIZER(sig.cred_guard_mutex), \
-}
-
-extern struct nsproxy init_nsproxy;
-
-#define INIT_SIGHAND(sighand) {
\
-   .count  = ATOMIC_INIT(1),   \
-   .action = { { { .sa_handler = SIG_DFL, } }, },  \
-   .siglock= __SPIN_LOCK_UNLOCKED(sighand.siglock),\
-   .signalfd_wqh   = __WAIT_QUEUE_HEAD_INITIALIZER(sighand.signalfd_wqh),  
\
-}
-
-extern struct group_info init_groups;
 
 #define INIT_STRUCT_PID {  \
.count  = ATOMIC_INIT(1),   \
@@ -103,8 +70,6 @@ extern struct group_info init_groups;
.pid = _struct_pid,\
 }
 
-extern struct cred init_cred;
-
 #define INIT_TASK_COMM "swapper"
 
 /* Attach to the init_task data structure for proper alignment */
diff --git a/init/init_task.c b/init/init_task.c
index aa4030a939e5..3ac6e754cf64 100644
--- a/init/init_task.c
+++ b/init/init_task.c
@@ -13,8 +13,34 @@
 #include 
 #include 
 
-static struct signal_struct init_signals = INIT_SIGNALS(init_signals);
-static struct sighand_struct init_sighand = INIT_SIGHAND(init_sighand);
+static struct signal_struct init_signals = {
+   .nr_threads = 1,
+   .thread_head= LIST_HEAD_INIT(init_task.thread_node),
+   .wait_chldexit  = 
__WAIT_QUEUE_HEAD_INITIALIZER(init_signals.wait_chldexit),
+   .shared_pending = {
+   .list = LIST_HEAD_INIT(init_signals.shared_pending.list),
+   .signal =  {{0}}
+   },
+   .rlim   = INIT_RLIMITS,
+   .cred_guard_mutex = __MUTEX_INITIALIZER(init_signals.cred_guard_mutex),
+#ifdef CONFIG_POSIX_TIMERS
+   .posix_timers = LIST_HEAD_INIT(init_signals.posix_timers),
+   .cputimer   = {
+   .cputime_atomic = 

[PATCH 4/5] Expand the INIT_SIGNALS and INIT_SIGHAND macros and remove

[David Howells]

There doesn't seem to be any need to have the INIT_SIGNALS and INIT_SIGHAND
macros, so expand them in their single places of use and remove them.

Signed-off-by: David Howells 
---

 include/linux/init_task.h |   43 ---
 init/init_task.c  |   30 --
 2 files changed, 32 insertions(+), 41 deletions(-)

diff --git a/include/linux/init_task.h b/include/linux/init_task.h
index b1385e1dca63..5b5f41328115 100644
--- a/include/linux/init_task.h
+++ b/include/linux/init_task.h
@@ -23,6 +23,9 @@
 
 extern struct files_struct init_files;
 extern struct fs_struct init_fs;
+extern struct nsproxy init_nsproxy;
+extern struct group_info init_groups;
+extern struct cred init_cred;
 
 #ifndef CONFIG_VIRT_CPU_ACCOUNTING_NATIVE
 #define INIT_PREV_CPUTIME(x)   .prev_cputime = {   \
@@ -33,52 +36,16 @@ extern struct fs_struct init_fs;
 #endif
 
 #ifdef CONFIG_POSIX_TIMERS
-#define INIT_POSIX_TIMERS(s)   \
-   .posix_timers = LIST_HEAD_INIT(s.posix_timers),
 #define INIT_CPU_TIMERS(s) \
.cpu_timers = { \
LIST_HEAD_INIT(s.cpu_timers[0]),\
LIST_HEAD_INIT(s.cpu_timers[1]),\
-   LIST_HEAD_INIT(s.cpu_timers[2]),
\
-   },
-#define INIT_CPUTIMER(s)   \
-   .cputimer   = { \
-   .cputime_atomic = INIT_CPUTIME_ATOMIC,  \
-   .running= false,\
-   .checking_timer = false,\
+   LIST_HEAD_INIT(s.cpu_timers[2]),\
},
 #else
-#define INIT_POSIX_TIMERS(s)
 #define INIT_CPU_TIMERS(s)
-#define INIT_CPUTIMER(s)
 #endif
 
-#define INIT_SIGNALS(sig) {\
-   .nr_threads = 1,\
-   .thread_head= LIST_HEAD_INIT(init_task.thread_node),\
-   .wait_chldexit  = __WAIT_QUEUE_HEAD_INITIALIZER(sig.wait_chldexit),\
-   .shared_pending = { \
-   .list = LIST_HEAD_INIT(sig.shared_pending.list),\
-   .signal =  {{0}}},  \
-   INIT_POSIX_TIMERS(sig)  \
-   INIT_CPU_TIMERS(sig)\
-   .rlim   = INIT_RLIMITS, \
-   INIT_CPUTIMER(sig)  \
-   INIT_PREV_CPUTIME(sig)  \
-   .cred_guard_mutex = \
-__MUTEX_INITIALIZER(sig.cred_guard_mutex), \
-}
-
-extern struct nsproxy init_nsproxy;
-
-#define INIT_SIGHAND(sighand) {
\
-   .count  = ATOMIC_INIT(1),   \
-   .action = { { { .sa_handler = SIG_DFL, } }, },  \
-   .siglock= __SPIN_LOCK_UNLOCKED(sighand.siglock),\
-   .signalfd_wqh   = __WAIT_QUEUE_HEAD_INITIALIZER(sighand.signalfd_wqh),  
\
-}
-
-extern struct group_info init_groups;
 
 #define INIT_STRUCT_PID {  \
.count  = ATOMIC_INIT(1),   \
@@ -103,8 +70,6 @@ extern struct group_info init_groups;
.pid = _struct_pid,\
 }
 
-extern struct cred init_cred;
-
 #define INIT_TASK_COMM "swapper"
 
 /* Attach to the init_task data structure for proper alignment */
diff --git a/init/init_task.c b/init/init_task.c
index aa4030a939e5..3ac6e754cf64 100644
--- a/init/init_task.c
+++ b/init/init_task.c
@@ -13,8 +13,34 @@
 #include 
 #include 
 
-static struct signal_struct init_signals = INIT_SIGNALS(init_signals);
-static struct sighand_struct init_sighand = INIT_SIGHAND(init_sighand);
+static struct signal_struct init_signals = {
+   .nr_threads = 1,
+   .thread_head= LIST_HEAD_INIT(init_task.thread_node),
+   .wait_chldexit  = 
__WAIT_QUEUE_HEAD_INITIALIZER(init_signals.wait_chldexit),
+   .shared_pending = {
+   .list = LIST_HEAD_INIT(init_signals.shared_pending.list),
+   .signal =  {{0}}
+   },
+   .rlim   = INIT_RLIMITS,
+   .cred_guard_mutex = __MUTEX_INITIALIZER(init_signals.cred_guard_mutex),
+#ifdef CONFIG_POSIX_TIMERS
+   .posix_timers = LIST_HEAD_INIT(init_signals.posix_timers),
+   .cputimer   = {
+   .cputime_atomic = INIT_CPUTIME_ATOMIC,
+