Messages in this thread | | | Date | Fri, 17 Dec 2021 13:46:53 -0500 | Subject | Re: [PATCH-next v3] mm/memcg: Properly handle memcg_stock access for PREEMPT_RT | From | Waiman Long <> |
| |
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
| |