linux-kernel - Re: [PATCH 04/10] mm/migrate: make migrate_pages() return nr

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAAPL-u_7UWmAPpRvZJQ=FGMU=OTV+n5evzb-kjU1uCS9OpzwRg@mail.gmail.com>
Date:   Thu, 8 Apr 2021 22:43:31 -0700
From:   Wei Xu <weixugc@...gle.com>
To:     Oscar Salvador <osalvador@...e.de>
Cc:     Yang Shi <shy828301@...il.com>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Linux MM <linux-mm@...ck.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Yang Shi <yang.shi@...ux.alibaba.com>,
        Huang Ying <ying.huang@...el.com>,
        Dan Williams <dan.j.williams@...el.com>,
        David Hildenbrand <david@...hat.com>
Subject: Re: [PATCH 04/10] mm/migrate: make migrate_pages() return nr_succeeded

I agree that it is a good further improvement to make nr_succeeded an optional
output argument of migrate_pages() given that most callers don't need it.  IMHO,
the most important thing in this matter is to ensure that nr_succeeded only
returns (when its return value is needed) the successfully migrated
pages in this
round and doesn't accumulate.  This is addressed by both proposals.

On Thu, Apr 8, 2021 at 10:06 PM Oscar Salvador <osalvador@...e.de> wrote:
>
> On Thu, Apr 08, 2021 at 01:40:33PM -0700, Yang Shi wrote:
> > Thanks a lot for the example code. You didn't miss anything. At first
> > glance, I thought your suggestion seemed neater. Actually I
> > misunderstood what Dave said about "That could really have caused some
> > interesting problems." with multiple calls to migrate_pages(). I was
> > thinking about:
> >
> > unsigned long foo()
> > {
> >     unsigned long *ret_succeeded;
> >
> >     migrate_pages(..., ret_succeeded);
> >
> >     migrate_pages(..., ret_succeeded);
> >
> >     return *ret_succeeded;
> > }
>
> But that would not be a problem as well. I mean I am not sure what is
> foo() supposed to do.
> I assume is supposed to return the *total* number of pages that were
> migrated?
>
> Then could do something like:
>
>  unsigned long foo()
>  {
>      unsigned long ret_succeeded;
>      unsigned long total_succeeded = 0;
>
>      migrate_pages(..., &ret_succeeded);
>      total_succeeded += ret_succeeded;
>
>      migrate_pages(..., &ret_succeeded);
>      total_succeeded += ret_succeeded;
>
>      return *total_succeeded;
>  }
>
>  But AFAICS, you would have to do that with Wei Xu's version and with
>  mine, no difference there.
>
> IIUC, Dave's concern was that nr_succeeded was only set to 0 at the beginning
> of the function, and never reset back, which means, we would carry the
> sum of previous nr_succeeded instead of the nr_succeeded in that round.
> That would be misleading for e.g: reclaim in case we were to call
> migrate_pages() several times, as instead of a delta value, nr_succeeded
> would accumulate.
>
> But that won't happen neither with Wei Xu's version nor with mine.
>
> --
> Oscar Salvador
> SUSE L3