Message-ID: <CAHbLzkoL7zKOFtRghEfsfeKOERZmTkjfi8MynuHf4oKXD9mcvQ@mail.gmail.com>
Date:   Mon, 6 Apr 2020 17:22:28 -0700
From:   Yang Shi <shy828301@...il.com>
To:     Joonsoo Kim <js1304@...il.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Linux MM <linux-mm@...ck.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Johannes Weiner <hannes@...xchg.org>,
        Michal Hocko <mhocko@...nel.org>,
        Hugh Dickins <hughd@...gle.com>,
        Minchan Kim <minchan@...nel.org>,
        Vlastimil Babka <vbabka@...e.cz>,
        Mel Gorman <mgorman@...hsingularity.net>, kernel-team@....com,
        Joonsoo Kim <iamjoonsoo.kim@....com>
Subject: Re: [PATCH v5 05/10] mm/swap: charge the page when adding to the swap cache

On Sun, Apr 5, 2020 at 6:03 PM Joonsoo Kim <js1304@...il.com> wrote:
>
> 2020년 4월 4일 (토) 오전 3:29, Yang Shi <shy828301@...il.com>님이 작성:
> >
> > On Thu, Apr 2, 2020 at 10:41 PM <js1304@...il.com> wrote:
> > >
> > > From: Joonsoo Kim <iamjoonsoo.kim@....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 that
> > > 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 exploiting
> > > this hole. For example, one can make all the pages swapped out and
> > > then call madvise_willneed() to load the all swapped-out pages without
> > > pressing the memcg. Although actual access requires charging, it's really
> > > big benefit to load the swapped-out pages to the memory without pressing
> > > the memcg.
> > >
> > > And, for workingset detection which is implemented on the following patch,
> > > a memcg should be committed before the workingset detection is executed.
> > > 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 of
> > > 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 readahead
> > > 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* charged
> > 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 readahead 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 fact 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.

> Thanks.

Powered by blists - more mailing lists