On Tue, Oct 17, 2023 at 7:44 PM Ryan Roberts <ryan.roberts@xxxxxxx> wrote:
On 19/09/2023 18:14, Nhat Pham wrote:
> From: Domenico Cerasuolo <cerasuolodomenico@xxxxxxxxx>
>
> Currently, we only have a single global LRU for zswap. This makes it
> impossible to perform worload-specific shrinking - an memcg cannot
> determine which pages in the pool it owns, and often ends up writing
> pages from other memcgs. This issue has been previously observed in
> practice and mitigated by simply disabling memcg-initiated shrinking:
>
> https://lore.kernel.org/all/20230530232435.3097106-1-nphamcs@xxxxxxxxx/T/#u
>
> This patch fully resolves the issue by replacing the global zswap LRU
> with memcg- and NUMA-specific LRUs, and modify the reclaim logic:
>
> a) When a store attempt hits an memcg limit, it now triggers a
> synchronous reclaim attempt that, if successful, allows the new
> hotter page to be accepted by zswap.
> b) If the store attempt instead hits the global zswap limit, it will
> trigger an asynchronous reclaim attempt, in which an memcg is
> selected for reclaim in a round-robin-like fashion.
>
> Signed-off-by: Domenico Cerasuolo <cerasuolodomenico@xxxxxxxxx>
> Co-developed-by: Nhat Pham <nphamcs@xxxxxxxxx>
> Signed-off-by: Nhat Pham <nphamcs@xxxxxxxxx>
> ---
> include/linux/list_lru.h | 39 +++++++
> include/linux/memcontrol.h | 5 +
> include/linux/zswap.h | 9 ++
> mm/list_lru.c | 46 ++++++--
> mm/swap_state.c | 19 ++++
> mm/zswap.c | 221 +++++++++++++++++++++++++++++--------
> 6 files changed, 287 insertions(+), 52 deletions(-)
>
[...]
> @@ -1199,8 +1272,10 @@ bool zswap_store(struct folio *folio)
> struct scatterlist input, output;
> struct crypto_acomp_ctx *acomp_ctx;
> struct obj_cgroup *objcg = NULL;
> + struct mem_cgroup *memcg = NULL;
> struct zswap_pool *pool;
> struct zpool *zpool;
> + int lru_alloc_ret;
> unsigned int dlen = PAGE_SIZE;
> unsigned long handle, value;
> char *buf;
> @@ -1218,14 +1293,15 @@ bool zswap_store(struct folio *folio)
> if (!zswap_enabled || !tree)
> return false;
>
> - /*
> - * XXX: zswap reclaim does not work with cgroups yet. Without a
> - * cgroup-aware entry LRU, we will push out entries system-wide based on
> - * local cgroup limits.
> - */
> objcg = get_obj_cgroup_from_folio(folio);
> - if (objcg && !obj_cgroup_may_zswap(objcg))
> - goto reject;
> + if (objcg && !obj_cgroup_may_zswap(objcg)) {
> + memcg = get_mem_cgroup_from_objcg(objcg);
> + if (shrink_memcg(memcg)) {
> + mem_cgroup_put(memcg);
> + goto reject;
> + }
> + mem_cgroup_put(memcg);
> + }
>
> /* reclaim space if needed */
> if (zswap_is_full()) {
> @@ -1240,7 +1316,11 @@ bool zswap_store(struct folio *folio)
> else
> zswap_pool_reached_full = false;
> }
> -
> + pool = zswap_pool_current_get();
> + if (!pool) {
> + ret = -EINVAL;
> + goto reject;
> + }
Hi, I'm working to add support for large folios within zswap, and noticed this
piece of code added by this change. I don't see any corresponding put. Have I
missed some detail or is there a bug here?
> /* allocate entry */
> entry = zswap_entry_cache_alloc(GFP_KERNEL);
> if (!entry) {
> @@ -1256,6 +1336,7 @@ bool zswap_store(struct folio *folio)
> entry->length = 0;
> entry->value = value;
> atomic_inc(&zswap_same_filled_pages);
> + zswap_pool_put(pool);
I see you put it in this error path, but after that, there is no further mention.
> goto insert_entry;
> }
> kunmap_atomic(src);
> @@ -1264,6 +1345,15 @@ bool zswap_store(struct folio *folio)
> if (!zswap_non_same_filled_pages_enabled)
> goto freepage;
>
> + if (objcg) {
> + memcg = get_mem_cgroup_from_objcg(objcg);
> + lru_alloc_ret = memcg_list_lru_alloc(memcg, &pool->list_lru, GFP_KERNEL);
> + mem_cgroup_put(memcg);
> +
> + if (lru_alloc_ret)
> + goto freepage;
> + }
> +
> /* if entry is successfully added, it keeps the reference */
> entry->pool = zswap_pool_current_get();
The entry takes it's reference to the pool here.
Thanks,
Ryan
Thanks Ryan, I think you're right. Coincidentally, we're about to send a new
version of the series, and will make sure to address this too.