Re: [PATCH-next v3] mm/memcg: Properly handle memcg_stock access for PREEMPT_RT

Waiman Long
2021-12-17T18:46:53Z

On 12/17/21 06:42, Sebastian Andrzej Siewior wrote:
> On 2021-12-14 09:44:12 [-0500], Waiman Long wrote:
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -2096,7 +2096,12 @@ struct obj_stock {
>> #endif
>> };
>>
>> +/*
>> + * The local_lock protects the whole memcg_stock_pcp structure including
>> + * the embedded obj_stock structures.
>> + */
>> struct memcg_stock_pcp {
>> + local_lock_t lock;
>> struct mem_cgroup *cached; /* this never be root cgroup */
>> unsigned int nr_pages;
>> struct obj_stock task_obj;
>> @@ -2145,7 +2150,7 @@ static bool consume_stock(struct mem_cgroup *memcg, unsigned int nr_pages)
>> if (nr_pages > MEMCG_CHARGE_BATCH)
>> return ret;
>>
>> - local_irq_save(flags);
>> + local_lock_irqsave(&memcg_stock.lock, flags);
> This still does not explain why the lock is acquired here where it
> appears to be unrelated to memcg_stock.lock.

consume_stock() can be called in both task and irq context. irq context
may include softirq where interrupt may have been enabled and something
get interrupt again. The original code just do a local_irq_save()
without documenting why we are doing so. So I didn't see a need to add
comment about that.

>>
>> stock = this_cpu_ptr(&memcg_stock);
>> if (memcg == stock->cached && stock->nr_pages >= nr_pages) {
>> @@ -2779,29 +2784,34 @@ static struct mem_cgroup *get_mem_cgroup_from_objcg(struct obj_cgroup *objcg)
>> * which is cheap in non-preempt kernel. The interrupt context object stock
>> * can only be accessed after disabling interrupt. User context code can
>> * access interrupt object stock, but not vice versa.
>> + *
>> + * This task and interrupt context optimization is disabled for PREEMPT_RT
>> + * as there is no performance gain in this case and changes will be made to
>> + * irq_obj only.
>> + *
>> + * For non-PREEMPT_RT, we are not replacing preempt_disable() by local_lock()
>> + * as nesting of task_obj and irq_obj are allowed which may cause lockdep
>> + * splat if local_lock() is used. Using separate local locks will complicate
>> + * the interaction between obj_stock and the broader memcg_stock object.
>> */
>> static inline struct obj_stock *get_obj_stock(unsigned long *pflags)
>> {
>> - struct memcg_stock_pcp *stock;
>> -
>> - if (likely(in_task())) {
>> + if (likely(in_task()) && !IS_ENABLED(CONFIG_PREEMPT_RT)) {
>> *pflags = 0UL;
>> preempt_disable();
>> - stock = this_cpu_ptr(&memcg_stock);
>> - return &stock->task_obj;
>> + return this_cpu_ptr(&memcg_stock.task_obj);
> Do we need to keep the memcg_stock.task_obj for !RT?
> I'm not really convinced that disabling either preemption or interrupts
> is so much better compared to actual locking locking with lockdep
> annotation. Looking at the history, I'm also impressed by that fact that
> disabling/ enabling interrupts is *so* expensive that all this is
> actually worth it.

For !RT with voluntary or no preemption, preempt_disable() is just a
compiler barrier. So it is definitely cheaper than disabling interrupt.
The performance benefit is less with preemptible but !RT kernel.
Microbenchmark testing shows a performance improvement of a few percents
depending on the exact benchmark.

Cheers,
Longman