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: <CAHbLzkr0AWWNMJ6i1551m75YfsVwdYuhLbW8sZqW_M-iH8vpBw@mail.gmail.com>
Date:   Fri, 9 Apr 2021 08:43:17 -0700
From:   Yang Shi <shy828301@...il.com>
To:     Oscar Salvador <osalvador@...e.de>
Cc:     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>, weixugc@...gle.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

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.

It is because nr_succeeded is reset for each migrate_pages() call.

You could do "*ret_succeeded += nr_succeeded" if we want an
accumulated counter, then you don't have to add total_succeeded. And
since nr_succeeded is reset for each migrate_pages() call, so both vm
counter and trace point are happy.

>
> 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.

I think the most straightforward concern is the vm counter and trace
point in migrate_pages(), if migrate_pages() is called multiple times
we may see messed up counters if nr_succeeded is not reset properly.
Of course both your and Wei's suggestion solve this problem.

But if we have usecase which returns nr_succeeded and call
migrate_pages() multiple times, I think we do want to return
accumulated value IMHO.

>
> But that won't happen neither with Wei Xu's version nor with mine.
>
> --
> Oscar Salvador
> SUSE L3