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=-4.1 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=no 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 D008DC433E1 for ; Tue, 28 Jul 2020 14:54:15 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id AD703206F5 for ; Tue, 28 Jul 2020 14:54:15 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="t5IBgxDm" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1730519AbgG1OyO (ORCPT ); Tue, 28 Jul 2020 10:54:14 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:39412 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1730455AbgG1OyN (ORCPT ); Tue, 28 Jul 2020 10:54:13 -0400 Received: from mail-io1-xd41.google.com (mail-io1-xd41.google.com [IPv6:2607:f8b0:4864:20::d41]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 748C8C061794; Tue, 28 Jul 2020 07:54:13 -0700 (PDT) Received: by mail-io1-xd41.google.com with SMTP id q75so12878716iod.1; Tue, 28 Jul 2020 07:54:13 -0700 (PDT) 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:content-transfer-encoding; bh=rlXEKJ8xEIOgNHiN29J7TVPDIEGm7u8gbZy3u1s9Ctw=; b=t5IBgxDmqzCT6sfkH5/d8FUEdSDdI2dJz1S8qZ1ZiU+8a6LEYgoUdHC7V+GMiMw/5Y TVjf/4KKXxfObmWFzed1Ov64eHn8wDiSC4LJTQC1LV6suKmQw72Wnud2TRs6QafppeYq +RTfvgb55wEeFd2AOM4jJHQOkfgm44l0ROWwTVjPmaW4J3ZwHRF747lsegTE9kOxt2nc lG30esH922EGIPfNLuuuNh+56AJL4x1Zpi8P2J2tfhf/C2+Fpm+kBa9PXCpksfIPy6wv GezNC01nzIpMgnSJn9ve9dutdqJpKucoQxvvzUOJrLX4Sr1u7TwJZ78crHveaaYoHnBY ydWA== 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:content-transfer-encoding; bh=rlXEKJ8xEIOgNHiN29J7TVPDIEGm7u8gbZy3u1s9Ctw=; b=UsxwC6WxKpFg0PRTJcKKF7Ei2X0jpRsh/s1SWw6D0B37XR9wEhlcPid9uPHx08C4Zm 9YEMJc8C4g68rO+R0v6ESKFdQv7v0fBggcXqlW4JzFgH6SK5VyrLt5TN8M1lijuST8Zk XxWy/veAqmGBJqldRQ1ybOXKeJ2EZ88o5yxRmtClQn8m7jOM1xfD4SM7w4tvsY81QE8G xGEr86iGhNhwUGST27ETSqU4lCApgOTLpkTTPQLBDXlSTCYKtuhM3nyvK3OZTRQh8sa7 pSYMXyuY4ZmieGM5xXt5hDvY3/yZxnW4A8qRwLITWXfB8RfH6ZOUKB2qOafc82nwH1GD ZDSw== X-Gm-Message-State: AOAM530P/wb6g7Z6tkrt4W/7/FxhZZJrbTXx3w/RytrukDwhD016mtxk kbVVSmOV/LAEqlTKu8/fNpHevpKXivBz0ZIqpGs= X-Google-Smtp-Source: ABdhPJzsjlGbnYWKWKFNjvkbjOng3dZaS0dcx1SQ26KWeacdxavQUtITJe1QdcIRNEyS9V0x9BCYJakotwCelSWEajY= X-Received: by 2002:a05:6638:771:: with SMTP id y17mr32711779jad.96.1595948052581; Tue, 28 Jul 2020 07:54:12 -0700 (PDT) MIME-Version: 1.0 References: <1595681998-19193-1-git-send-email-alex.shi@linux.alibaba.com> <1595681998-19193-18-git-send-email-alex.shi@linux.alibaba.com> In-Reply-To: From: Alexander Duyck Date: Tue, 28 Jul 2020 07:54:01 -0700 Message-ID: Subject: Re: [PATCH v17 17/21] mm/lru: replace pgdat lru_lock with lruvec lock To: Alex Shi Cc: Andrew Morton , Mel Gorman , Tejun Heo , Hugh Dickins , Konstantin Khlebnikov , Daniel Jordan , Yang Shi , Matthew Wilcox , Johannes Weiner , kbuild test robot , linux-mm , LKML , cgroups@vger.kernel.org, Shakeel Butt , Joonsoo Kim , Wei Yang , "Kirill A. Shutemov" , Rong Chen , Michal Hocko , Vladimir Davydov Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Jul 28, 2020 at 4:20 AM Alex Shi wrote= : > > > > =E5=9C=A8 2020/7/28 =E4=B8=8A=E5=8D=887:34, Alexander Duyck =E5=86=99=E9= =81=93: > >> @@ -1876,6 +1876,12 @@ static unsigned noinline_for_stack move_pages_t= o_lru(struct lruvec *lruvec, > >> * list_add(&pa= ge->lru,) > >> * list_add(&page->lru,) //corrupt > >> */ > >> + new_lruvec =3D mem_cgroup_page_lruvec(page, page_pgdat= (page)); > >> + if (new_lruvec !=3D lruvec) { > >> + if (lruvec) > >> + spin_unlock_irq(&lruvec->lru_lock); > >> + lruvec =3D lock_page_lruvec_irq(page); > >> + } > >> SetPageLRU(page); > >> > >> if (unlikely(put_page_testzero(page))) { > > I was going through the code of the entire patch set and I noticed > > these changes in move_pages_to_lru. What is the reason for adding the > > new_lruvec logic? My understanding is that we are moving the pages to > > the lruvec provided are we not?If so why do we need to add code to get > > a new lruvec? The code itself seems to stand out from the rest of the > > patch as it is introducing new code instead of replacing existing > > locking code, and it doesn't match up with the description of what > > this function is supposed to do since it changes the lruvec. > > this new_lruvec is the replacement of removed line, as following code: > >> - lruvec =3D mem_cgroup_page_lruvec(page, pgdat); > This recheck is for the page move the root memcg, otherwise it cause the = bug: Okay, now I see where the issue is. You moved this code so now it has a different effect than it did before. You are relocking things before you needed to. Don't forget that when you came into this function you already had the lock. In addition the patch is broken as it currently stands as you aren't using similar logic in the code just above this addition if you encounter an evictable page. As a result this is really difficult to review as there are subtle bugs here. I suppose the correct fix is to get rid of this line, but it should be placed everywhere the original function was calling spin_lock_irq(). In addition I would consider changing the arguments/documentation for move_pages_to_lru. You aren't moving the pages to lruvec, so there is probably no need to pass that as an argument. Instead I would pass pgdat since that isn't going to be moving and is the only thing you actually derive based on the original lruvec. 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=-4.1 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=no 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 75562C433E0 for ; Tue, 28 Jul 2020 14:54:15 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 35EDE20786 for ; Tue, 28 Jul 2020 14:54:15 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="t5IBgxDm" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 35EDE20786 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 C903B6B0095; Tue, 28 Jul 2020 10:54:14 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id C411A6B0096; Tue, 28 Jul 2020 10:54:14 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id B58CF8D0005; Tue, 28 Jul 2020 10:54:14 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0091.hostedemail.com [216.40.44.91]) by kanga.kvack.org (Postfix) with ESMTP id A007F6B0095 for ; Tue, 28 Jul 2020 10:54:14 -0400 (EDT) Received: from smtpin26.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with ESMTP id 11DA633CD for ; Tue, 28 Jul 2020 14:54:14 +0000 (UTC) X-FDA: 77087780028.26.shop11_520f83826f6a Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin26.hostedemail.com (Postfix) with ESMTP id CC64D1804B65A for ; Tue, 28 Jul 2020 14:54:13 +0000 (UTC) X-HE-Tag: shop11_520f83826f6a X-Filterd-Recvd-Size: 6300 Received: from mail-io1-f65.google.com (mail-io1-f65.google.com [209.85.166.65]) by imf11.hostedemail.com (Postfix) with ESMTP for ; Tue, 28 Jul 2020 14:54:13 +0000 (UTC) Received: by mail-io1-f65.google.com with SMTP id s189so13782358iod.2 for ; Tue, 28 Jul 2020 07:54:13 -0700 (PDT) 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:content-transfer-encoding; bh=rlXEKJ8xEIOgNHiN29J7TVPDIEGm7u8gbZy3u1s9Ctw=; b=t5IBgxDmqzCT6sfkH5/d8FUEdSDdI2dJz1S8qZ1ZiU+8a6LEYgoUdHC7V+GMiMw/5Y TVjf/4KKXxfObmWFzed1Ov64eHn8wDiSC4LJTQC1LV6suKmQw72Wnud2TRs6QafppeYq +RTfvgb55wEeFd2AOM4jJHQOkfgm44l0ROWwTVjPmaW4J3ZwHRF747lsegTE9kOxt2nc lG30esH922EGIPfNLuuuNh+56AJL4x1Zpi8P2J2tfhf/C2+Fpm+kBa9PXCpksfIPy6wv GezNC01nzIpMgnSJn9ve9dutdqJpKucoQxvvzUOJrLX4Sr1u7TwJZ78crHveaaYoHnBY ydWA== 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:content-transfer-encoding; bh=rlXEKJ8xEIOgNHiN29J7TVPDIEGm7u8gbZy3u1s9Ctw=; b=qbuGroGQaaSuMDHDl9jpQuqz8MBZ0dMTjGEQ+ZpyVpPTCZ3zdCDPhvksuX6fZK/CI6 dpojHq2SV0vg0yflgKeOj/L4hT8QsSJ/hT1rsrrqxJpDj/wcy6vhD+0VY5sw2kYsaDIX v50QYADBn2jK8F6Mr55B/cGAYNpJJKB9TkurUZi6MKDcLojfUfu/czHMmJ3qQK6laJ5p 7id9l3vZq8t9wBq8naglyLy+RznqklgX1cNrdNW6oXgY54t+vCSX5gAd5Pt1Cyr7WSoE etPAbPGELIyTbilrtx+6GjKQTPGRGCdvDXIgQcmB8YN5tNe/hRtL/Wa/mfAGDCtivX5m VQpQ== X-Gm-Message-State: AOAM53275ikhET85mZXRMSrEJS66idIg5GaqmuvU9K/6Ds8nDbq8o/KS XgZpyOrBCBmaVw1p0gVWbHKxtlF4Qzkqx3yFc04= X-Google-Smtp-Source: ABdhPJzsjlGbnYWKWKFNjvkbjOng3dZaS0dcx1SQ26KWeacdxavQUtITJe1QdcIRNEyS9V0x9BCYJakotwCelSWEajY= X-Received: by 2002:a05:6638:771:: with SMTP id y17mr32711779jad.96.1595948052581; Tue, 28 Jul 2020 07:54:12 -0700 (PDT) MIME-Version: 1.0 References: <1595681998-19193-1-git-send-email-alex.shi@linux.alibaba.com> <1595681998-19193-18-git-send-email-alex.shi@linux.alibaba.com> In-Reply-To: From: Alexander Duyck Date: Tue, 28 Jul 2020 07:54:01 -0700 Message-ID: Subject: Re: [PATCH v17 17/21] mm/lru: replace pgdat lru_lock with lruvec lock To: Alex Shi Cc: Andrew Morton , Mel Gorman , Tejun Heo , Hugh Dickins , Konstantin Khlebnikov , Daniel Jordan , Yang Shi , Matthew Wilcox , Johannes Weiner , kbuild test robot , linux-mm , LKML , cgroups@vger.kernel.org, Shakeel Butt , Joonsoo Kim , Wei Yang , "Kirill A. Shutemov" , Rong Chen , Michal Hocko , Vladimir Davydov Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: CC64D1804B65A X-Spamd-Result: default: False [0.00 / 100.00] X-Rspamd-Server: rspam03 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 Tue, Jul 28, 2020 at 4:20 AM Alex Shi wrote= : > > > > =E5=9C=A8 2020/7/28 =E4=B8=8A=E5=8D=887:34, Alexander Duyck =E5=86=99=E9= =81=93: > >> @@ -1876,6 +1876,12 @@ static unsigned noinline_for_stack move_pages_t= o_lru(struct lruvec *lruvec, > >> * list_add(&pa= ge->lru,) > >> * list_add(&page->lru,) //corrupt > >> */ > >> + new_lruvec =3D mem_cgroup_page_lruvec(page, page_pgdat= (page)); > >> + if (new_lruvec !=3D lruvec) { > >> + if (lruvec) > >> + spin_unlock_irq(&lruvec->lru_lock); > >> + lruvec =3D lock_page_lruvec_irq(page); > >> + } > >> SetPageLRU(page); > >> > >> if (unlikely(put_page_testzero(page))) { > > I was going through the code of the entire patch set and I noticed > > these changes in move_pages_to_lru. What is the reason for adding the > > new_lruvec logic? My understanding is that we are moving the pages to > > the lruvec provided are we not?If so why do we need to add code to get > > a new lruvec? The code itself seems to stand out from the rest of the > > patch as it is introducing new code instead of replacing existing > > locking code, and it doesn't match up with the description of what > > this function is supposed to do since it changes the lruvec. > > this new_lruvec is the replacement of removed line, as following code: > >> - lruvec =3D mem_cgroup_page_lruvec(page, pgdat); > This recheck is for the page move the root memcg, otherwise it cause the = bug: Okay, now I see where the issue is. You moved this code so now it has a different effect than it did before. You are relocking things before you needed to. Don't forget that when you came into this function you already had the lock. In addition the patch is broken as it currently stands as you aren't using similar logic in the code just above this addition if you encounter an evictable page. As a result this is really difficult to review as there are subtle bugs here. I suppose the correct fix is to get rid of this line, but it should be placed everywhere the original function was calling spin_lock_irq(). In addition I would consider changing the arguments/documentation for move_pages_to_lru. You aren't moving the pages to lruvec, so there is probably no need to pass that as an argument. Instead I would pass pgdat since that isn't going to be moving and is the only thing you actually derive based on the original lruvec. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alexander Duyck Subject: Re: [PATCH v17 17/21] mm/lru: replace pgdat lru_lock with lruvec lock Date: Tue, 28 Jul 2020 07:54:01 -0700 Message-ID: References: <1595681998-19193-1-git-send-email-alex.shi@linux.alibaba.com> <1595681998-19193-18-git-send-email-alex.shi@linux.alibaba.com> Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Return-path: 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:content-transfer-encoding; bh=rlXEKJ8xEIOgNHiN29J7TVPDIEGm7u8gbZy3u1s9Ctw=; b=t5IBgxDmqzCT6sfkH5/d8FUEdSDdI2dJz1S8qZ1ZiU+8a6LEYgoUdHC7V+GMiMw/5Y TVjf/4KKXxfObmWFzed1Ov64eHn8wDiSC4LJTQC1LV6suKmQw72Wnud2TRs6QafppeYq +RTfvgb55wEeFd2AOM4jJHQOkfgm44l0ROWwTVjPmaW4J3ZwHRF747lsegTE9kOxt2nc lG30esH922EGIPfNLuuuNh+56AJL4x1Zpi8P2J2tfhf/C2+Fpm+kBa9PXCpksfIPy6wv GezNC01nzIpMgnSJn9ve9dutdqJpKucoQxvvzUOJrLX4Sr1u7TwJZ78crHveaaYoHnBY ydWA== In-Reply-To: Sender: cgroups-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-ID: Content-Type: text/plain; charset="utf-8" To: Alex Shi Cc: Andrew Morton , Mel Gorman , Tejun Heo , Hugh Dickins , Konstantin Khlebnikov , Daniel Jordan , Yang Shi , Matthew Wilcox , Johannes Weiner , kbuild test robot , linux-mm , LKML , cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Shakeel Butt , Joonsoo Kim , Wei Yang , "Kirill A. Shutemov" , Rong Chen , Michal Hocko , Vladimir Davydov On Tue, Jul 28, 2020 at 4:20 AM Alex Shi wrote= : > > > > =E5=9C=A8 2020/7/28 =E4=B8=8A=E5=8D=887:34, Alexander Duyck =E5=86=99=E9= =81=93: > >> @@ -1876,6 +1876,12 @@ static unsigned noinline_for_stack move_pages_t= o_lru(struct lruvec *lruvec, > >> * list_add(&pa= ge->lru,) > >> * list_add(&page->lru,) //corrupt > >> */ > >> + new_lruvec =3D mem_cgroup_page_lruvec(page, page_pgdat= (page)); > >> + if (new_lruvec !=3D lruvec) { > >> + if (lruvec) > >> + spin_unlock_irq(&lruvec->lru_lock); > >> + lruvec =3D lock_page_lruvec_irq(page); > >> + } > >> SetPageLRU(page); > >> > >> if (unlikely(put_page_testzero(page))) { > > I was going through the code of the entire patch set and I noticed > > these changes in move_pages_to_lru. What is the reason for adding the > > new_lruvec logic? My understanding is that we are moving the pages to > > the lruvec provided are we not?If so why do we need to add code to get > > a new lruvec? The code itself seems to stand out from the rest of the > > patch as it is introducing new code instead of replacing existing > > locking code, and it doesn't match up with the description of what > > this function is supposed to do since it changes the lruvec. > > this new_lruvec is the replacement of removed line, as following code: > >> - lruvec =3D mem_cgroup_page_lruvec(page, pgdat); > This recheck is for the page move the root memcg, otherwise it cause the = bug: Okay, now I see where the issue is. You moved this code so now it has a different effect than it did before. You are relocking things before you needed to. Don't forget that when you came into this function you already had the lock. In addition the patch is broken as it currently stands as you aren't using similar logic in the code just above this addition if you encounter an evictable page. As a result this is really difficult to review as there are subtle bugs here. I suppose the correct fix is to get rid of this line, but it should be placed everywhere the original function was calling spin_lock_irq(). In addition I would consider changing the arguments/documentation for move_pages_to_lru. You aren't moving the pages to lruvec, so there is probably no need to pass that as an argument. Instead I would pass pgdat since that isn't going to be moving and is the only thing you actually derive based on the original lruvec.