Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Date:   Tue, 26 Sep 2023 13:47:33 -0700 (PDT)
From:   Hugh Dickins <hughd@google.com>
To:     Matthew Wilcox <willy@infradead.org>
cc:     Hugh Dickins <hughd@google.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Andi Kleen <ak@linux.intel.com>,
        Christoph Lameter <cl@linux.com>,
        Mike Kravetz <mike.kravetz@oracle.com>,
        David Hildenbrand <david@redhat.com>,
        Suren Baghdasaryan <surenb@google.com>,
        Yang Shi <shy828301@gmail.com>,
        Sidhartha Kumar <sidhartha.kumar@oracle.com>,
        Vishal Moola <vishal.moola@gmail.com>,
        Kefeng Wang <wangkefeng.wang@huawei.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Tejun Heo <tj@kernel.org>,
        Mel Gorman <mgorman@techsingularity.net>,
        Michal Hocko <mhocko@suse.com>, linux-kernel@vger.kernel.org,
        linux-mm@kvack.org
Subject: Re: [PATCH 03/12] mempolicy: fix migrate_pages(2) syscall return
 nr_failed
In-Reply-To: <ZRIIIFm5IMnkGh3T@casper.infradead.org>
Message-ID: <20de9b0-39fd-76bf-ea7f-3e9df0dd79d9@google.com>
References: <2d872cef-7787-a7ca-10e-9d45a64c80b4@google.com> <ddad2cee-cbad-7b5d-935a-59f961b7c3a@google.com> <ZRIIIFm5IMnkGh3T@casper.infradead.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Mon, 25 Sep 2023, Matthew Wilcox wrote:
> On Mon, Sep 25, 2023 at 01:24:02AM -0700, Hugh Dickins wrote:
> > "man 2 migrate_pages" says "On success migrate_pages() returns the number
> > of pages that could not be moved".  Although 5.3 and 5.4 commits fixed
> > mbind(MPOL_MF_STRICT|MPOL_MF_MOVE*) to fail with EIO when not all pages
> > could be moved (because some could not be isolated for migration),
> > migrate_pages(2) was left still reporting only those pages failing at the
> > migration stage, forgetting those failing at the earlier isolation stage.
> > 
> > Fix that by accumulating a long nr_failed count in struct queue_pages,
> > returned by queue_pages_range() when it's not returning an error, for
> > adding on to the nr_failed count from migrate_pages() in mm/migrate.c.
> > A count of pages?  It's more a count of folios, but changing it to pages
> > would entail more work (also in mm/migrate.c): does not seem justified.
> 
> I certainly see what you're saying.  If a folio is only partially mapped
> (in an extreme case, the VMA is PAGE_SIZE and maps one page of a 512-page
> folio), then setting nr_failed to folio_nr_pages() is misleading at best.

Actually, that wasn't what I was thinking when I said that: but thank you
for the comment, you've helped me to see that what I'm actually doing is
not what is claimed there.

What I was thinking, something I'm taking as an axiom, is that the units
of failure when isolating must match the units of failure when migrating,
whatever they are.

And migrate_pages(), the internal one, has this helpfully explicit comment:
 * Returns the number of {normal folio, large folio, hugetlb} that were not
 * migrated, or an error code. The number of large folio splits will be
 * considered as the number of non-migrated large folio, no matter how many
 * split folios of the large folio are migrated successfully.

(TBH I haven't spent long enough to actually understand what the second
sentence is saying: I do realize that splits complicate the issue, but the
function wouldn't be expected to return a "number of large folio splits"
anyway.  One day, I should work out what the code is actually doing, and
try to reword that sentence better.)

So above I was trying to say that migrate_pages(), the syscall, returns
that quantity: totalling the failed-isolation and failed-migration folios.

But you've alerted me to how in fact I'm doing an nr_failed++ for each
PTE of a failing-to-isolate folio, not as claimed.  It looks like I need
to record "qp->large" in the case of failure as well as success.  (And
then bother about when isolation fails on the first PTE, but succeeds
by the time of a later PTE? maybe, or maybe that just gets silly.)
I must fix that in v2.

> 
> > +static void queue_folios_pmd(pmd_t *pmd, spinlock_t *ptl, unsigned long addr,
> >  				unsigned long end, struct mm_walk *walk)
...
> > +	if (!(qp->flags & (MPOL_MF_MOVE | MPOL_MF_MOVE_ALL)) ||
> > +	    !vma_migratable(walk->vma) ||
> > +	    !migrate_folio_add(folio, qp->pagelist, qp->flags))
> > +		qp->nr_failed++;
> 
> However, I think here, we would do well to increment by HPAGE_PMD_NR.
> Or whatever equivalent is flavour of the week.

I *really* wanted to do that (and increment nr_failed PTE by PTE as I'm
doing, rather than as I claimed), and gave it some thought: but I don't
think it can be done - or not without abandoning the axiom (in which
case it's impossible to say what migrate_pages(2) is counting), or
adding a layer of complication which simply isn't justifiable.

Certainly we could change the definition of what migrate_pages(internal)
returns (though I haven't researched who depends on it: IIRC-long-ago
there's maybe only one other caller who cares, to update a stat); but
that still would not help.  Because whether migrate_pages(internal)
returns 1 or HPAGE_PMD_NR for an unmigratable and unsplittable THP, it
has no idea whether that THP got into the pagelist via a PMD or via one
or some number more of PTEs.  More info would have to be passed down
separately, folio by folio: an auxiliary xarray perhaps, but let's not.

If it turns out that I'm deluded, and it can be easily done, please
clarify one point: you made this comment on queue_folios_pmd(), but
what about queue_folios_hugetlb()?  Would you nowadays prefer hugetlb
to count 1 or folio_nr_pages()?  I think the latter.

> 
> Bravo to the other changes.

Thanks - I'm guessing your enthusiasm is mainly due to that "qp->large"
realization, which we ought to have thought of before.  I'm afraid it's
going to get more complicated, once COWs are feeding on Ryan's ALFalfA -
might need large[MAX_ORDER], or some better way.  But no great hurry,
nothing will crash if it's occasionally not-quite-right.

> 
> Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Many thanks for all these rapid and encouraging reviews.

Hugh