Messages in this thread | | | Date | Wed, 13 Mar 2024 10:33:54 +0800 | Subject | Re: [PATCH v2] mm/migrate: put dest folio on deferred split list if source was there. | From | "Yin, Fengwei" <> |
| |
On 3/13/2024 10:07 AM, Yin, Fengwei wrote: > > > On 3/13/2024 2:46 AM, Matthew Wilcox wrote: >> On Tue, Mar 12, 2024 at 02:32:43PM -0400, Zi Yan wrote: >>> On 12 Mar 2024, at 12:38, Matthew Wilcox wrote: >>>> Folios with a positive refcount are >>>> removed from the per-node or per-cgroup list _at which point there is >>>> an undocumented assumption_ that they will not be removed from the >>>> local list because they have a positive refcount. >>> >>> But that sounds very subtle if not broken. As an outsider of >> >> I merely deduced this requirement; I didn't come up with it ... > My understanding is that this requirement is because of just local > list in deferred_split_scan(). > > Using fbatch instead of local list here as your created for that > issue debugging can eliminate this subtlety? May not good idea because it's possible the folios in fbatch can be removed from deferred_list by migration.
> > > Regards > Yin, Fengwei > >> >>> deferred_split_scan(), only !list_empty(folio->_deferred_list) is >>> checked. >>> The condition can be true if the folio is on split_queue or >>> local list of deferred_split_scan() with elevated refcount. In that >>> case, >>> the folio cannot be removed from the list (either split_queue or >>> local list) >>> even if split_queue_lock is held, since local list manipulation is >>> not under >>> split_queue_lock. This makes _deferred_list a one-way train to anyone >>> except deferred_split_scan(), namely folios can only be added into >>> _deferred_list until they are freed or split by deferred_split_scan(). >>> >>> Is that intended? If yes, maybe we should document it. If not, using >>> split_queue_lock to protect local list, or more explicitly >>> folio->_deferred_list >>> might be better? >> >> To be fair, the folio can be split by anybody as >> split_huge_page_to_list_to_order() is careful to only manipulate the >> deferred list while the refcount is frozen at 0. I'm still trying to >> figure out where to document this behaviour of the deferred list that >> someone (for example, your good self) would actually see it. >> >> >> >
| |