> -----Original Message----- > From: Yosry Ahmed <yosryahmed@xxxxxxxxxx> > Sent: Tuesday, October 1, 2024 4:39 PM > To: Sridhar, Kanchana P <kanchana.p.sridhar@xxxxxxxxx> > Cc: linux-kernel@xxxxxxxxxxxxxxx; linux-mm@xxxxxxxxx; > hannes@xxxxxxxxxxx; nphamcs@xxxxxxxxx; chengming.zhou@xxxxxxxxx; > usamaarif642@xxxxxxxxx; shakeel.butt@xxxxxxxxx; ryan.roberts@xxxxxxx; > Huang, Ying <ying.huang@xxxxxxxxx>; 21cnbao@xxxxxxxxx; akpm@linux- > foundation.org; willy@xxxxxxxxxxxxx; Zou, Nanhai <nanhai.zou@xxxxxxxxx>; > Feghali, Wajdi K <wajdi.k.feghali@xxxxxxxxx>; Gopal, Vinodh > <vinodh.gopal@xxxxxxxxx> > Subject: Re: [PATCH v9 6/7] mm: zswap: Support large folios in zswap_store(). > > [..] > > > > > > > > > store_failed: > > > > > > > > > zpool_free(entry->pool->zpool, entry->handle); > > > > > > > > > -put_pool: > > > > > > > > > - zswap_pool_put(entry->pool); > > > > > > > > > -freepage: > > > > > > > > > +put_pool_objcg: > > > > > > > > > + zswap_pool_put(pool); > > > > > > > > > + obj_cgroup_put(objcg); > > > > > > > > > > > > > > > > I think if we reorder the function we can drop these calls, make > the > > > > > > > > comments positioned a bit better, and centralize the entry > > > > > > > > initializations. I am also not a fan of passing a semi-initialized > > > > > > > > entry to zswap_compress() to get the pool pointer. > > > > > > > > > > > > > > > > Does the following diff improve things or did I miss something? > > > > > > > > > > > > > > We shouldn’t be adding the entry to the xarray before initializing its > > > pool > > > > > > > and objcg, right? Please let me know if I am misunderstanding > what > > > > you're > > > > > > > proposing in the diff. > > > > > > > > > > > > It should be safe. We already initialize entry->lru after we insert > > > > > > the entry in the tree. See the comment above the call to > > > > > > zswap_lru_add(). Basically we are protected against concurrent > > > > > > stores/loads through the folio lock, and are protected against > > > > > > writeback because the entry is not on the LRU yet. > > > > > > > > > > Thanks for the clarification, Yosry. Since this is a change in the entry > > > > > initialization wrt the mainline, is it Ok if this is done in a follow-up > patch? > > > > > > > > Sure. We can discuss it separately. Do you want me to send a patch or > > > > do you intend to? > > > > > > Thanks Yosry! I will send the patch separately. > > > > Hi Yosry, > > > > I am preparing the follow-up patch so I can submit it once this patch-series > is > > merged to mm-unstable (since these changes have dependencies on my > > existing patch). > > > > Is my understanding correct that the folio lock also protects against swapoff > > happening in between addition of the entry to the xarray and initializing its > > members, which will need to be valid for > > swapoff --> ... -> free_swap_slot() --> zswap_invalidate() ? Would > appreciate > > it if you can confirm. > > Yes, the folio lock should protect against swapoff, as the folio must > be in the swapcache. > > For shmem, try_to_unuse() (called by swapoff()) will end up calling > shmem_swapin_folio(), which will lookup the folio in the swapcache, > find it, then lock it before proceeding to delete it from the swap > cache and ultimately freeing the swap entry. > > For anonymous memory, try_to_unuse() will call unuse_mm() -> .. -> > unuse_pte_range(), which will also lookup the folio and lock it before > deleting it from the swap cache and freeing the entry. > > try_to_unuse() will also loop over any remaining swapcache entries, > lock the folios and then try to free the swap entry. Sounds good Yosry. Thanks for the explanations! Thanks, Kanchana
