From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.3 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 3A303C33CAF for ; Fri, 17 Jan 2020 00:47:32 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id E288720748 for ; Fri, 17 Jan 2020 00:47:31 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org E288720748 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linux.intel.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 4401E6B02D4; Thu, 16 Jan 2020 19:47:31 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 3F1906B02D5; Thu, 16 Jan 2020 19:47:31 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 2DF9F6B02D6; Thu, 16 Jan 2020 19:47:31 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0117.hostedemail.com [216.40.44.117]) by kanga.kvack.org (Postfix) with ESMTP id 128EC6B02D4 for ; Thu, 16 Jan 2020 19:47:31 -0500 (EST) Received: from smtpin28.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay03.hostedemail.com (Postfix) with SMTP id B97F5824805A for ; Fri, 17 Jan 2020 00:47:30 +0000 (UTC) X-FDA: 76385287860.28.laugh53_74a4d26aa14d X-HE-Tag: laugh53_74a4d26aa14d X-Filterd-Recvd-Size: 5645 Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by imf34.hostedemail.com (Postfix) with ESMTP for ; Fri, 17 Jan 2020 00:47:29 +0000 (UTC) X-Amp-Result: UNSCANNABLE X-Amp-File-Uploaded: False Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga107.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 16 Jan 2020 16:47:28 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.70,327,1574150400"; d="scan'208";a="306066002" Received: from unknown (HELO localhost) ([10.239.159.54]) by orsmga001.jf.intel.com with ESMTP; 16 Jan 2020 16:47:24 -0800 Date: Fri, 17 Jan 2020 08:47:35 +0800 From: Wei Yang To: David Rientjes Cc: Kirill Tkhai , Wei Yang , hannes@cmpxchg.org, mhocko@kernel.org, vdavydov.dev@gmail.com, akpm@linux-foundation.org, kirill.shutemov@linux.intel.com, yang.shi@linux.alibaba.com, cgroups@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, alexander.duyck@gmail.com, stable@vger.kernel.org Subject: Re: [Patch v3] mm: thp: grab the lock before manipulation defer list Message-ID: <20200117004735.GA16207@richard> Reply-To: Wei Yang References: <20200116013100.7679-1-richardw.yang@linux.intel.com> <0bb34c4a-97c7-0b3c-cf43-8af6cf9c4396@virtuozzo.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.9.4 (2018-02-28) X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Thu, Jan 16, 2020 at 02:01:59PM -0800, David Rientjes wrote: >On Thu, 16 Jan 2020, Kirill Tkhai wrote: > >> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c >> > index c5b5f74cfd4d..6450bbe394e2 100644 >> > --- a/mm/memcontrol.c >> > +++ b/mm/memcontrol.c >> > @@ -5360,10 +5360,12 @@ static int mem_cgroup_move_account(struct page *page, >> > } >> > >> > #ifdef CONFIG_TRANSPARENT_HUGEPAGE >> > - if (compound && !list_empty(page_deferred_list(page))) { >> > + if (compound) { >> > spin_lock(&from->deferred_split_queue.split_queue_lock); >> > - list_del_init(page_deferred_list(page)); >> > - from->deferred_split_queue.split_queue_len--; >> > + if (!list_empty(page_deferred_list(page))) { >> > + list_del_init(page_deferred_list(page)); >> > + from->deferred_split_queue.split_queue_len--; >> > + } >> > spin_unlock(&from->deferred_split_queue.split_queue_lock); >> > } >> > #endif >> > @@ -5377,11 +5379,13 @@ static int mem_cgroup_move_account(struct page *page, >> > page->mem_cgroup = to; >> > >> > #ifdef CONFIG_TRANSPARENT_HUGEPAGE >> > - if (compound && list_empty(page_deferred_list(page))) { >> > + if (compound) { >> > spin_lock(&to->deferred_split_queue.split_queue_lock); >> > - list_add_tail(page_deferred_list(page), >> > - &to->deferred_split_queue.split_queue); >> > - to->deferred_split_queue.split_queue_len++; >> > + if (list_empty(page_deferred_list(page))) { >> > + list_add_tail(page_deferred_list(page), >> > + &to->deferred_split_queue.split_queue); >> > + to->deferred_split_queue.split_queue_len++; >> > + } >> > spin_unlock(&to->deferred_split_queue.split_queue_lock); >> > } >> > #endif >> >> The patch looks OK for me. But there is another question. I forget, why we unconditionally >> add a page with empty deferred list to deferred_split_queue. Shouldn't we also check that >> it was initially in the list? Something like: >> >> diff --git a/mm/memcontrol.c b/mm/memcontrol.c >> index d4394ae4e5be..0be0136adaa6 100644 >> --- a/mm/memcontrol.c >> +++ b/mm/memcontrol.c >> @@ -5289,6 +5289,7 @@ static int mem_cgroup_move_account(struct page *page, >> struct pglist_data *pgdat; >> unsigned long flags; >> unsigned int nr_pages = compound ? hpage_nr_pages(page) : 1; >> + bool split = false; >> int ret; >> bool anon; >> >> @@ -5346,6 +5347,7 @@ static int mem_cgroup_move_account(struct page *page, >> if (!list_empty(page_deferred_list(page))) { >> list_del_init(page_deferred_list(page)); >> from->deferred_split_queue.split_queue_len--; >> + split = true; >> } >> spin_unlock(&from->deferred_split_queue.split_queue_lock); >> } >> @@ -5360,7 +5362,7 @@ static int mem_cgroup_move_account(struct page *page, >> page->mem_cgroup = to; >> >> #ifdef CONFIG_TRANSPARENT_HUGEPAGE >> - if (compound) { >> + if (compound && split) { >> spin_lock(&to->deferred_split_queue.split_queue_lock); >> if (list_empty(page_deferred_list(page))) { >> list_add_tail(page_deferred_list(page), >> > >I think that's a good point, especially considering that the current code >appears to unconditionally place any compound page on the deferred split >queue of the destination memcg. The correct list that it should appear >on, I believe, depends on whether the pmd has been split for the process >being moved: note the MC_TARGET_PAGE caveat in >mem_cgroup_move_charge_pte_range() that does not move the charge for >compound pages with split pmds. So when mem_cgroup_move_account() is >called with compound == true, we're moving the charge of the entire >compound page: why would it appear on that memcg's deferred split queue? Well, Kirill's change is easy to understand, while your statement here is a bit hard for me. Seems I lack some knowledge about cgroup. I am sorry about this. :-( -- Wei Yang Help you, Help me