On Tue, Oct 1, 2024 at 9:58 AM Sridhar, Kanchana P <kanchana.p.sridhar@xxxxxxxxx> wrote: > > > -----Original Message----- > > From: Yosry Ahmed <yosryahmed@xxxxxxxxxx> > > Sent: Monday, September 30, 2024 11:00 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?
