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@xxxxxxxxxxxxx>
Many thanks for all these rapid and encouraging reviews.
Hugh
