On 2024/3/21 05:31, Yosry Ahmed wrote: > On Wed, Mar 20, 2024 at 2:19 PM Johannes Weiner <hannes@xxxxxxxxxxx> wrote: >> >> On Wed, Mar 20, 2024 at 05:07:21PM -0400, Johannes Weiner wrote: >>> On Wed, Mar 20, 2024 at 01:49:17PM -0700, Yosry Ahmed wrote: >>>> Hey folks, >>>> >>>> I was looking at cleaning up the same-filled handling code in zswap, >>>> when it hit me that after the xarray conversion, the only member of >>>> struct zwap_entry that is relevant to same-filled pages is now the >>>> objcg pointer. >>>> >>>> The xarray allows a pointer to be tagged by up to two tags (1 and 3), >>>> so we can completely avoid allocating a zswap_entry for same-filled >>>> pages by storing a tagged objcg pointer directly in the xarray >>>> instead. >>>> >>>> Basically the xarray would then either have a pointer to struct >>>> zswap_entry or struct obj_cgroup, where the latter is tagged as >>>> SAME_FILLED_ONE or SAME_FILLED_ZERO. >>>> >>>> There are two benefits of this: >>>> - Saving some memory (precisely 64 bytes per same-filled entry). >>>> - Further separating handling of same-filled pages from compressed >>>> pages, which results in some nice cleanups (especially in >>>> zswap_store()). It also makes further improvements easier (e.g. >>>> skipping limit checking for same-filled entries). I also think this is a good idea. :) Which could simplify the code too. >>> >>> This sounds interesting. >>> >>> Where would you store the byte value it's filled with? Or would you >>> limit it to zero-filled only? >> >> The dumb thing about objcg is that for same-filled entries we really >> only need it for bumping ZSWPIN. Nothing else. entry->length is 0 for >> them, so even though we call the charge function, it doesn't actually >> do anything. >> >> Loading them is cheap and doesn't involve decompression. An argument >> could be made to exclude them from ZSWPOUT and ZSWPIN entirely. >> >> Or cheat a little and bump ZSWPIN for current->objcg instead - >> probably good enough to make excessive thrashing discoverable by the >> workload that's directly affected. >> >> Then you could get rid of the objcg pointer and use the xarray slot >> for whatever else you'd want. > > Yeah it's only useful for the stats. Using current->objcg would work, > and should be ultimately pointing to the same memcg in *most* cases, I In some cases where the current objcg is not "correct", the testcases in test_zswap.c may break? Maybe we can use swap_cgroup info to charge the stats to the correct memcg? Not sure if this is feasible. > assume. We still wouldn't be able to store a full word as we do today, > because the xarray needs 1 bit for its own usage. So the same-filled > implementation would still need to change from repeated words (8 > bytes) to something smaller -- or we can just allocate a separate > struct for same-filled pages. Yes, this seems an unavoidable limit of value in xarray... Thanks.
