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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 15 Apr 2021 15:06:28 -0400
From:   Waiman Long <llong@...hat.com>
To:     Johannes Weiner <hannes@...xchg.org>,
        Waiman Long <llong@...hat.com>
Cc:     Michal Hocko <mhocko@...nel.org>,
        Vladimir Davydov <vdavydov.dev@...il.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Tejun Heo <tj@...nel.org>, Christoph Lameter <cl@...ux.com>,
        Pekka Enberg <penberg@...nel.org>,
        David Rientjes <rientjes@...gle.com>,
        Joonsoo Kim <iamjoonsoo.kim@....com>,
        Vlastimil Babka <vbabka@...e.cz>, Roman Gushchin <guro@...com>,
        linux-kernel@...r.kernel.org, cgroups@...r.kernel.org,
        linux-mm@...ck.org, Shakeel Butt <shakeelb@...gle.com>,
        Muchun Song <songmuchun@...edance.com>,
        Alex Shi <alex.shi@...ux.alibaba.com>,
        Chris Down <chris@...isdown.name>,
        Yafang Shao <laoar.shao@...il.com>,
        Wei Yang <richard.weiyang@...il.com>,
        Masayoshi Mizuma <msys.mizuma@...il.com>,
        Xing Zhengjun <zhengjun.xing@...ux.intel.com>
Subject: Re: [PATCH v3 5/5] mm/memcg: Optimize user context object stock
 access

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@...hat.com>
>>>> Acked-by: Roman Gushchin <guro@...com>
>>>> Reviewed-by: Shakeel Butt <shakeelb@...gle.com>
>>> 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