Replace a comma between expression statements by a semicolon.
Signed-off-by: Xu Wang <[email protected]>
---
mm/hugetlb_cgroup.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
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 */
--
2.17.1
On Tue, 18 Aug 2020 06:43:33 +0000 Xu Wang <[email protected]> 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 <[email protected]> writes:
> On Tue, 18 Aug 2020 06:43:33 +0000 Xu Wang <[email protected]> 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