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=-6.6 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS 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 4EC1EC3F68F for ; Mon, 6 Jan 2020 16:18:47 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 0B590207FF for ; Mon, 6 Jan 2020 16:18:46 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="ScZ+BLpe" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 0B590207FF Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 9A4018E0005; Mon, 6 Jan 2020 11:18:46 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 955258E0001; Mon, 6 Jan 2020 11:18:46 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 8692B8E0005; Mon, 6 Jan 2020 11:18:46 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0176.hostedemail.com [216.40.44.176]) by kanga.kvack.org (Postfix) with ESMTP id 710E78E0001 for ; Mon, 6 Jan 2020 11:18:46 -0500 (EST) Received: from smtpin15.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay03.hostedemail.com (Postfix) with SMTP id 395848249980 for ; Mon, 6 Jan 2020 16:18:46 +0000 (UTC) X-FDA: 76347717852.15.stop34_4c885e3f5fe4a X-HE-Tag: stop34_4c885e3f5fe4a X-Filterd-Recvd-Size: 5867 Received: from mail-io1-f66.google.com (mail-io1-f66.google.com [209.85.166.66]) by imf01.hostedemail.com (Postfix) with ESMTP for ; Mon, 6 Jan 2020 16:18:45 +0000 (UTC) Received: by mail-io1-f66.google.com with SMTP id b10so49149273iof.11 for ; Mon, 06 Jan 2020 08:18:45 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=1iBF+C1NHazjfiJGahas8jIgSGj241V/MA8/jj2HpLE=; b=ScZ+BLpeYEmcBri4/E40jt+m15BKdbAIG7h7sy3ejJj/Dtb+nL8mk9HYuTan58Ac5v 1jtRC/XgW9iWes1rejXCb5Nkiv5JXH6T9ZGxBYCL+6tKO6G18SbfNq01Hee7NCjGU29q sPkjPpV3m1fQxCSNdaOmoqXu4lvyH7Q5dXDZKFCvLTwrQW1XaQf0NumtQUoDu2227ya2 DfJoLQvdnFKa7/6WrrmLuPVLOmX1NyFUKfWqUg8DzsYodq7qhN3ILwABQCrq9awbO9fu r430448Ffc4Qp7BbEPtygBu1l6bcwp97AvALV4Cn3puvjnJCSlDYaTHUm59DJXzKmGPX vWyQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=1iBF+C1NHazjfiJGahas8jIgSGj241V/MA8/jj2HpLE=; b=ZC26NSCiASZwuTPWM01jesH6PPNGJehlqPYJRc230SMCzKuSUAYEb7hvWB/C2mO1XZ kjEjd//x6JWkgNutPO9hU6gLUv4qOqcLauea3GrZEN01KUKTzKW2jVR0afHfwzI0aWeE m1L4J3vYgbCZpz3K/VwcLjyevUqtfZ2W5kBGJ+d9QtbjS5giqjU9GvQp2FNh0Ai5Y9aN hx9A6rQD5MKbtm3sEbiiX3tgJNKquSKTclDXK0GzrGachCDr8+yv0iU0qannTpS2szbN 0Xm7d9lRBYaEbtgt2umgUlJg2OnMDL0V9dkLxAx/isx951kEmj9REfu2UvGf00hUHsn7 idgw== X-Gm-Message-State: APjAAAXFY1XPvQRVywLRHsYp9r0SMVgIVm9zzn2Z/ZBycZslVUA40WcN J1D9zhUowSscEtr38d4BZBQ8GMU1wbMvpdL7kpU= X-Google-Smtp-Source: APXvYqzuPLiJ4vUg0YkwQhUHnUnwKKB/CsT/pr8SpYnnKWoouOidOXCDH2ImDMv0T/GGKjlvg+D0xcL+eVRS4FkS4C8= X-Received: by 2002:a5d:80d9:: with SMTP id h25mr63343225ior.97.1578327524845; Mon, 06 Jan 2020 08:18:44 -0800 (PST) MIME-Version: 1.0 References: <20200103143407.1089-1-richardw.yang@linux.intel.com> In-Reply-To: <20200103143407.1089-1-richardw.yang@linux.intel.com> From: Alexander Duyck Date: Mon, 6 Jan 2020 08:18:34 -0800 Message-ID: Subject: Re: [RFC PATCH] mm: thp: grab the lock before manipulation defer list To: Wei Yang Cc: Johannes Weiner , Michal Hocko , vdavydov.dev@gmail.com, Andrew Morton , "Kirill A. Shutemov" , cgroups@vger.kernel.org, linux-mm , LKML , Yang Shi Content-Type: text/plain; charset="UTF-8" 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 Fri, Jan 3, 2020 at 6:34 AM Wei Yang wrote: > > As all the other places, we grab the lock before manipulate the defer list. > Current implementation may face a race condition. > > Fixes: 87eaceb3faa5 ("mm: thp: make deferred split shrinker memcg aware") > > Signed-off-by: Wei Yang > > --- > I notice the difference during code reading and just confused about the > difference. No specific test is done since limited knowledge about cgroup. > > Maybe I miss something important? > --- > mm/memcontrol.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index bc01423277c5..62b7ec34ef1a 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -5368,12 +5368,12 @@ static int mem_cgroup_move_account(struct page *page, > } > > #ifdef CONFIG_TRANSPARENT_HUGEPAGE > + spin_lock(&from->deferred_split_queue.split_queue_lock); > if (compound && !list_empty(page_deferred_list(page))) { > - spin_lock(&from->deferred_split_queue.split_queue_lock); > list_del_init(page_deferred_list(page)); > from->deferred_split_queue.split_queue_len--; > - spin_unlock(&from->deferred_split_queue.split_queue_lock); > } > + spin_unlock(&from->deferred_split_queue.split_queue_lock); > #endif > /* > * It is safe to change page->mem_cgroup here because the page So I suspect the lock placement has to do with the compound boolean value passed to the function. One thing you might want to do is pull the "if (compound)" check out and place it outside of the spinlock check. It would then simplify this signficantly so it is something like if (compound) { spin_lock(); list = page_deferred_list(page); if (!list_empty(list)) { list_del_init(list); from->..split_queue_len--; } spin_unlock(); } Same for the block below. I would pull the check for compound outside of the spinlock call since it is a value that shouldn't change and would eliminate an unnecessary lock in the non-compound case. > @@ -5385,13 +5385,13 @@ static int mem_cgroup_move_account(struct page *page, > page->mem_cgroup = to; > > #ifdef CONFIG_TRANSPARENT_HUGEPAGE > + spin_lock(&to->deferred_split_queue.split_queue_lock); > if (compound && list_empty(page_deferred_list(page))) { > - 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++; > - spin_unlock(&to->deferred_split_queue.split_queue_lock); > } > + spin_unlock(&to->deferred_split_queue.split_queue_lock); > #endif > > spin_unlock_irqrestore(&from->move_lock, flags); > --