* [PATCH memcg] mm/page_alloc.c: avoid statistic update with 0
@ 2021-10-08 9:24 Vasily Averin
2021-10-08 11:47 ` Vlastimil Babka
0 siblings, 1 reply; 6+ messages in thread
From: Vasily Averin @ 2021-10-08 9:24 UTC (permalink / raw)
To: Michal Hocko
Cc: Johannes Weiner, Vladimir Davydov, Andrew Morton, cgroups,
linux-mm, linux-kernel, kernel, Mel Gorman, Uladzislau Rezki,
Vlastimil Babka
__alloc_pages_bulk can call __count_zid_vm_events and zone_statistics
with nr_account = 0.
Fixes: 3e23060b2d0b ("mm/page_alloc: batch the accounting updates in the bulk allocator")
Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
---
mm/page_alloc.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 602819a232e5..e67113452ee8 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -5364,9 +5364,10 @@ unsigned long __alloc_pages_bulk(gfp_t gfp, int preferred_nid,
}
local_unlock_irqrestore(&pagesets.lock, flags);
-
- __count_zid_vm_events(PGALLOC, zone_idx(zone), nr_account);
- zone_statistics(ac.preferred_zoneref->zone, zone, nr_account);
+ if (nr_account) {
+ __count_zid_vm_events(PGALLOC, zone_idx(zone), nr_account);
+ zone_statistics(ac.preferred_zoneref->zone, zone, nr_account);
+ }
if (objcg)
memcg_bulk_post_charge_hook(objcg, nr_pre_charge - nr_account);
--
2.31.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH memcg] mm/page_alloc.c: avoid statistic update with 0
2021-10-08 9:24 [PATCH memcg] mm/page_alloc.c: avoid statistic update with 0 Vasily Averin
@ 2021-10-08 11:47 ` Vlastimil Babka
2021-10-12 10:42 ` Vasily Averin
0 siblings, 1 reply; 6+ messages in thread
From: Vlastimil Babka @ 2021-10-08 11:47 UTC (permalink / raw)
To: Vasily Averin, Michal Hocko
Cc: Johannes Weiner, Vladimir Davydov, Andrew Morton, cgroups,
linux-mm, linux-kernel, kernel, Mel Gorman, Uladzislau Rezki
On 10/8/21 11:24, Vasily Averin wrote:
> __alloc_pages_bulk can call __count_zid_vm_events and zone_statistics
> with nr_account = 0.
But that's not a bug, right? Just an effective no-op that's not commonly
happening, so is it worth the check?
> Fixes: 3e23060b2d0b ("mm/page_alloc: batch the accounting updates in the bulk allocator")
> Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
> ---
> mm/page_alloc.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 602819a232e5..e67113452ee8 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -5364,9 +5364,10 @@ unsigned long __alloc_pages_bulk(gfp_t gfp, int preferred_nid,
> }
>
> local_unlock_irqrestore(&pagesets.lock, flags);
> -
> - __count_zid_vm_events(PGALLOC, zone_idx(zone), nr_account);
> - zone_statistics(ac.preferred_zoneref->zone, zone, nr_account);
> + if (nr_account) {
> + __count_zid_vm_events(PGALLOC, zone_idx(zone), nr_account);
> + zone_statistics(ac.preferred_zoneref->zone, zone, nr_account);
> + }
> if (objcg)
> memcg_bulk_post_charge_hook(objcg, nr_pre_charge - nr_account);
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH memcg] mm/page_alloc.c: avoid statistic update with 0
2021-10-08 11:47 ` Vlastimil Babka
@ 2021-10-12 10:42 ` Vasily Averin
2021-10-12 11:09 ` David Hildenbrand
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Vasily Averin @ 2021-10-12 10:42 UTC (permalink / raw)
To: Vlastimil Babka, Michal Hocko
Cc: Johannes Weiner, Vladimir Davydov, Andrew Morton, cgroups,
linux-mm, linux-kernel, kernel, Mel Gorman, Uladzislau Rezki
On 08.10.2021 14:47, Vlastimil Babka wrote:
> On 10/8/21 11:24, Vasily Averin wrote:
>> __alloc_pages_bulk can call __count_zid_vm_events and zone_statistics
>> with nr_account = 0.
>
> But that's not a bug, right? Just an effective no-op that's not commonly
> happening, so is it worth the check?
Why not?
Yes, it's not a bug, it just makes the kernel a bit more efficient in a very unlikely case.
However, it looks strange and makes uninformed code reviewers like me worry about possible
problems inside the affected functions. No one else calls these functions from 0.
>> Fixes: 3e23060b2d0b ("mm/page_alloc: batch the accounting updates in the bulk allocator")
>> Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
>> ---
>> mm/page_alloc.c | 7 ++++---
>> 1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index 602819a232e5..e67113452ee8 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -5364,9 +5364,10 @@ unsigned long __alloc_pages_bulk(gfp_t gfp, int preferred_nid,
>> }
>>
>> local_unlock_irqrestore(&pagesets.lock, flags);
>> -
>> - __count_zid_vm_events(PGALLOC, zone_idx(zone), nr_account);
>> - zone_statistics(ac.preferred_zoneref->zone, zone, nr_account);
>> + if (nr_account) {
>> + __count_zid_vm_events(PGALLOC, zone_idx(zone), nr_account);
>> + zone_statistics(ac.preferred_zoneref->zone, zone, nr_account);
>> + }
>> if (objcg)
>> memcg_bulk_post_charge_hook(objcg, nr_pre_charge - nr_account);
>>
>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH memcg] mm/page_alloc.c: avoid statistic update with 0
2021-10-12 10:42 ` Vasily Averin
@ 2021-10-12 11:09 ` David Hildenbrand
2021-10-12 11:38 ` Mel Gorman
2021-10-12 12:02 ` Chris Down
2 siblings, 0 replies; 6+ messages in thread
From: David Hildenbrand @ 2021-10-12 11:09 UTC (permalink / raw)
To: Vasily Averin, Vlastimil Babka, Michal Hocko
Cc: Johannes Weiner, Vladimir Davydov, Andrew Morton, cgroups,
linux-mm, linux-kernel, kernel, Mel Gorman, Uladzislau Rezki
On 12.10.21 12:42, Vasily Averin wrote:
> On 08.10.2021 14:47, Vlastimil Babka wrote:
>> On 10/8/21 11:24, Vasily Averin wrote:
>>> __alloc_pages_bulk can call __count_zid_vm_events and zone_statistics
>>> with nr_account = 0.
>>
>> But that's not a bug, right? Just an effective no-op that's not commonly
>> happening, so is it worth the check?
>
> Why not?
>
> Yes, it's not a bug, it just makes the kernel a bit more efficient in a very unlikely case.
> However, it looks strange and makes uninformed code reviewers like me worry about possible
> problems inside the affected functions. No one else calls these functions from 0.
>
If it's not a BUG we'd better leave "Fixes:" tags away., it tends to
confuse people looking for actual BUGs.
I'm also not sure if this micro-optimization is worth it. "bit more
efficient in a very unlikely case" doesn't sound very compelling ... and
personally I'd assume accounting functions can deal with a delta of 0.
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH memcg] mm/page_alloc.c: avoid statistic update with 0
2021-10-12 10:42 ` Vasily Averin
2021-10-12 11:09 ` David Hildenbrand
@ 2021-10-12 11:38 ` Mel Gorman
2021-10-12 12:02 ` Chris Down
2 siblings, 0 replies; 6+ messages in thread
From: Mel Gorman @ 2021-10-12 11:38 UTC (permalink / raw)
To: Vasily Averin
Cc: Vlastimil Babka, Michal Hocko, Johannes Weiner, Vladimir Davydov,
Andrew Morton, cgroups, linux-mm, linux-kernel, kernel,
Uladzislau Rezki
On Tue, Oct 12, 2021 at 01:42:41PM +0300, Vasily Averin wrote:
> On 08.10.2021 14:47, Vlastimil Babka wrote:
> > On 10/8/21 11:24, Vasily Averin wrote:
> >> __alloc_pages_bulk can call __count_zid_vm_events and zone_statistics
> >> with nr_account = 0.
> >
> > But that's not a bug, right? Just an effective no-op that's not commonly
> > happening, so is it worth the check?
>
> Why not?
>
> Yes, it's not a bug, it just makes the kernel a bit more efficient in a very unlikely case.
At the cost of an additional branch in the likely case that may be
mispredicted.
> However, it looks strange and makes uninformed code reviewers like me worry about possible
> problems inside the affected functions. No one else calls these functions from 0.
>
The accounting functions should still work with a 0 delta.
--
Mel Gorman
SUSE Labs
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH memcg] mm/page_alloc.c: avoid statistic update with 0
2021-10-12 10:42 ` Vasily Averin
2021-10-12 11:09 ` David Hildenbrand
2021-10-12 11:38 ` Mel Gorman
@ 2021-10-12 12:02 ` Chris Down
2 siblings, 0 replies; 6+ messages in thread
From: Chris Down @ 2021-10-12 12:02 UTC (permalink / raw)
To: Vasily Averin
Cc: Vlastimil Babka, Michal Hocko, Johannes Weiner, Vladimir Davydov,
Andrew Morton, cgroups, linux-mm, linux-kernel, kernel,
Mel Gorman, Uladzislau Rezki
Vasily Averin writes:
>Yes, it's not a bug, it just makes the kernel a bit more efficient in a very unlikely case.
>However, it looks strange and makes uninformed code reviewers like me worry about possible
>problems inside the affected functions. No one else calls these functions from 0.
This statement is meaningless without data. If you have proof that it makes the
kernel more efficient, then please provide the profiles.
As it is I'd be surprised if this improved things. Either the code is hot
enough that the additional branch is cumbersome, or it's cold enough that it
doesn't even matter.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-10-12 12:02 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-08 9:24 [PATCH memcg] mm/page_alloc.c: avoid statistic update with 0 Vasily Averin
2021-10-08 11:47 ` Vlastimil Babka
2021-10-12 10:42 ` Vasily Averin
2021-10-12 11:09 ` David Hildenbrand
2021-10-12 11:38 ` Mel Gorman
2021-10-12 12:02 ` Chris Down
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).