| Message ID | 20220110012354.144394-1-xiujianfeng@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [-next,v2] sched: Use struct_size() helper in task_numa_group() | expand |
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)
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.
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
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
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.
On Tue, 11 Jan 2022 12:30:42 +0100 Peter Zijlstra <peterz@infradead.org> wrote: > > > > 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. If for some reason faults changes in size, the original code must be updated whereas the new code is robust enough to not need changing. > > > 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. It's a C hack and far from trivial. Maybe to you as you are use to these hacks. But seriously, this is not something the average C coder is use to, as variable length structures are rather unique to the kernel. Note that struct_size() is commonly used in the kernel. Better start getting use to it ;-) -- Steve
On Tue, Jan 11, 2022 at 10:14:25AM -0500, Steven Rostedt wrote: > On Tue, 11 Jan 2022 12:30:42 +0100 > Peter Zijlstra <peterz@infradead.org> wrote: > > > > > > 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. > > If for some reason faults changes in size, the original code must be > updated whereas the new code is robust enough to not need changing. Then I would still much prefer something like: unsigned int size = sizeof(*grp) + NR_NUMA_HINT_FAULT_STATS * numa_node_ids * sizeof(gfp->faults); Which is still far more readable than some obscure macro. But again, the Changelog doesn't mention any actual benefit of the patch and makes the code less clear. > It's a C hack and far from trivial. Maybe to you as you are use to > these hacks. But seriously, this is not something the average C coder > is use to, as variable length structures are rather unique to the > kernel. That's just not true, I've used them in userspace too (even before I started tinkering with the kernel). I've even used this pattern in other languages. It is a fairly useful and common pattern to have a small structure and an array in the same memory allocation. Think hash-tables, the structure contains the size of the table and some other things, like for example a seed for the hash function or a lock, and then the table itself as an array. I can't, nor do I want to, remember all these stupid little macros. Esp. not for trivial things like this.
On Thu, Jan 13, 2022 at 10:18:57AM +0100, Peter Zijlstra wrote: > On Tue, Jan 11, 2022 at 10:14:25AM -0500, Steven Rostedt wrote: > > On Tue, 11 Jan 2022 12:30:42 +0100 > > Peter Zijlstra <peterz@infradead.org> wrote: > > > > > > > > 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. > > > > If for some reason faults changes in size, the original code must be > > updated whereas the new code is robust enough to not need changing. I think this alone is reason enough. :) > Then I would still much prefer something like: > > unsigned int size = sizeof(*grp) + > NR_NUMA_HINT_FAULT_STATS * numa_node_ids * sizeof(gfp->faults); > > Which is still far more readable than some obscure macro. But again, the I'm not sure it's _obscure_, but it is relatively new. It's even documented. ;) https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments That said, the original patch is incomplete: it should be using size_t for "size". > It is a fairly useful and common pattern to have a small structure and > an array in the same memory allocation. > > Think hash-tables, the structure contains the size of the table and some > other things, like for example a seed for the hash function or a lock, > and then the table itself as an array. Right, the use of flexible arrays is very common in the kernel. So much so that we've spent years fixing all the ancient "fake flexible arrays" scattered around the kernel messing up all kinds of compile-time and run-time flaw mitigations. Flexible array manipulations are notoriously prone to mistakes (overflows in allocation, mismatched bounds storage sizes, array index overflows, etc). These helpers (with more to come) help remove some of the foot-guns that C would normally impart to them. > I can't, nor do I want to, remember all these stupid little macros. Esp. > not for trivial things like this. Well, the good news is that other folks will (and are) fixing them for you. :) Even if you never make mistakes with flexible arrays, other people do, and so we need to take on some improvements to the robustness of the kernel source tree-wide. -Kees
在 2022/1/15 11:50, Kees Cook 写道: > On Thu, Jan 13, 2022 at 10:18:57AM +0100, Peter Zijlstra wrote: >> On Tue, Jan 11, 2022 at 10:14:25AM -0500, Steven Rostedt wrote: >>> On Tue, 11 Jan 2022 12:30:42 +0100 >>> Peter Zijlstra <peterz@infradead.org> wrote: >>> >>>>>>> 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. >>> If for some reason faults changes in size, the original code must be >>> updated whereas the new code is robust enough to not need changing. > I think this alone is reason enough. :) > >> Then I would still much prefer something like: >> >> unsigned int size = sizeof(*grp) + >> NR_NUMA_HINT_FAULT_STATS * numa_node_ids * sizeof(gfp->faults); >> >> Which is still far more readable than some obscure macro. But again, the > I'm not sure it's _obscure_, but it is relatively new. It's even > documented. ;) > https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments > > That said, the original patch is incomplete: it should be using size_t > for "size". thanks, I will send a v3 patch with this change and more detailed commit message. >> It is a fairly useful and common pattern to have a small structure and >> an array in the same memory allocation. >> >> Think hash-tables, the structure contains the size of the table and some >> other things, like for example a seed for the hash function or a lock, >> and then the table itself as an array. > Right, the use of flexible arrays is very common in the kernel. So much > so that we've spent years fixing all the ancient "fake flexible arrays" > scattered around the kernel messing up all kinds of compile-time and > run-time flaw mitigations. Flexible array manipulations are notoriously > prone to mistakes (overflows in allocation, mismatched bounds storage > sizes, array index overflows, etc). These helpers (with more to come) > help remove some of the foot-guns that C would normally impart to them. > >> I can't, nor do I want to, remember all these stupid little macros. Esp. >> not for trivial things like this. > Well, the good news is that other folks will (and are) fixing them for > you. :) Even if you never make mistakes with flexible arrays, other > people do, and so we need to take on some improvements to the robustness > of the kernel source tree-wide. > > -Kees >
On Fri, Jan 14, 2022 at 07:50:47PM -0800, Kees Cook wrote: > On Thu, Jan 13, 2022 at 10:18:57AM +0100, Peter Zijlstra wrote: > > Then I would still much prefer something like: > > > > unsigned int size = sizeof(*grp) + > > NR_NUMA_HINT_FAULT_STATS * numa_node_ids * sizeof(gfp->faults); > > > > Which is still far more readable than some obscure macro. But again, the > > I'm not sure it's _obscure_, but it is relatively new. It's even > documented. ;) > https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments I'm one of those people who doesn't read documentation, I read code. I also flat out refuse to read any documentation that isn't plain text. > > I can't, nor do I want to, remember all these stupid little macros. Esp. > > not for trivial things like this. > > Well, the good news is that other folks will (and are) fixing them for > you. :) Even if you never make mistakes with flexible arrays, other > people do, and so we need to take on some improvements to the robustness > of the kernel source tree-wide. But nobody helps me read the code when I trip over crap like this :/ Why do we have to have endless silly helpers for things that can be trivially expressed in regular C? I appreciate things like container_of() because if you write that out it's a mess, but this, very much not so. struct_size(grp, faults, NR_NUMA_HINT_FAULTS_STATS * numa_node_ids); vs sizeof(*gfp) + sizeof(grp->faults) * NR_NUMA_HINT_FAULT_STATS * nr_node_ids; The latter wins hands down, instantly obvious what it does while with the former I'd have to look up the macro.
On Tue, Jan 18, 2022 at 09:57:45AM +0100, Peter Zijlstra wrote: > On Fri, Jan 14, 2022 at 07:50:47PM -0800, Kees Cook wrote: > > On Thu, Jan 13, 2022 at 10:18:57AM +0100, Peter Zijlstra wrote: > > > > Then I would still much prefer something like: > > > > > > unsigned int size = sizeof(*grp) + > > > NR_NUMA_HINT_FAULT_STATS * numa_node_ids * sizeof(gfp->faults); > > > > > > Which is still far more readable than some obscure macro. But again, the > > > > I'm not sure it's _obscure_, but it is relatively new. It's even > > documented. ;) > > https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments > > I'm one of those people who doesn't read documentation, I read code. > > I also flat out refuse to read any documentation that isn't plain text. Sure, which is why it's in the tree: Documentation/process/deprecated.rst > > > > I can't, nor do I want to, remember all these stupid little macros. Esp. > > > not for trivial things like this. > > > > Well, the good news is that other folks will (and are) fixing them for > > you. :) Even if you never make mistakes with flexible arrays, other > > people do, and so we need to take on some improvements to the robustness > > of the kernel source tree-wide. > > But nobody helps me read the code when I trip over crap like this :/ Why > do we have to have endless silly helpers for things that can be > trivially expressed in regular C? I appreciate things like > container_of() because if you write that out it's a mess, but this, very > much not so. > > struct_size(grp, faults, NR_NUMA_HINT_FAULTS_STATS * numa_node_ids); > > vs > > sizeof(*gfp) + sizeof(grp->faults) * NR_NUMA_HINT_FAULT_STATS * nr_node_ids; > > The latter wins hands down, instantly obvious what it does while with > the former I'd have to look up the macro. One of the drivers is general robustness. The open-coded version can have bugs slowly drift in, especially with the sizeof() ends up naming a structs like: sizeof(struct object) + sizeof(struct element) * NR_NUMA_HINT_FAULT_STATS * nr_node_ids; One of the points of struct_size() is to include the semantic sanity checking that the compiler can do (i.e. making sure "faults" is a member of "grp", correctly sizing them both, avoiding overflows, etc, etc). I know what you mean about not liking looking up new macros, but given C's fragility in these areas, it's been important for us to swap stuff out shift the burdens to the compiler as much as possible. -Kees
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)
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(-)