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

[Lance Yang <lance.yang@linux.dev>]

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