On 3/17/25 22:54, Shakeel Butt wrote: > 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() You mean first other user than mod_memcg_state() itself. > 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(). I see the point. Well least memcg_stats_lock() isn't expensive, and it's a no-op on non-debug !RT anyway. Acked-by: Vlastimil Babka <vbabka@xxxxxxx>
