Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <1595681998-19193-1-git-send-email-alex.shi@linux.alibaba.com>
 <1595681998-19193-22-git-send-email-alex.shi@linux.alibaba.com>
 <CAKgT0UfpHjBTHvtZz7=WMhZZAunVYuNMpuYBQCiorERb5seFUQ@mail.gmail.com> <f34e790f-50e6-112c-622f-d7ab804c6d22@linux.alibaba.com>
In-Reply-To: <f34e790f-50e6-112c-622f-d7ab804c6d22@linux.alibaba.com>
From:   Alexander Duyck <alexander.duyck@gmail.com>
Date:   Tue, 4 Aug 2020 07:29:58 -0700
Message-ID: <CAKgT0UckqbmYJDE3L2Bg1Nr=Y=GT0OBx1GEhaZ14EbRTzd8tiw@mail.gmail.com>
Subject: Re: [PATCH v17 21/21] mm/lru: revise the comments of lru_lock
To:     Alex Shi <alex.shi@linux.alibaba.com>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        Mel Gorman <mgorman@techsingularity.net>,
        Tejun Heo <tj@kernel.org>, Hugh Dickins <hughd@google.com>,
        Konstantin Khlebnikov <khlebnikov@yandex-team.ru>,
        Daniel Jordan <daniel.m.jordan@oracle.com>,
        Yang Shi <yang.shi@linux.alibaba.com>,
        Matthew Wilcox <willy@infradead.org>,
        Johannes Weiner <hannes@cmpxchg.org>,
        kbuild test robot <lkp@intel.com>,
        linux-mm <linux-mm@kvack.org>,
        LKML <linux-kernel@vger.kernel.org>, cgroups@vger.kernel.org,
        Shakeel Butt <shakeelb@google.com>,
        Joonsoo Kim <iamjoonsoo.kim@lge.com>,
        Wei Yang <richard.weiyang@gmail.com>,
        "Kirill A. Shutemov" <kirill@shutemov.name>,
        Rong Chen <rong.a.chen@intel.com>,
        Andrey Ryabinin <aryabinin@virtuozzo.com>,
        Jann Horn <jannh@google.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Aug 4, 2020 at 3:04 AM Alex Shi <alex.shi@linux.alibaba.com> wrote:
>
>
>
> =E5=9C=A8 2020/8/4 =E4=B8=8A=E5=8D=886:37, Alexander Duyck =E5=86=99=E9=
=81=93:
> >>
> >>  shrink_inactive_list() also diverts any unevictable pages that it fin=
ds on the
> >> -inactive lists to the appropriate zone's unevictable list.
> >> +inactive lists to the appropriate node's unevictable list.
> >>
> >>  shrink_inactive_list() should only see SHM_LOCK'd pages that became S=
HM_LOCK'd
> >>  after shrink_active_list() had moved them to the inactive list, or pa=
ges mapped
> > Same here.
>
> lruvec is used per memcg per node actually, and it fallback to node if me=
mcg disabled.
> So the comments are still right.
>
> And most of changes just fix from zone->lru_lock to pgdat->lru_lock chang=
e.

Actually in my mind one thing that might work better would be to
explain what the lruvec is and where it resides. Then replace zone
with lruvec since that is really where the unevictable list resides.
Then it would be correct for both the memcg and pgdat case.

> >
> >> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> >> index 64ede5f150dc..44738cdb5a55 100644
> >> --- a/include/linux/mm_types.h
> >> +++ b/include/linux/mm_types.h
> >> @@ -78,7 +78,7 @@ struct page {
> >>                 struct {        /* Page cache and anonymous pages */
> >>                         /**
> >>                          * @lru: Pageout list, eg. active_list protect=
ed by
> >> -                        * pgdat->lru_lock.  Sometimes used as a gener=
ic list
> >> +                        * lruvec->lru_lock.  Sometimes used as a gene=
ric list
> >>                          * by the page owner.
> >>                          */
> >>                         struct list_head lru;
> >> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> >> index 8af956aa13cf..c92289a4e14d 100644
> >> --- a/include/linux/mmzone.h
> >> +++ b/include/linux/mmzone.h
> >> @@ -115,7 +115,7 @@ static inline bool free_area_empty(struct free_are=
a *area, int migratetype)
> >>  struct pglist_data;
> >>
> >>  /*
> >> - * zone->lock and the zone lru_lock are two of the hottest locks in t=
he kernel.
> >> + * zone->lock and the lru_lock are two of the hottest locks in the ke=
rnel.
> >>   * So add a wild amount of padding here to ensure that they fall into=
 separate
> >>   * cachelines.  There are very few zone structures in the machine, so=
 space
> >>   * consumption is not a concern here.
> > So I don't believe you are using ZONE_PADDING in any way to try and
> > protect the LRU lock currently. At least you aren't using it in the
> > lruvec. As such it might make sense to just drop the reference to the
> > lru_lock here. That reminds me that we still need to review the
> > placement of the lru_lock and determine if there might be a better
> > placement and/or padding that might improve performance when under
> > heavy stress.
> >
>
> Right, is it the following looks better?
>
> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> index ccc76590f823..0ed520954843 100644
> --- a/include/linux/mmzone.h
> +++ b/include/linux/mmzone.h
> @@ -113,8 +113,7 @@ static inline bool free_area_empty(struct free_area *=
area, int migratetype)
>  struct pglist_data;
>
>  /*
> - * zone->lock and the lru_lock are two of the hottest locks in the kerne=
l.
> - * So add a wild amount of padding here to ensure that they fall into se=
parate
> + * Add a wild amount of padding here to ensure datas fall into separate
>   * cachelines.  There are very few zone structures in the machine, so sp=
ace
>   * consumption is not a concern here.
>   */
>
> Thanks!
> Alex

I would maybe tweak it to make sure it is clear that we are using this
to pad out items that are likely to cause cache thrash such as various
hot spinocks and such.