Date:   Tue, 11 Jan 2022 12:30:42 +0100
From:   Peter Zijlstra <peterz@infradead.org>
To:     Steven Rostedt <rostedt@goodmis.org>
Cc:     Xiu Jianfeng <xiujianfeng@huawei.com>, mingo@redhat.com,
        juri.lelli@redhat.com, vincent.guittot@linaro.org,
        dietmar.eggemann@arm.com, bsegall@google.com, mgorman@suse.de,
        bristot@redhat.com, gustavoars@kernel.org,
        linux-kernel@vger.kernel.org, linux-hardening@vger.kernel.org,
        Linus Torvalds <torvalds@linux-foundation.org>
Subject: Re: [PATCH -next, v2] sched: Use struct_size() helper in
 task_numa_group()
Message-ID: <Yd1qYsFcgcp/uHSa@hirez.programming.kicks-ass.net>
References: <20220110012354.144394-1-xiujianfeng@huawei.com>
 <Ydy3N577YD0JJr2N@hirez.programming.kicks-ass.net>
 <20220110193158.31e1eaea@gandalf.local.home>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220110193158.31e1eaea@gandalf.local.home>
Precedence: bulk

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,

Unsafe how? Changelog doesn't mention anything, nor do you. In fact,
Changelog says there is no functional change, which makes me hate the
thing for obscuring something that was simple.

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

I find it the other way around, because now I need to find and untangle
the unholy mess that is struct_size(), whereas currently it is trivial
C.