| Message ID | b2371951-bb8a-e62e-8d33-10830bbf6275@virtuozzo.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [memcg] mm/page_alloc.c: avoid statistic update with 0 | expand |
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); > >
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); >> >> >
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.
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.
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.
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);
__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(-)