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=-8.2 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, USER_AGENT_SANE_1 autolearn=ham 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 3E6D3C33C9E for ; Sun, 12 Jan 2020 02:29:09 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 02BA920848 for ; Sun, 12 Jan 2020 02:29:08 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 02BA920848 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 6BF498E0006; Sat, 11 Jan 2020 21:29:08 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 66F228E0001; Sat, 11 Jan 2020 21:29:08 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 584DC8E0006; Sat, 11 Jan 2020 21:29:08 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0230.hostedemail.com [216.40.44.230]) by kanga.kvack.org (Postfix) with ESMTP id 3B6DC8E0001 for ; Sat, 11 Jan 2020 21:29:08 -0500 (EST) Received: from smtpin19.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with SMTP id E2827181AC9B6 for ; Sun, 12 Jan 2020 02:29:07 +0000 (UTC) X-FDA: 76367399934.19.sky80_15704b5699516 X-HE-Tag: sky80_15704b5699516 X-Filterd-Recvd-Size: 5037 Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by imf19.hostedemail.com (Postfix) with ESMTP for ; Sun, 12 Jan 2020 02:29:07 +0000 (UTC) X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by orsmga106.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 11 Jan 2020 18:29:05 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.69,423,1571727600"; d="scan'208";a="218389651" Received: from richard.sh.intel.com (HELO localhost) ([10.239.159.54]) by fmsmga007.fm.intel.com with ESMTP; 11 Jan 2020 18:29:03 -0800 Date: Sun, 12 Jan 2020 10:28:58 +0800 From: Wei Yang To: "Kirill A. Shutemov" Cc: Wei Yang , hannes@cmpxchg.org, mhocko@kernel.org, vdavydov.dev@gmail.com, akpm@linux-foundation.org, cgroups@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, kirill.shutemov@linux.intel.com, yang.shi@linux.alibaba.com, alexander.duyck@gmail.com, rientjes@google.com Subject: Re: [Patch v2] mm: thp: grab the lock before manipulation defer list Message-ID: <20200112022858.GA17733@richard> Reply-To: Wei Yang References: <20200109143054.13203-1-richardw.yang@linux.intel.com> <20200111000352.efy6krudecpshezh@box> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200111000352.efy6krudecpshezh@box> 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 Sat, Jan 11, 2020 at 03:03:52AM +0300, Kirill A. Shutemov wrote: >On Thu, Jan 09, 2020 at 10:30:54PM +0800, Wei Yang wrote: >> As all the other places, we grab the lock before manipulate the defer list. >> Current implementation may face a race condition. >> >> For example, the potential race would be: >> >> CPU1 CPU2 >> mem_cgroup_move_account split_huge_page_to_list >> !list_empty >> lock >> !list_empty >> list_del >> unlock >> lock >> # !list_empty might not hold anymore >> list_del_init >> unlock > >I don't think this particular race is possible. Both parties take page >lock before messing with deferred queue, but anytway: I am afraid not. Page lock is per page, while defer queue is per pgdate or memcg. It is possible two page in the same pgdate or memcg grab page lock respectively and then access the same defer queue concurrently. > >Acked-by: Kirill A. Shutemov > >> >> When this sequence happens, the list_del_init() in >> mem_cgroup_move_account() would crash if CONFIG_DEBUG_LIST since the >> page is already been removed by list_del in split_huge_page_to_list(). >> >> Fixes: 87eaceb3faa5 ("mm: thp: make deferred split shrinker memcg aware") >> >> Signed-off-by: Wei Yang >> Acked-by: David Rientjes >> >> --- >> v2: >> * move check on compound outside suggested by Alexander >> * an example of the race condition, suggested by Michal >> --- >> mm/memcontrol.c | 18 +++++++++++------- >> 1 file changed, 11 insertions(+), 7 deletions(-) >> >> diff --git a/mm/memcontrol.c b/mm/memcontrol.c >> index bc01423277c5..1492eefe4f3c 100644 >> --- a/mm/memcontrol.c >> +++ b/mm/memcontrol.c >> @@ -5368,10 +5368,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 >> @@ -5385,11 +5387,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 >> -- >> 2.17.1 >> >> > >-- > Kirill A. Shutemov -- Wei Yang Help you, Help me