__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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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);
>
>
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 <[email protected]>
>> ---
>> 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);
>>
>>
>
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
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
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.