linux-kernel - Re: [PATCH v6] zswap: replace RB tree with xarray

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Fri, 15 Mar 2024 18:30:37 -0700
From: Yosry Ahmed <yosryahmed@...gle.com>
To: Chris Li <chrisl@...nel.org>
Cc: Andrew Morton <akpm@...ux-foundation.org>, linux-kernel@...r.kernel.org, 
	linux-mm@...ck.org, Nhat Pham <nphamcs@...il.com>, 
	Johannes Weiner <hannes@...xchg.org>, "Matthew Wilcox (Oracle)" <willy@...radead.org>, 
	Chengming Zhou <zhouchengming@...edance.com>, Barry Song <v-songbaohua@...o.com>, 
	Barry Song <baohua@...nel.org>, Chengming Zhou <chengming.zhou@...ux.dev>
Subject: Re: [PATCH v6] zswap: replace RB tree with xarray

[..]
>
> @@ -1555,28 +1473,35 @@ bool zswap_store(struct folio *folio)
>  insert_entry:
>         entry->swpentry = swp;
>         entry->objcg = objcg;
> -       if (objcg) {
> -               obj_cgroup_charge_zswap(objcg, entry->length);
> -               /* Account before objcg ref is moved to tree */


I do not understand this comment, but it seems to care about the
charging happening before the entry is added to the tree. This patch
will move it after the tree insertion.

Johannes, do you mind elaborating what this comment is referring to?
It should be clarified, updated, or removed as part of this movement.

>
> -               count_objcg_event(objcg, ZSWPOUT);
> -       }
>
> -       /* map */
> -       spin_lock(&tree->lock);
>         /*
>          * The folio may have been dirtied again, invalidate the
>          * possibly stale entry before inserting the new entry.
>          */
> -       if (zswap_rb_insert(&tree->rbroot, entry, &dupentry) == -EEXIST) {
> -               zswap_invalidate_entry(tree, dupentry);
> -               WARN_ON(zswap_rb_insert(&tree->rbroot, entry, &dupentry));
> +       old = xa_store(tree, offset, entry, GFP_KERNEL);
> +       if (xa_is_err(old)) {
> +               int err = xa_err(old);
> +               if (err == -ENOMEM)
> +                       zswap_reject_alloc_fail++;
> +               else
> +                       WARN_ONCE(err, "%s: xa_store failed: %d\n",
> +                                 __func__, err);
> +               goto store_failed;
> +       }
> +       if (old)
> +               zswap_entry_free(old);
> +
> +       if (objcg) {
> +               obj_cgroup_charge_zswap(objcg, entry->length);
> +               /* Account before objcg ref is moved to tree */
> +               count_objcg_event(objcg, ZSWPOUT);
>         }
> +
>         if (entry->length) {
>                 INIT_LIST_HEAD(&entry->lru);
>                 zswap_lru_add(&zswap.list_lru, entry);
>                 atomic_inc(&zswap.nr_stored);
>         }
> -       spin_unlock(&tree->lock);
>
>         /* update stats */
>         atomic_inc(&zswap_stored_pages);

[..]