* [PATCH] hugetlb_cgroup: convert comma to semicolon
@ 2020-08-18 6:43 Xu Wang
2020-08-19 1:40 ` Andrew Morton
0 siblings, 1 reply; 8+ messages in thread
From: Xu Wang @ 2020-08-18 6:43 UTC (permalink / raw)
To: akpm, linux-mm; +Cc: linux-kernel, Xu Wang
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(-)
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] hugetlb_cgroup: convert comma to semicolon
2020-08-18 6:43 [PATCH] hugetlb_cgroup: convert comma to semicolon Xu Wang
@ 2020-08-19 1:40 ` Andrew Morton
2020-08-19 8:14 ` Giuseppe Scrivano
0 siblings, 1 reply; 8+ messages in thread
From: Andrew Morton @ 2020-08-19 1:40 UTC (permalink / raw)
To: Xu Wang; +Cc: linux-mm, linux-kernel, Tejun Heo, Giuseppe Scrivano
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] hugetlb_cgroup: convert comma to semicolon
2020-08-19 1:40 ` Andrew Morton
@ 2020-08-19 8:14 ` Giuseppe Scrivano
0 siblings, 0 replies; 8+ messages in thread
From: Giuseppe Scrivano @ 2020-08-19 8:14 UTC (permalink / raw)
To: Andrew Morton; +Cc: Xu Wang, linux-mm, linux-kernel, Tejun Heo
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] hugetlb_cgroup: convert comma to semicolon
@ 2020-08-19 8:14 ` Giuseppe Scrivano
0 siblings, 0 replies; 8+ messages in thread
From: Giuseppe Scrivano @ 2020-08-19 8:14 UTC (permalink / raw)
To: Andrew Morton; +Cc: Xu Wang, linux-mm, linux-kernel, Tejun Heo
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] hugetlb_cgroup: convert comma to semicolon
2020-08-19 8:14 ` Giuseppe Scrivano
(?)
@ 2020-08-23 15:21 ` Matthew Wilcox
2020-08-23 15:54 ` Matthew Wilcox
2020-08-23 16:04 ` Joe Perches
-1 siblings, 2 replies; 8+ messages in thread
From: Matthew Wilcox @ 2020-08-23 15:21 UTC (permalink / raw)
To: Giuseppe Scrivano
Cc: Andrew Morton, Xu Wang, linux-mm, linux-kernel, Tejun Heo
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;
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] hugetlb_cgroup: convert comma to semicolon
2020-08-23 15:21 ` Matthew Wilcox
@ 2020-08-23 15:54 ` Matthew Wilcox
2020-08-23 16:04 ` Joe Perches
1 sibling, 0 replies; 8+ messages in thread
From: Matthew Wilcox @ 2020-08-23 15:54 UTC (permalink / raw)
To: Giuseppe Scrivano
Cc: Andrew Morton, Xu Wang, linux-mm, linux-kernel, Tejun Heo
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 ;-)
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] hugetlb_cgroup: convert comma to semicolon
2020-08-23 15:21 ` Matthew Wilcox
@ 2020-08-23 16:04 ` Joe Perches
2020-08-23 16:04 ` Joe Perches
1 sibling, 0 replies; 8+ messages in thread
From: Joe Perches @ 2020-08-23 16:04 UTC (permalink / raw)
To: Matthew Wilcox, Giuseppe Scrivano
Cc: Andrew Morton, Xu Wang, linux-mm, linux-kernel, Tejun Heo
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] hugetlb_cgroup: convert comma to semicolon
@ 2020-08-23 16:04 ` Joe Perches
0 siblings, 0 replies; 8+ messages in thread
From: Joe Perches @ 2020-08-23 16:04 UTC (permalink / raw)
To: Matthew Wilcox, Giuseppe Scrivano
Cc: Andrew Morton, Xu Wang, linux-mm, linux-kernel, Tejun Heo
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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-08-23 16:04 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-18 6:43 [PATCH] hugetlb_cgroup: convert comma to semicolon Xu Wang
2020-08-19 1:40 ` Andrew Morton
2020-08-19 8:14 ` Giuseppe Scrivano
2020-08-19 8:14 ` Giuseppe Scrivano
2020-08-23 15:21 ` Matthew Wilcox
2020-08-23 15:54 ` Matthew Wilcox
2020-08-23 16:04 ` Joe Perches
2020-08-23 16:04 ` Joe Perches
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.