On Mon, Mar 17, 2025 at 09:56:39PM +0100, Vlastimil Babka wrote: > On 3/15/25 18:49, Shakeel Butt wrote: > > For non-PREEMPT_RT kernels, drain_obj_stock() is always called with irq > > disabled, so we can use __mod_memcg_state() instead of > > mod_memcg_state(). For PREEMPT_RT, we need to add memcg_stats_[un]lock > > in __mod_memcg_state(). > > > > Signed-off-by: Shakeel Butt <shakeel.butt@xxxxxxxxx> > > Reviewed-by: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx> > > I've asked in the RFC and from Sebastian's answer I think my question was > misunderstod, so let me try again. > > After this patch we'll have from mod_memcg_state(): > > mod_memcg_state() > local_irq_save(flags); > __mod_memcg_state() > memcg_stats_lock(); <- new and unnecessary? > > Instead of modifying __mod_memcg_state() we could be more targetted and just > do in drain_obj_stock(): > > memcg_stats_lock(); > __mod_memcg_state(); > memcg_stats_unlock(); > > Am I missing something? This seems unnecessary because this patch is adding the first user of __mod_memcg_state() but I think maintainability is better with memcg_stats_[un]lock() inside __mod_memcg_state(). Let's take the example of __mod_memcg_lruvec_state(). It is being called from places where non-RT kernel, the irqs are disabled through spin_lock_irq*, so on RT kernel, the irq would not be disabled and thus explicitly need preemption disabled. What if in future __mod_memcg_state() is being used by a caller which assumes preemption is disabled through irq disable. The RT kernel would be buggy there. I am not sure if it is easy to force the future users to explicitly add memcg_stats_[un]lock() across the call to __mod_memcg_state().
