From: Thomas Gleixner <email@example.com> To: Mel Gorman <firstname.lastname@example.org>, Andrew Morton <email@example.com> Cc: Ingo Molnar <firstname.lastname@example.org>, Vlastimil Babka <email@example.com>, Hugh Dickins <firstname.lastname@example.org>, Linux-MM <email@example.com>, Linux-RT-Users <firstname.lastname@example.org>, LKML <email@example.com>, Mel Gorman <firstname.lastname@example.org>, Peter Zijlstra <email@example.com> Subject: Re: [PATCH 2/2] mm/vmstat: Protect per cpu variables with preempt disable on RT Date: Wed, 04 Aug 2021 01:54:47 +0200 [thread overview] Message-ID: <87czqu2iew.ffs@tglx> (raw) In-Reply-To: <firstname.lastname@example.org> Mel! On Fri, Jul 23 2021 at 11:00, Mel Gorman wrote: > From: Ingo Molnar <email@example.com> > > Disable preemption on -RT for the vmstat code. On vanila the code runs > in IRQ-off regions while on -RT it may not when stats are updated under > a local_lock. "preempt_disable" ensures that the same resources is not > updated in parallel due to preemption. > > This patch differs from the preempt-rt version where __count_vm_event and > __count_vm_events are also protected. The counters are explicitly "allowed > to be to be racy" so there is no need to protect them from preemption. Only > the accurate page stats that are updated by a read-modify-write need > protection. > > Signed-off-by: Ingo Molnar <firstname.lastname@example.org> > Signed-off-by: Thomas Gleixner <email@example.com> > Signed-off-by: Mel Gorman <firstname.lastname@example.org> > --- > mm/vmstat.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/mm/vmstat.c b/mm/vmstat.c > index b0534e068166..d06332c221b1 100644 > --- a/mm/vmstat.c > +++ b/mm/vmstat.c > @@ -319,6 +319,7 @@ void __mod_zone_page_state(struct zone *zone, enum zone_stat_item item, > long x; > long t; > > + preempt_disable_rt(); Yes, this is smart to some extent. But in reality it's a bandaid simply because nobody can tell which item of vmstat requires which protection. If you go back in RT history then you will figure out that we were able to eliminate _all_ occurences of preempt_disable_rt() except for this one. Even mm developers are wary about this: <tglx> so in vmstat.c there is this magic comment: <tglx> * For use when we know that interrupts are disabled <tglx> * or when we know that preemption is disabled and that <tglx> * particular counter cannot be updated from interrupt context. <tglx> how can I know which counters need what? <mm_expert> I don't think there's a list, one would have to check on counter to counter basis :/ <tglx> and of course there is nothing which validates that, right? <mm_expert> exactly Brilliant stuff which prevents you to do any validation on this. Over the years there have been several issues where callers had to be fixed by analysing bug reports instead of having a proper instrumentation in that code which would have told the developer that he got it wrong. Of course on RT kernels the preempt_disable_rt() will serialize everything correctly, but as we have learned over the years just slapping _if_rt() or if_not_rt() variants of things around is most of the time papering over the underlying problem of badly defined protection scopes. Let's not proliferate that. As I said in the above IRC conversation: <tglx> I fundamentally hate this preempt_disable_rt() muck Thanks, tglx
next prev parent reply other threads:[~2021-08-03 23:54 UTC|newest] Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-07-23 10:00 [PATCH 0/2] Protect vmstats on PREEMPT_RT Mel Gorman 2021-07-23 10:00 ` [PATCH 1/2] preempt: Provide preempt_*_(no)rt variants Mel Gorman 2021-07-23 10:00 ` [PATCH 2/2] mm/vmstat: Protect per cpu variables with preempt disable on RT Mel Gorman 2021-08-03 23:54 ` Thomas Gleixner [this message] 2021-08-04 9:54 ` Mel Gorman 2021-08-04 13:42 ` Vlastimil Babka 2021-08-04 14:23 ` Mel Gorman 2021-08-05 12:56 ` Thomas Gleixner 2021-08-05 14:04 ` Mel Gorman 2021-08-05 15:42 ` Thomas Gleixner 2021-08-05 18:47 ` Daniel Vacek
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=87czqu2iew.ffs@tglx \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --subject='Re: [PATCH 2/2] mm/vmstat: Protect per cpu variables with preempt disable on RT' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
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).