| Message ID | 20200818064333.21759-1-vulab@iscas.ac.cn (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hugetlb_cgroup: convert comma to semicolon | expand |
On Tue, 18 Aug 2020 06:43:33 +0000 Xu Wang <vulab@iscas.ac.cn> wrote: > Replace a comma between expression statements by a semicolon. > > ... > > --- a/mm/hugetlb_cgroup.c > +++ b/mm/hugetlb_cgroup.c > @@ -655,7 +655,7 @@ static void __init __hugetlb_cgroup_file_dfl_init(int idx) > snprintf(cft->name, MAX_CFTYPE_NAME, "%s.events", buf); > cft->private = MEMFILE_PRIVATE(idx, 0); > cft->seq_show = hugetlb_events_show; > - cft->file_offset = offsetof(struct hugetlb_cgroup, events_file[idx]), > + cft->file_offset = offsetof(struct hugetlb_cgroup, events_file[idx]); > cft->flags = CFTYPE_NOT_ON_ROOT; > > /* Add the events.local file */ > @@ -664,7 +664,7 @@ static void __init __hugetlb_cgroup_file_dfl_init(int idx) > cft->private = MEMFILE_PRIVATE(idx, 0); > cft->seq_show = hugetlb_events_local_show; > cft->file_offset = offsetof(struct hugetlb_cgroup, > - events_local_file[idx]), > + events_local_file[idx]); > cft->flags = CFTYPE_NOT_ON_ROOT; > > /* NULL terminate the last cft */ Fixes: faced7e0806cf4 ("mm: hugetlb controller for cgroups v2") Wow, why does this code even work. Presumably the initial value of cft->file_offset simply doesn't matter. Giuseppe, could you please check? We might have some unneeded code in there.
Andrew Morton <akpm@linux-foundation.org> writes: > On Tue, 18 Aug 2020 06:43:33 +0000 Xu Wang <vulab@iscas.ac.cn> wrote: > >> Replace a comma between expression statements by a semicolon. >> >> ... >> >> --- a/mm/hugetlb_cgroup.c >> +++ b/mm/hugetlb_cgroup.c >> @@ -655,7 +655,7 @@ static void __init __hugetlb_cgroup_file_dfl_init(int idx) >> snprintf(cft->name, MAX_CFTYPE_NAME, "%s.events", buf); >> cft->private = MEMFILE_PRIVATE(idx, 0); >> cft->seq_show = hugetlb_events_show; >> - cft->file_offset = offsetof(struct hugetlb_cgroup, events_file[idx]), >> + cft->file_offset = offsetof(struct hugetlb_cgroup, events_file[idx]); >> cft->flags = CFTYPE_NOT_ON_ROOT; >> >> /* Add the events.local file */ >> @@ -664,7 +664,7 @@ static void __init __hugetlb_cgroup_file_dfl_init(int idx) >> cft->private = MEMFILE_PRIVATE(idx, 0); >> cft->seq_show = hugetlb_events_local_show; >> cft->file_offset = offsetof(struct hugetlb_cgroup, >> - events_local_file[idx]), >> + events_local_file[idx]); >> cft->flags = CFTYPE_NOT_ON_ROOT; >> >> /* NULL terminate the last cft */ > > Fixes: faced7e0806cf4 ("mm: hugetlb controller for cgroups v2") Xu, thanks for spotting it. Was this code causing any issue or have you found by inspecting it? > Wow, why does this code even work. Presumably the initial value of > cft->file_offset simply doesn't matter. Giuseppe, could you please > check? We might have some unneeded code in there. I think in this case having two expressions as part of the same statement is equivalent to having two separate statements. Both cft->file_offset and cft->flags get the expected value. Regards, Giuseppe
On Wed, Aug 19, 2020 at 10:14:11AM +0200, Giuseppe Scrivano wrote: > >> - cft->file_offset = offsetof(struct hugetlb_cgroup, events_file[idx]), > >> + cft->file_offset = offsetof(struct hugetlb_cgroup, events_file[idx]); > >> cft->flags = CFTYPE_NOT_ON_ROOT; > > I think in this case having two expressions as part of the same > statement is equivalent to having two separate statements. Both > cft->file_offset and cft->flags get the expected value. That's not how the comma operator works. It will evaluate offsetof(struct hugetlb_cgroup, events_file[idx]) and then discard the result. Since it has no side-effects, this is effectively doing: cft->file_offset = cft->flags = CFTYPE_NOT_ON_ROOT;
On Sun, Aug 23, 2020 at 04:21:30PM +0100, Matthew Wilcox wrote: > On Wed, Aug 19, 2020 at 10:14:11AM +0200, Giuseppe Scrivano wrote: > > >> - cft->file_offset = offsetof(struct hugetlb_cgroup, events_file[idx]), > > >> + cft->file_offset = offsetof(struct hugetlb_cgroup, events_file[idx]); > > >> cft->flags = CFTYPE_NOT_ON_ROOT; > > > > I think in this case having two expressions as part of the same > > statement is equivalent to having two separate statements. Both > > cft->file_offset and cft->flags get the expected value. > > That's not how the comma operator works. > > It will evaluate offsetof(struct hugetlb_cgroup, events_file[idx]) and > then discard the result. Since it has no side-effects, this is effectively > doing: > > cft->file_offset = cft->flags = CFTYPE_NOT_ON_ROOT; _oh_. I tested this. I'm wrong because the comma operator is at lower precedence than assignment. Testcase: struct a { int x; int y; }; void g(struct a *a) { a->x = 1, a->y = 0; } void h(struct a *a) { a->x = (1, a->y = 0); } test.c: In function ‘h’: test.c:12:12: warning: left-hand operand of comma expression has no effect [-Wunused-value] 12 | a->x = (1, | ^ 0000000000000000 <g>: 0: 48 c7 07 01 00 00 00 movq $0x1,(%rdi) 7: c3 retq 8: 0f 1f 84 00 00 00 00 nopl 0x0(%rax,%rax,1) f: 00 0000000000000010 <h>: 10: 48 c7 07 00 00 00 00 movq $0x0,(%rdi) 17: c3 retq So there's no bug here! It's just confusing, so should be fixed. (I think Andrew was confused too ;-)
On Sun, 2020-08-23 at 16:21 +0100, Matthew Wilcox wrote: > On Wed, Aug 19, 2020 at 10:14:11AM +0200, Giuseppe Scrivano wrote: > > > > - cft->file_offset = offsetof(struct hugetlb_cgroup, events_file[idx]), > > > > + cft->file_offset = offsetof(struct hugetlb_cgroup, events_file[idx]); > > > > cft->flags = CFTYPE_NOT_ON_ROOT; > > > > I think in this case having two expressions as part of the same > > statement is equivalent to having two separate statements. Both > > cft->file_offset and cft->flags get the expected value. > > That's not how the comma operator works. > > It will evaluate offsetof(struct hugetlb_cgroup, events_file[idx]) and > then discard the result. Since it has no side-effects, this is effectively > doing: > > cft->file_offset = cft->flags = CFTYPE_NOT_ON_ROOT; $ gcc -x c - #include <stdio.h> #include <stdlib.h> struct foo { int a; char b[50]; }; int main(int argc, char **argv) { int a; int b; a = sizeof(struct foo), b = 1; printf("a: %d, b: %d\n", a, b); return 0; } $ ./a.out a: 56, b: 1
diff --git a/mm/hugetlb_cgroup.c b/mm/hugetlb_cgroup.c index aabf65d4d91b..1f87aec9ab5c 100644 --- a/mm/hugetlb_cgroup.c +++ b/mm/hugetlb_cgroup.c @@ -655,7 +655,7 @@ static void __init __hugetlb_cgroup_file_dfl_init(int idx) snprintf(cft->name, MAX_CFTYPE_NAME, "%s.events", buf); cft->private = MEMFILE_PRIVATE(idx, 0); cft->seq_show = hugetlb_events_show; - cft->file_offset = offsetof(struct hugetlb_cgroup, events_file[idx]), + cft->file_offset = offsetof(struct hugetlb_cgroup, events_file[idx]); cft->flags = CFTYPE_NOT_ON_ROOT; /* Add the events.local file */ @@ -664,7 +664,7 @@ static void __init __hugetlb_cgroup_file_dfl_init(int idx) cft->private = MEMFILE_PRIVATE(idx, 0); cft->seq_show = hugetlb_events_local_show; cft->file_offset = offsetof(struct hugetlb_cgroup, - events_local_file[idx]), + events_local_file[idx]); cft->flags = CFTYPE_NOT_ON_ROOT; /* NULL terminate the last cft */
Replace a comma between expression statements by a semicolon. Signed-off-by: Xu Wang <vulab@iscas.ac.cn> --- mm/hugetlb_cgroup.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)