Re: [PATCH 0/4] memcg: cleanup the memcg stats interfaces

Re: [PATCH 0/4] memcg: cleanup the memcg stats interfaces

[Harry Yoo]

On Tue, Nov 11, 2025 at 10:48:18AM +0800, Qi Zheng wrote:
> Hi Shakeel,
> 
> On 11/11/25 10:39 AM, Shakeel Butt wrote:
> > On Tue, Nov 11, 2025 at 10:23:15AM +0800, Qi Zheng wrote:
> > > Hi,
> > > 
> > [...]
> > > > 
> > > > Are you or Qi planning a follow-up that converts spin_lock_irq() to
> > > > spin_lock() in places where they disabled IRQs was just to update vmstat?
> > > 
> > > Perhaps this change could be implemented together in [PATCH 1/4]?
> > > 
> > > Of course, it's also reasonable to make it a separate patch. If we
> > > choose this method, I’m fine with either me or Shakeel doing it.
> > > 
> > 
> > Let's do it separately as I wanted to keep the memcg related changes
> > self-contained.
> 
> OK.

Agreed.

> > Qi, can you please take a stab at that?
> 
> Sure, I will do it.

I'll be more than happy to review that ;)

> > > > Qi's zombie memcg series will depends on that work I guess..
> > > 
> > > Yes, and there are other places that also need to be converted, such as
> > > __folio_migrate_mapping().
> > 
> > I see __mod_zone_page_state() usage in __folio_migrate_mapping() and
> > using the same reasoning we can convert it to use mod_zone_page_state().
> > Where else do you need to do these conversions (other than
> > __folio_migrate_mapping)?
> 
> I mean converting these places to use spin_lock() instead of
> spin_lock_irq().

Just one thing I noticed while looking at __folio_migrate_mapping()...

- xas_lock_irq() -> xas_unlock() -> local_irq_enable()
- swap_cluster_get_and_lock_irq() -> swap_cluster_unlock() -> local_irq_enable()

is wrong because spin_lock_irq() doesn't disable IRQ on PREEMPT_RT.

Not 100% sure if it would be benign or lead to actual bugs that need
to be fixed in -stable kernels.

Cc'ing RT folks again :)

-- 
Cheers,
Harry / Hyeonggon

Re: [PATCH 0/4] memcg: cleanup the memcg stats interfaces

[Sebastian Andrzej Siewior]

> > I mean converting these places to use spin_lock() instead of
> > spin_lock_irq().
> 
> Just one thing I noticed while looking at __folio_migrate_mapping()...
> 
> - xas_lock_irq() -> xas_unlock() -> local_irq_enable()
> - swap_cluster_get_and_lock_irq() -> swap_cluster_unlock() -> local_irq_enable()
> 
> is wrong because spin_lock_irq() doesn't disable IRQ on PREEMPT_RT.
> 
> Not 100% sure if it would be benign or lead to actual bugs that need
> to be fixed in -stable kernels.
> 
> Cc'ing RT folks again :)

The tail in __folio_migrate_mapping() after xas_unlock()/
swap_cluster_unlock(), is limited to __mod_lruvec_state() based stats
updates. There is a preempt_disable_nested() in __mod_zone_page_state()
to ensure that the update happens on the same CPU and is not preempted.
On PREEMPT_RT there should be no in-IRQ updates of these counters.
The IRQ enable at the end does nothing. There might be CPU migration
between the individual stats updates.
If it remains like this, it is fine. Please don't advertise ;)

Sebastian