On Wed, Nov 19, 2025 at 5:06 AM Lorenzo Stoakes <[email protected]> wrote: > > On Wed, Oct 22, 2025 at 12:37:15PM -0600, Nico Pache wrote: > > There are cases where, if an attempted collapse fails, all subsequent > > orders are guaranteed to also fail. Avoid these collapse attempts by > > bailing out early. > > > > Signed-off-by: Nico Pache <[email protected]> > > --- > > mm/khugepaged.c | 31 ++++++++++++++++++++++++++++++- > > 1 file changed, 30 insertions(+), 1 deletion(-) > > > > diff --git a/mm/khugepaged.c b/mm/khugepaged.c > > index e2319bfd0065..54f5c7888e46 100644 > > --- a/mm/khugepaged.c > > +++ b/mm/khugepaged.c > > @@ -1431,10 +1431,39 @@ static int collapse_scan_bitmap(struct mm_struct > > *mm, unsigned long address, > > ret = collapse_huge_page(mm, address, referenced, > > unmapped, cc, mmap_locked, > > order, offset); > > - if (ret == SCAN_SUCCEED) { > > + > > + /* > > + * Analyze failure reason to determine next action: > > + * - goto next_order: try smaller orders in same > > region > > + * - continue: try other regions at same order > > The stack is a DFS, so this isn't correct, you may have pushed a bunch of > higher > order candidate mTHPs (I don't like plain 'region') which you will also true. > > > + * - break: stop all attempts (system-wide failure) > > + */ > > This comment isn't hugely helpful, just put the relevant comments next to each > of the goto, continue, break (soon to be return re: review below) statements > please. > > > + switch (ret) { > > + /* Cases were we should continue to the next region */ > > + case SCAN_SUCCEED: > > collapsed += 1UL << order; > > + fallthrough; > > + case SCAN_PTE_MAPPED_HUGEPAGE: > > continue; > > Would we not run into trouble potentially in the previous patch's > implementation > of this examing candidate mTHPs that are part of an already existing huge > page, > or would a folio check in the collapse just make this wasted work?
whoops almost missed this comment. There is a folio check in the __collapse_huge_page_isolate function that handles this. I think Dev has some plans to try and add partially-mapped support as the todo comment suggests (I think he already has some patches from earlier mTHP work). /* * TODO: In some cases of partially-mapped folios, we'd actually * want to collapse. */ > > > + /* Cases were lower orders might still succeed */ > > + case SCAN_LACK_REFERENCED_PAGE: > > + case SCAN_EXCEED_NONE_PTE: > > How can we, having checked the max_pte_none, still fail due to this? > > > + case SCAN_EXCEED_SWAP_PTE: > > + case SCAN_EXCEED_SHARED_PTE: > > + case SCAN_PAGE_LOCK: > > + case SCAN_PAGE_COUNT: > > + case SCAN_PAGE_LRU: > > + case SCAN_PAGE_NULL: > > + case SCAN_DEL_PAGE_LRU: > > + case SCAN_PTE_NON_PRESENT: > > + case SCAN_PTE_UFFD_WP: > > + case SCAN_ALLOC_HUGE_PAGE_FAIL: > > + goto next_order; > > + /* All other cases should stop collapse attempts */ > > I don't love us having a catch-all, plase spell out all cases. > > > + default: > > + break; > > } > > + break; > > _Hate_ this double break. Just return collapsed please. > > > } > > > > next_order: > > -- > > 2.51.0 > > >
