Re: [PATCH v3 5/5] mm/memcg: Optimize user context object stock access

[Waiman Long]

On 4/15/21 2:53 PM, Johannes Weiner wrote:
> On Thu, Apr 15, 2021 at 02:16:17PM -0400, Waiman Long wrote:
> > On 4/15/21 1:53 PM, Johannes Weiner wrote:
> > > On Tue, Apr 13, 2021 at 09:20:27PM -0400, Waiman Long wrote:
> > > > Most kmem_cache_alloc() calls are from user context. With instrumentation
> > > > enabled, the measured amount of kmem_cache_alloc() calls from non-task
> > > > context was about 0.01% of the total.
> > > >
> > > > The irq disable/enable sequence used in this case to access content
> > > > from object stock is slow.  To optimize for user context access, there
> > > > are now two object stocks for task context and interrupt context access
> > > > respectively.
> > > >
> > > > The task context object stock can be accessed after disabling preemption
> > > > 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.
> > > >
> > > > The mod_objcg_state() function is also modified to make sure that memcg
> > > > and lruvec stat updates are done with interrupted disabled.
> > > >
> > > > The downside of this change is that there are more data stored in local
> > > > object stocks and not reflected in the charge counter and the vmstat
> > > > arrays.  However, this is a small price to pay for better performance.
> > > >
> > > > Signed-off-by: Waiman Long <longman@xxxxxxxxxx>
> > > > Acked-by: Roman Gushchin <guro@xxxxxx>
> > > > Reviewed-by: Shakeel Butt <shakeelb@xxxxxxxxxx>
> > > This makes sense, and also explains the previous patch a bit
> > > better. But please merge those two.
> > The reason I broke it into two is so that the patches are individually
> > easier to review. I prefer to update the commit log of patch 4 to explain
> > why the obj_stock structure is introduced instead of merging the two.
> Well I did not find them easier to review separately.
>
> > > > @@ -2327,7 +2365,9 @@ static void drain_local_stock(struct work_struct *dummy)
> > > >    	local_irq_save(flags);
> > > >    	stock = this_cpu_ptr(&memcg_stock);
> > > > -	drain_obj_stock(&stock->obj);
> > > > +	drain_obj_stock(&stock->irq_obj);
> > > > +	if (in_task())
> > > > +		drain_obj_stock(&stock->task_obj);
> > > >    	drain_stock(stock);
> > > >    	clear_bit(FLUSHING_CACHED_CHARGE, &stock->flags);
> > > > @@ -3183,7 +3223,7 @@ static inline void mod_objcg_state(struct obj_cgroup *objcg,
> > > >    	memcg = obj_cgroup_memcg(objcg);
> > > >    	if (pgdat)
> > > >    		lruvec = mem_cgroup_lruvec(memcg, pgdat);
> > > > -	__mod_memcg_lruvec_state(memcg, lruvec, idx, nr);
> > > > +	mod_memcg_lruvec_state(memcg, lruvec, idx, nr);
> > > >    	rcu_read_unlock();
> > > This is actually a bug introduced in the earlier patch, isn't it?
> > > Calling __mod_memcg_lruvec_state() without irqs disabled...
> > Not really, in patch 3, mod_objcg_state() is called only in the stock update
> > context where interrupt had already been disabled. But now, that is no
> > longer the case, that is why i need to update mod_objcg_state() to make sure
> > irq is disabled before updating vmstat data array.
> Oh, I see it now. Man, that's subtle. We've had several very hard to
> track down preemption bugs in those stats, because they manifest as
> counter imbalances and you have no idea if there is a leak somewhere.
>
> The convention for these functions is that the __ prefix indicates
> that preemption has been suitably disabled. Please always follow this
> convention, even if the semantic change is temporary.
I see. I will fix that in the next version.
> Btw, is there a reason why the stock caching isn't just part of
> mod_objcg_state()? Why does the user need to choose if they want the
> caching or not? It's not like we ask for this when charging, either.
Yes, I can revert it to make mod_objcg_state() to call 
mod_obj_stock_state() internally instead of the other way around.

Cheers,
Longman