On Wed, Jun 7, 2023 at 11:30 AM Yosry Ahmed <yosryahmed@xxxxxxxxxx> wrote:
>
> On Tue, Jun 6, 2023 at 7:56 AM Domenico Cerasuolo
> <cerasuolodomenico@xxxxxxxxx> wrote:
> >
> > Previously, zswap_header served the purpose of storing the swpentry
> > within zpool pages. This allowed zpool implementations to pass relevant
> > information to the writeback function. However, with the current
> > implementation, writeback is directly handled within zswap.
> > Consequently, there is no longer a necessity for zswap_header, as the
> > swp_entry_t can be stored directly in zswap_entry.
> >
> > Suggested-by: Yosry Ahmed <yosryahmed@xxxxxxxxxx>
> > Signed-off-by: Domenico Cerasuolo <cerasuolodomenico@xxxxxxxxx>
>
> Thanks for this cleanup. It gives us back some of the memory used by
> list_head in zswap entry, and remove an unnecessary zpool map
> operation.
>
> > ---
> > mm/zswap.c | 52 ++++++++++++++++++++++------------------------------
> > 1 file changed, 22 insertions(+), 30 deletions(-)
> >
> > diff --git a/mm/zswap.c b/mm/zswap.c
> > index ef8604812352..f689444dd5a7 100644
> > --- a/mm/zswap.c
> > +++ b/mm/zswap.c
> > @@ -193,7 +193,7 @@ struct zswap_pool {
> > */
> > struct zswap_entry {
> > struct rb_node rbnode;
> > - pgoff_t offset;
> > + swp_entry_t swpentry;
> > int refcount;
> > unsigned int length;
> > struct zswap_pool *pool;
> > @@ -205,10 +205,6 @@ struct zswap_entry {
> > struct list_head lru;
> > };
> >
> > -struct zswap_header {
> > - swp_entry_t swpentry;
> > -};
> > -
> > /*
> > * The tree lock in the zswap_tree struct protects a few things:
> > * - the rbtree
> > @@ -250,7 +246,7 @@ static bool zswap_has_pool;
> > pr_debug("%s pool %s/%s\n", msg, (p)->tfm_name, \
> > zpool_get_type((p)->zpool))
> >
> > -static int zswap_writeback_entry(struct zswap_entry *entry, struct zswap_header *zhdr,
> > +static int zswap_writeback_entry(struct zswap_entry *entry,
> > struct zswap_tree *tree);
> > static int zswap_pool_get(struct zswap_pool *pool);
> > static void zswap_pool_put(struct zswap_pool *pool);
> > @@ -311,12 +307,14 @@ static struct zswap_entry *zswap_rb_search(struct rb_root *root, pgoff_t offset)
> > {
> > struct rb_node *node = root->rb_node;
> > struct zswap_entry *entry;
> > + pgoff_t entry_offset;
> >
> > while (node) {
> > entry = rb_entry(node, struct zswap_entry, rbnode);
> > - if (entry->offset > offset)
> > + entry_offset = swp_offset(entry->swpentry);
> > + if (entry_offset > offset)
> > node = node->rb_left;
> > - else if (entry->offset < offset)
> > + else if (entry_offset < offset)
> > node = node->rb_right;
> > else
> > return entry;
> > @@ -333,13 +331,15 @@ static int zswap_rb_insert(struct rb_root *root, struct zswap_entry *entry,
> > {
> > struct rb_node **link = &root->rb_node, *parent = NULL;
> > struct zswap_entry *myentry;
> > + pgoff_t myentry_offset, entry_offset = swp_offset(entry->swpentry);
> >
> > while (*link) {
> > parent = *link;
> > myentry = rb_entry(parent, struct zswap_entry, rbnode);
> > - if (myentry->offset > entry->offset)
> > + myentry_offset = swp_offset(myentry->swpentry);
> > + if (myentry_offset > entry_offset)
> > link = &(*link)->rb_left;
> > - else if (myentry->offset < entry->offset)
> > + else if (myentry_offset < entry_offset)
>
> This whole myentry thing is very confusing to me. I initially thought
> myentry would be the entry passed in as an argument. Can we change the
> naming here to make it more consistent with zswap_rb_search() naming?
>
I'm not sure I understood the suggestion, is it related to the addition of
myentry_offset variable or is myentry itself that you would like to change?
> > link = &(*link)->rb_right;
> > else {
> > *dupentry = myentry;
> > @@ -598,7 +598,6 @@ static struct zswap_pool *zswap_pool_find_get(char *type, char *compressor)
> > static int zswap_shrink(struct zswap_pool *pool)
> > {
> > struct zswap_entry *lru_entry, *tree_entry = NULL;
> > - struct zswap_header *zhdr;
> > struct zswap_tree *tree;
> > int swpoffset;
> > int ret;
> > @@ -611,15 +610,13 @@ static int zswap_shrink(struct zswap_pool *pool)
> > }
> > lru_entry = list_last_entry(&pool->lru, struct zswap_entry, lru);
> > list_del_init(&lru_entry->lru);
> > - zhdr = zpool_map_handle(pool->zpool, lru_entry->handle, ZPOOL_MM_RO);
> > - tree = zswap_trees[swp_type(zhdr->swpentry)];
> > - zpool_unmap_handle(pool->zpool, lru_entry->handle);
> > /*
> > * Once the pool lock is dropped, the lru_entry might get freed. The
> > * swpoffset is copied to the stack, and lru_entry isn't deref'd again
> > * until the entry is verified to still be alive in the tree.
> > */
> > - swpoffset = swp_offset(zhdr->swpentry);
> > + swpoffset = swp_offset(lru_entry->swpentry);
> > + tree = zswap_trees[swp_type(lru_entry->swpentry)];
> > spin_unlock(&pool->lru_lock);
> >
> > /* hold a reference from tree so it won't be freed during writeback */
> > @@ -633,7 +630,7 @@ static int zswap_shrink(struct zswap_pool *pool)
> > }
> > spin_unlock(&tree->lock);
> >
> > - ret = zswap_writeback_entry(lru_entry, zhdr, tree);
> > + ret = zswap_writeback_entry(lru_entry, tree);
> >
> > spin_lock(&tree->lock);
> > if (ret) {
> > @@ -1046,10 +1043,10 @@ static int zswap_get_swap_cache_page(swp_entry_t entry,
> > * the swap cache, the compressed version stored by zswap can be
> > * freed.
> > */
> > -static int zswap_writeback_entry(struct zswap_entry *entry, struct zswap_header *zhdr,
> > +static int zswap_writeback_entry(struct zswap_entry *entry,
> > struct zswap_tree *tree)
> > {
> > - swp_entry_t swpentry = zhdr->swpentry;
> > + swp_entry_t swpentry = entry->swpentry;
> > struct page *page;
> > struct scatterlist input, output;
> > struct crypto_acomp_ctx *acomp_ctx;
> > @@ -1089,7 +1086,7 @@ static int zswap_writeback_entry(struct zswap_entry *entry, struct zswap_header
> > * writing.
> > */
> > spin_lock(&tree->lock);
> > - if (zswap_rb_search(&tree->rbroot, entry->offset) != entry) {
> > + if (zswap_rb_search(&tree->rbroot, swp_offset(entry->swpentry)) != entry) {
> > spin_unlock(&tree->lock);
> > delete_from_swap_cache(page_folio(page));
> > ret = -ENOMEM;
> > @@ -1101,8 +1098,7 @@ static int zswap_writeback_entry(struct zswap_entry *entry, struct zswap_header
> > acomp_ctx = raw_cpu_ptr(entry->pool->acomp_ctx);
> > dlen = PAGE_SIZE;
> >
> > - zhdr = zpool_map_handle(pool, entry->handle, ZPOOL_MM_RO);
> > - src = (u8 *)zhdr + sizeof(struct zswap_header);
> > + src = zpool_map_handle(pool, entry->handle, ZPOOL_MM_RO);
> > if (!zpool_can_sleep_mapped(pool)) {
> > memcpy(tmp, src, entry->length);
> > src = tmp;
> > @@ -1196,11 +1192,10 @@ static int zswap_frontswap_store(unsigned type, pgoff_t offset,
> > struct obj_cgroup *objcg = NULL;
> > struct zswap_pool *pool;
> > int ret;
> > - unsigned int hlen, dlen = PAGE_SIZE;
> > + unsigned int dlen = PAGE_SIZE;
> > unsigned long handle, value;
> > char *buf;
> > u8 *src, *dst;
> > - struct zswap_header zhdr = { .swpentry = swp_entry(type, offset) };
> > gfp_t gfp;
> >
> > /* THP isn't supported */
> > @@ -1245,7 +1240,7 @@ static int zswap_frontswap_store(unsigned type, pgoff_t offset,
> > src = kmap_atomic(page);
> > if (zswap_is_page_same_filled(src, &value)) {
> > kunmap_atomic(src);
> > - entry->offset = offset;
> > + entry->swpentry = swp_entry(type, offset);
> > entry->length = 0;
> > entry->value = value;
> > atomic_inc(&zswap_same_filled_pages);
> > @@ -1299,11 +1294,10 @@ static int zswap_frontswap_store(unsigned type, pgoff_t offset,
> > }
> >
> > /* store */
> > - hlen = sizeof(zhdr);
> > gfp = __GFP_NORETRY | __GFP_NOWARN | __GFP_KSWAPD_RECLAIM;
> > if (zpool_malloc_support_movable(entry->pool->zpool))
> > gfp |= __GFP_HIGHMEM | __GFP_MOVABLE;
> > - ret = zpool_malloc(entry->pool->zpool, hlen + dlen, gfp, &handle);
> > + ret = zpool_malloc(entry->pool->zpool, dlen, gfp, &handle);
> > if (ret == -ENOSPC) {
> > zswap_reject_compress_poor++;
> > goto put_dstmem;
> > @@ -1313,13 +1307,12 @@ static int zswap_frontswap_store(unsigned type, pgoff_t offset,
> > goto put_dstmem;
> > }
> > buf = zpool_map_handle(entry->pool->zpool, handle, ZPOOL_MM_WO);
> > - memcpy(buf, &zhdr, hlen);
> > - memcpy(buf + hlen, dst, dlen);
> > + memcpy(buf, dst, dlen);
> > zpool_unmap_handle(entry->pool->zpool, handle);
> > mutex_unlock(acomp_ctx->mutex);
> >
> > /* populate entry */
> > - entry->offset = offset;
> > + entry->swpentry = swp_entry(type, offset);
> > entry->handle = handle;
> > entry->length = dlen;
> >
> > @@ -1418,7 +1411,6 @@ static int zswap_frontswap_load(unsigned type, pgoff_t offset,
> > /* decompress */
> > dlen = PAGE_SIZE;
> > src = zpool_map_handle(entry->pool->zpool, entry->handle, ZPOOL_MM_RO);
> > - src += sizeof(struct zswap_header);
> >
> > if (!zpool_can_sleep_mapped(entry->pool->zpool)) {
> > memcpy(tmp, src, entry->length);
> > --
> > 2.34.1
> >
