Re: [PATCH 1/1] mm/mmu_gather: replace IPI with synchronize_rcu() when batch allocation fails

[Lance Yang]

On 2026/2/23 23:31, Dave Hansen wrote:
> On 2/23/26 04:58, Lance Yang wrote:
> ...
> > +/**
> > + * tlb_remove_table_sync_rcu() - synchronize with software page-table walkers
> > + *
> > + * Like tlb_remove_table_sync_one() but uses RCU grace period instead of IPI
> > + * broadcast. Should be used in slow paths where sleeping is acceptable.
>
> Just a nit on comments: Use imperative voice:
>
> 	... Use in slow paths where sleeping is acceptable.

Okay, thanks.

> > + * Software/Lockless page-table walkers use local_irq_disable(), which is also
> > + * an RCU read-side critical section. synchronize_rcu() waits for all such
> > + * sections, providing the same guarantee as tlb_remove_table_sync_one() but
> > + * without disrupting all CPUs with IPIs.
>
> Yep, synchronize_rcu() is likely slower (longer wall clock time) but
> less disruptive to other CPUs.
>
> Is it worth explaining here that this should be used when code really
> needs to _wait_ and *not* for freeing memory? Freeing memory should use
> RCU callbacks that don't cause latency spikes in this thread, not this.

Good point! Worth clarifying. Something like:

Note: Use this when code really needs to wait for synchronization,
*not* for freeing memory. Memory freeing should use RCU callbacks
that don't cause latency spikes in this thread.

> > + * Context: Can sleep/block. Cannot be called from any atomic context.
>
> As a general rule, expressing constraints like this is best done in
> code, not comments, so:
>
> 	might_sleep();
> or
> 	WARN_ON_ONCE(!in_atomic());
>
> seem appropriate.
>
> I didn't see any obvious warning like that in the top levels of
> synchronize_rcu().

Yep, synchronize_rcu() does call might_sleep() internally:

synchronize_rcu()
  -> synchronize_rcu_normal()
    -> wait_rcu_gp()      -> __wait_rcu_gp()
        -> might_sleep()
But adding an explicit might_sleep() here makes the constraint
more obvious. I'll add it :)

> > +static void tlb_remove_table_sync_rcu(void)
> > +{
> > +	synchronize_rcu();
> > +}
> > +
> >   #else /* !CONFIG_MMU_GATHER_RCU_TABLE_FREE */
> >
> >   static void tlb_remove_table_free(struct mmu_table_batch *batch)
> > @@ -303,6 +321,10 @@ static void tlb_remove_table_free(struct mmu_table_batch *batch)
> >   	__tlb_remove_table_free(batch);
> >   }
> >
> > +static void tlb_remove_table_sync_rcu(void)
> > +{
> > +}
> > +
> >   #endif /* CONFIG_MMU_GATHER_RCU_TABLE_FREE */
> This seems a _little_ dangerous to even define. We don't want this
> sneaking into use when it doesn't do anything.

This follows the same pattern as tlb_remove_table_sync_one(), which
also has an empty stub in the !CONFIG_MMU_GATHER_RCU_TABLE_FREE path.

It should be in tlb.h like tlb_remove_table_sync_one(). Will put
it there.


Thanks,
Lance