Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <1585892447-32059-1-git-send-email-iamjoonsoo.kim@lge.com>
 <1585892447-32059-6-git-send-email-iamjoonsoo.kim@lge.com>
 <CAHbLzkqdupWUv7vPpqDpOARuYkBiTxmQxNi-zaw_TWVB1FsNjQ@mail.gmail.com>
 <CAAmzW4Mr7NtOpPKpskhnMABJeVoe-CvOuC6TEfJabS7HdcwLbQ@mail.gmail.com> <CAHbLzkoL7zKOFtRghEfsfeKOERZmTkjfi8MynuHf4oKXD9mcvQ@mail.gmail.com>
In-Reply-To: <CAHbLzkoL7zKOFtRghEfsfeKOERZmTkjfi8MynuHf4oKXD9mcvQ@mail.gmail.com>
From:   Joonsoo Kim <js1304@gmail.com>
Date:   Tue, 7 Apr 2020 10:27:44 +0900
Message-ID: <CAAmzW4MNqyYqH+GbOE8Ardz2BNi5whHxP0FmwgjX1zPHNCXw_g@mail.gmail.com>
Subject: Re: [PATCH v5 05/10] mm/swap: charge the page when adding to the swap cache
To:     Yang Shi <shy828301@gmail.com>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        Linux MM <linux-mm@kvack.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Johannes Weiner <hannes@cmpxchg.org>,
        Michal Hocko <mhocko@kernel.org>,
        Hugh Dickins <hughd@google.com>,
        Minchan Kim <minchan@kernel.org>,
        Vlastimil Babka <vbabka@suse.cz>,
        Mel Gorman <mgorman@techsingularity.net>, kernel-team@lge.com,
        Joonsoo Kim <iamjoonsoo.kim@lge.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

2020=EB=85=84 4=EC=9B=94 7=EC=9D=BC (=ED=99=94) =EC=98=A4=EC=A0=84 9:22, Ya=
ng Shi <shy828301@gmail.com>=EB=8B=98=EC=9D=B4 =EC=9E=91=EC=84=B1:
>
> On Sun, Apr 5, 2020 at 6:03 PM Joonsoo Kim <js1304@gmail.com> wrote:
> >
> > 2020=EB=85=84 4=EC=9B=94 4=EC=9D=BC (=ED=86=A0) =EC=98=A4=EC=A0=84 3:29=
, Yang Shi <shy828301@gmail.com>=EB=8B=98=EC=9D=B4 =EC=9E=91=EC=84=B1:
> > >
> > > On Thu, Apr 2, 2020 at 10:41 PM <js1304@gmail.com> wrote:
> > > >
> > > > From: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> > > >
> > > > Currently, some swapped-in pages are not charged to the memcg until
> > > > actual access to the page happens. I checked the code and found tha=
t
> > > > it could cause a problem. In this implementation, even if the memcg
> > > > is enabled, one can consume a lot of memory in the system by exploi=
ting
> > > > this hole. For example, one can make all the pages swapped out and
> > > > then call madvise_willneed() to load the all swapped-out pages with=
out
> > > > pressing the memcg. Although actual access requires charging, it's =
really
> > > > big benefit to load the swapped-out pages to the memory without pre=
ssing
> > > > the memcg.
> > > >
> > > > And, for workingset detection which is implemented on the following=
 patch,
> > > > a memcg should be committed before the workingset detection is exec=
uted.
> > > > For this purpose, the best solution, I think, is charging the page =
when
> > > > adding to the swap cache. Charging there is not that hard. Caller o=
f
> > > > adding the page to the swap cache has enough information about the =
charged
> > > > memcg. So, what we need to do is just passing this information to
> > > > the right place.
> > > >
> > > > With this patch, specific memcg could be pressured more since reada=
head
> > > > pages are also charged to it now. This would result in performance
> > > > degradation to that user but it would be fair since that readahead =
is for
> > > > that user.
> > >
> > > If I read the code correctly, the readahead pages may be *not* charge=
d
> > > to it at all but other memcgs since mem_cgroup_try_charge() would
> > > retrieve the target memcg id from the swap entry then charge to it
> > > (generally it is the memcg from who the page is swapped out). So, it
> > > may open a backdoor to let one memcg stress other memcgs?
> >
> > It looks like you talk about the call path on CONFIG_MEMCG_SWAP.
> >
> > The owner (task) for a anonymous page cannot be changed. It means that
> > the previous owner written on the swap entry will be the next user. So,
> > I think that using the target memcg id from the swap entry for readahea=
d pages
> > is valid way.
> >
> > As you concerned, if someone can control swap-readahead to readahead
> > other's swap entry, one memcg could stress other memcg by using the fac=
t above.
> > However, as far as I know, there is no explicit way to readahead other'=
s swap
> > entry so no problem.
>
> Swap cluster readahead would readahead in pages on consecutive swap
> entries which may belong to different memcgs, however I just figured
> out patch #8 ("mm/swap: do not readahead if the previous owner of the
> swap entry isn't me") would prevent from reading ahead pages belonging
> to other memcgs. This would kill the potential problem.

Yes, that patch kill the potential problem. However, I think that swap clus=
ter
readahead would not open the backdoor even without the patch #8 in
CONFIG_MEMCG_SWAP case, because:

1. consecutive swap space is usually filled by the same task.
2. swap cluster readahead needs a large I/O price to the offender and effec=
t
isn't serious to the target.
3. those pages would be charged to their previous owner and it is valid.

Thanks.