[v2] sched: Use struct_size() helper in task_numa_group()

[PATCH -next, v2] sched: Use struct_size() helper in task_numa_group()

Posted by Xiu Jianfeng 4 years, 5 months ago

Make use of struct_size() helper instead of an open-coded calculation.
There is no functional change in this patch.

Link: https://github.com/KSPP/linux/issues/160
Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
---
 kernel/sched/fair.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 095b0aa378df..af933a7f9e5d 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -2437,9 +2437,8 @@ static void task_numa_group(struct task_struct *p, int cpupid, int flags,
 	int i;
 
 	if (unlikely(!deref_curr_numa_group(p))) {
-		unsigned int size = sizeof(struct numa_group) +
-				    NR_NUMA_HINT_FAULT_STATS *
-				    nr_node_ids * sizeof(unsigned long);
+		unsigned int size = struct_size(grp, faults,
+						NR_NUMA_HINT_FAULT_STATS * nr_node_ids);
 
 		grp = kzalloc(size, GFP_KERNEL | __GFP_NOWARN);
 		if (!grp)
-- 
2.17.1

Re: [PATCH -next, v2] sched: Use struct_size() helper in task_numa_group()

Posted by Steven Rostedt 4 years, 5 months ago

On Mon, 10 Jan 2022 09:23:54 +0800
Xiu Jianfeng <xiujianfeng@huawei.com> wrote:

> Make use of struct_size() helper instead of an open-coded calculation.
> There is no functional change in this patch.

Reviewed-by: Steven Rostedt <rostedt@goodmis.org>

-- Steve

> 
> Link: https://github.com/KSPP/linux/issues/160
> Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
> ---
>  kernel/sched/fair.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 095b0aa378df..af933a7f9e5d 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -2437,9 +2437,8 @@ static void task_numa_group(struct task_struct *p, int cpupid, int flags,
>  	int i;
>  
>  	if (unlikely(!deref_curr_numa_group(p))) {
> -		unsigned int size = sizeof(struct numa_group) +
> -				    NR_NUMA_HINT_FAULT_STATS *
> -				    nr_node_ids * sizeof(unsigned long);
> +		unsigned int size = struct_size(grp, faults,
> +						NR_NUMA_HINT_FAULT_STATS * nr_node_ids);
>  
>  		grp = kzalloc(size, GFP_KERNEL | __GFP_NOWARN);
>  		if (!grp)

Re: [PATCH -next, v2] sched: Use struct_size() helper in task_numa_group()

Posted by Peter Zijlstra 4 years, 5 months ago

On Mon, Jan 10, 2022 at 09:23:54AM +0800, Xiu Jianfeng wrote:
> Make use of struct_size() helper instead of an open-coded calculation.
> There is no functional change in this patch.
> 
> Link: https://github.com/KSPP/linux/issues/160
> Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
> ---
>  kernel/sched/fair.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 095b0aa378df..af933a7f9e5d 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -2437,9 +2437,8 @@ static void task_numa_group(struct task_struct *p, int cpupid, int flags,
>  	int i;
>  
>  	if (unlikely(!deref_curr_numa_group(p))) {
> -		unsigned int size = sizeof(struct numa_group) +
> -				    NR_NUMA_HINT_FAULT_STATS *
> -				    nr_node_ids * sizeof(unsigned long);
> +		unsigned int size = struct_size(grp, faults,
> +						NR_NUMA_HINT_FAULT_STATS * nr_node_ids);

Again, why?! The old code was perfectly readable, this, not so much.

Re: [PATCH -next, v2] sched: Use struct_size() helper in task_numa_group()

Posted by Steven Rostedt 4 years, 5 months ago

On Mon, 10 Jan 2022 23:46:15 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> On Mon, Jan 10, 2022 at 09:23:54AM +0800, Xiu Jianfeng wrote:
> > Make use of struct_size() helper instead of an open-coded calculation.
> > There is no functional change in this patch.
> > 
> > Link: https://github.com/KSPP/linux/issues/160
> > Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
> > ---
> >  kernel/sched/fair.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index 095b0aa378df..af933a7f9e5d 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -2437,9 +2437,8 @@ static void task_numa_group(struct task_struct *p, int cpupid, int flags,
> >  	int i;
> >  
> >  	if (unlikely(!deref_curr_numa_group(p))) {
> > -		unsigned int size = sizeof(struct numa_group) +
> > -				    NR_NUMA_HINT_FAULT_STATS *
> > -				    nr_node_ids * sizeof(unsigned long);
> > +		unsigned int size = struct_size(grp, faults,
> > +						NR_NUMA_HINT_FAULT_STATS * nr_node_ids);  
> 
> Again, why?! The old code was perfectly readable, this, not so much.

Because it is unsafe, and there is an effort to get rid of all open coded
struct_size() code. Linus has told me to do the same with my code.

  https://lore.kernel.org/all/CAHk-=wiGWjxs7EVUpccZEi6esvjpHJdgHQ=vtUeJ5crL62hx9A@mail.gmail.com/

And to be honest, the new change is a lot easier to read than the original
code.

struct_size() lets you know the field "faults" and the number of elements.
You don't need to know the size of "faults". Whereas the original code,
how is that readable? From that code, how do you know what the
sizeof(unsigned long) is for?

-- Steve

Re: [PATCH -next, v2] sched: Use struct_size() helper in task_numa_group()

Posted by Gustavo A. R. Silva 4 years, 5 months ago

On Mon, Jan 10, 2022 at 07:31:58PM -0500, Steven Rostedt wrote:
> On Mon, 10 Jan 2022 23:46:15 +0100
> Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > On Mon, Jan 10, 2022 at 09:23:54AM +0800, Xiu Jianfeng wrote:
> > > Make use of struct_size() helper instead of an open-coded calculation.
> > > There is no functional change in this patch.
> > > 
> > > Link: https://github.com/KSPP/linux/issues/160
> > > Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
> > > ---
> > >  kernel/sched/fair.c | 5 ++---
> > >  1 file changed, 2 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > > index 095b0aa378df..af933a7f9e5d 100644
> > > --- a/kernel/sched/fair.c
> > > +++ b/kernel/sched/fair.c
> > > @@ -2437,9 +2437,8 @@ static void task_numa_group(struct task_struct *p, int cpupid, int flags,
> > >  	int i;
> > >  
> > >  	if (unlikely(!deref_curr_numa_group(p))) {
> > > -		unsigned int size = sizeof(struct numa_group) +
> > > -				    NR_NUMA_HINT_FAULT_STATS *
> > > -				    nr_node_ids * sizeof(unsigned long);
> > > +		unsigned int size = struct_size(grp, faults,
> > > +						NR_NUMA_HINT_FAULT_STATS * nr_node_ids);  
> > 
> > Again, why?! The old code was perfectly readable, this, not so much.
> 
> Because it is unsafe, and there is an effort to get rid of all open coded
> struct_size() code. Linus has told me to do the same with my code.
> 
>   https://lore.kernel.org/all/CAHk-=wiGWjxs7EVUpccZEi6esvjpHJdgHQ=vtUeJ5crL62hx9A@mail.gmail.com/
> 
> And to be honest, the new change is a lot easier to read than the original
> code.

I agree.

Also, I was taking a look at the thread above and noticed the sparse
warning doesn't go away. However, the change is correct. :)

gustavo@beefy:~/git/linux$ grep 'using sizeof on a flexible structure' next-20220110.out | grep kernel/trace/trace.c
kernel/trace/trace.c:1009:17: warning: using sizeof on a flexible structure
kernel/trace/trace.c:2660:17: warning: using sizeof on a flexible structure
kernel/trace/trace.c:2770:51: warning: using sizeof on a flexible structure
kernel/trace/trace.c:3358:16: warning: using sizeof on a flexible structure
kernel/trace/trace.c:3418:16: warning: using sizeof on a flexible structure
kernel/trace/trace.c:7082:16: warning: using sizeof on a flexible structure
kernel/trace/trace.c:7160:16: warning: using sizeof on a flexible structure
gustavo@beefy:~/git/linux$ grep -nw struct_size kernel/trace/trace.c
2770:			int max_len = PAGE_SIZE - struct_size(entry, array, 1);

Thanks
--
Gustavo

> 
> struct_size() lets you know the field "faults" and the number of elements.
> You don't need to know the size of "faults". Whereas the original code,
> how is that readable? From that code, how do you know what the
> sizeof(unsigned long) is for?
> 
> -- Steve