On 3/21/24 12:07, Oscar Salvador wrote: > On Thu, Mar 21, 2024 at 11:50:36AM +0100, Vlastimil Babka wrote: >> Yeah I think we could keep that logic. > > I am all for keeping it. > >> But we could also simply subtract the refcount of the old handle (the >> "allocated for migration") in __folio_copy_owner() no? Then we wouldn't need >> the extra migrate_handle. > > Since new_page will have the old handle pointing to the old stack after > the call, we > could uncharge the old_page to the migrate_stack, which new_page->_handle holds > before it gets changed. > > So we basically swap it. > > It could work, but I kinda have a bittersweet feeling here. > I am trying to work towards to reduce the number of lookups in the > hlist, but for the approach described above I would need to lookup > the stack for new_page->handle in order to substract the page. Understood, but migration is kinda heavy and non-fast-path operation already so the extra lookup wouldn't be in a critical fast path. > OTHO, I understand that adding migrate_handle kinda wasted memory. > 16MB for 16GB of memory. > >> Also we might have more issues here. Most page owner code takes care to set >> everything for all pages within a folio, but __folio_copy_owner() and >> __set_page_owner_migrate_reason() don't. > > I did not check deeply but do not we split the folio upon migration > in case it is large? > Which means we should reach split_page_owner() before the copy takes > place. Do I get it right? When I mean is we have __set_page_owner_handle() setting up everything for tail pages and then we have __folio_copy_owner updating only the head page, so this will create kinda a mixed up information. Which might not be an issue as __folio_copy_owner() should mean it's a proper folio thus compound page thus nobody ever will check those mismatched tail pages... Maybe we could adjust __set_page_owner_handle() to skip tail pages for compound pages as well and unify this, and tail pages would be only setup for those weird high-order non-compound pages so that the odd case in __free_pages() works? Or maybe page_owner is considered debugging functionality enough so it might be worth having the redundant data in tail pages in case something goes wrong. But either way now it seems we're not doing it consistently.
