On Thu, Mar 30, 2023 at 12:49 AM Michal Hocko <mhocko@xxxxxxxx> wrote: > > On Thu 30-03-23 00:13:16, Yosry Ahmed wrote: > > On Thu, Mar 30, 2023 at 12:06 AM Michal Hocko <mhocko@xxxxxxxx> wrote: > [...] > > > So the real question is. Do we really care so deeply? After all somebody > > > might be calling this from within a spin lock or irq disabled section > > > resulting in a similar situation without noticing. > > > > There are discussions in [1] about making atomic rstat flush not > > disable irqs throughout the process, so in that case it would only > > result in a similar situation if the caller has irq disabled. The only > > caller that might have irq disabled is the same caller that might be > > in irq context before this series: mem_cgroup_usage(). > > > > On that note, and while I have your attention, I was wondering if we > > can eliminate the flush call completely from mem_cgroup_usage(), and > > read the global stats counters for root memcg instead of the root > > counters. There might be subtle differences, but the root memcg usage > > isn't super accurate now anyway (e.g. it doesn't include kernel > > memory). > > root memcg stats are imprecise indeed and I have to admit I do not > really know why we are adding more work for that case. I have tried to > dig into git history for that yesterday but couldn't find a satisfying > answer. It goes all the way down to 2d146aa3aa842 which has done the > switch to rstat. Maybe Johannes remembers. I think it goes back even before that. Even before rstat, we used to get the root usage by iterating over the hierarchy. Maybe we didn't have the global counters at some point or they weren't in line with the root memcg (e.g. use_hierarchy = 0). It would be great if we can just use the global counters here. Hopefully Johannes can confirm or deny this. > > Anyway, back to this particular patch. I still do not see strong reasons > to be verbose about !in_task case so I would just drop this patch. I will drop this patch in the next iteration. If I implement a patch that makes rstat flushing not disable irqs all throughout (like [1]), whether part of this series or not, and we remove flushing from mem_cgroup_usage(), then at this point: - No existing flushers will be disabling irqs. - rstat flushing itself will not be disabling irqs throughout the entire flush. If we achieve that, do you think it makes sense to add WARN_ON_ONCE(irqs_disabled()) instead to prevent future users from flushing while disabling irqs or in irq context? All I am trying to achieve here is make sure we don't regress. I don't want to minimize the current atomic flushers now only to have them increase later. [1] https://lore.kernel.org/lkml/CAJD7tkZrp=4zWvjE9_010TAG1T_crCbf9P64UzJABspgcrGPKg@xxxxxxxxxxxxxx/ > -- > Michal Hocko > SUSE Labs
