On 6 Mar 2025, at 4:19, David Hildenbrand wrote: > On 05.03.25 22:08, Zi Yan wrote: >> On 5 Mar 2025, at 15:50, Hugh Dickins wrote: >> >>> On Wed, 5 Mar 2025, Zi Yan wrote: >>>> On 4 Mar 2025, at 6:49, Hugh Dickins wrote: >>>>> >>>>> I think (might be wrong, I'm in a rush) my mods are all to this >>>>> "add two new (not yet used) functions for folio_split()" patch: >>>>> please merge them in if you agree. >>>>> >>>>> 1. From source inspection, it looks like a folio_set_order() was missed. >>>> >>>> Actually no. folio_set_order(folio, new_order) is called multiple times >>>> in the for loop above. It is duplicated but not missing. >>> >>> I was about to disagree with you, when at last I saw that, yes, >>> it is doing that on "folio" at the time of setting up "new_folio". >>> >>> That is confusing: in all other respects, that loop is reading folio >>> to set up new_folio. Do you have a reason for doing it there? >> >> No. I agree your fix is better. Just point out folio_set_order() should >> not trigger a bug. >> >>> >>> The transient "nested folio" situation is anomalous either way. >>> I'd certainly prefer it to be done at the point where you >>> ClearPageCompound when !new_order; but if you think there's an issue >>> with racing isolate_migratepages_block() or something like that, which >>> your current placement handles better, then please add a line of comment >>> both where you do it and where I expected to find it - thanks. >> >> Sure. I will use your patch unless I find some racing issue. >> >>> >>> (Historically, there was quite a lot of difficulty in getting the order >>> of events in __split_huge_page_tail() to be safe: I wonder whether we >>> shall see a crop of new weird bugs from these changes. I note that your >>> loops advance forwards, whereas the old ones went backwards: but I don't >>> have anything to say you're wrong. I think it's mainly a matter of how >>> the first tail or two gets handled: which might be why you want to >>> folio_set_order(folio, new_order) at the earliest opportunity.) >> >> I am worried about that too. In addition, in __split_huge_page_tail(), >> page refcount is restored right after new tail folio split is done, >> whereas I needed to delay them until all new after-split folios >> are done, since non-uniform split is iterative and only the after-split >> folios NOT containing the split_at page will be released. These >> folios are locked and frozen after __split_folio_to_order() like >> the original folio. Maybe because there are more such locked frozen >> folios than before? > > What's the general concern here? > > A frozen folio cannot be referenced and consequently not trusted. For example, if we want to speculatively lookup a folio in the pagecache and find it to be frozen, we'll have to spin (retry) until we find a folio that is unfrozen. > > While a folio has a refcount of 0, there are no guarantees. It could change its size, it could be freed + reallocated (changed mapping etc) ... > > So whoever wants to grab a speculative reference -- using folio_try_get() -- must re-verify folio properties after grabbing the speculative reference succeeded. Including whether it is small/large, number of pages, mapping, ... > > The important part is to unfreeze a folio only once it was fully prepared (e.g., order set, compound pages links to head set up etc). > > I am not sure if the sequence in which you process folios during a split matters here when doing a split: only that, whatever new folio is unfrozen, is properly initialized. Got it. Thanks for the confirmation. My worry came from that after I rebased on mm-everything-2025-03-05-03-54, which does not have folio_split() patches, I see a crash saying a buddy page is hit in __split_folio_to_order(). It turns out that I did not add the changes from your “mm: let _folio_nr_pages overlay memcg_data in first tail page” patch. With that fixed, no crash is observed so far. Best Regards, Yan, Zi
