| Message ID | 20221026200613.1031261-5-nphamcs@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Implement writeback for zsmalloc | expand |
On Wed, Oct 26, 2022 at 01:06:12PM -0700, Nhat Pham wrote: > This adds fields to zs_pool to store evict handlers for writeback, > analogous to the zbud allocator. > > Signed-off-by: Nhat Pham <nphamcs@gmail.com> Acked-by: Johannes Weiner <hannes@cmpxchg.org> > --- > mm/zsmalloc.c | 36 +++++++++++++++++++++++++++++++++++- > 1 file changed, 35 insertions(+), 1 deletion(-) Sort of tangential, but this patch highlights the boiler plate that is involved in supporting multiple allocator backends. We evaluated zswap with a variety of workloads, and found zbud and z3fold simply not dense enough for the compression ratios we would get at the higher end. They also seem more limited at lower compression rates due to the space they require for the in-line metadata. With writeback support, zsmalloc finally appears to be a superset of the others. I believe Google in their DCs is using zsmalloc exclusively, too. Android is using zram, which only supports zsmalloc. Given their impact on the codebase wrt maintenance and hackability, it begs the question zbud and z3fold are worth keeping around. Seth, Dan, are you aware of usecases where zsmalloc would be a no go?
On (22/10/26 13:06), Nhat Pham wrote: > +struct zs_pool; > + > +struct zs_ops { > + int (*evict)(struct zs_pool *pool, unsigned long handle); > +}; > + > struct zs_pool { > const char *name; > > @@ -242,6 +248,12 @@ struct zs_pool { > /* List tracking the zspages in LRU order by most recently added object */ > struct list_head lru; > > +#ifdef CONFIG_ZPOOL > + const struct zs_ops *ops; > + struct zpool *zpool; > + const struct zpool_ops *zpool_ops; > +#endif I really don't think I follow this zpoll_ops and zs_ops things. Why do we have zs_ops?
Essentially, the zpool constructor allows us to set things up with a null struct zpool, zpool_ops, or zpool_ops->evict, which we have to handle. A similar null-handling pattern can be observed in zbud (mm/zbud.c) and z3fold (mm/z3fold.c) - see zbud_zpool_evict and zbud_zpool_create for e.g. In particular: 1. pool->zpool_ops is the ops (containing the evict handler zpool_ops->evict) passed into the zpool constructor (zs_zpool_create) 2. pool->ops/zs_zpool_ops (struct zs_ops) is a struct wrapping zs_zpool_evict, which itself is a wrapper for the zpool evict handler (pool->zpool_ops->evict). zs_zpool_evict also handles the case where zpool or zpool_ops is null, or zpool_ops->evict is not defined (i.e return -ENOENT). FWIW, I do think this is quite convoluted. In the long run, we might want to simplify this design, but for this patch series I think it is wise to err on the safe side and follow the other two allocators' design for consistency. That said, while staring at the code again, I found a bug - in the case pool->zpool_ops is null, pool->ops is undefined garbage. The v3 patch will fix that to follow zbud's pattern (pool->ops = NULL in this case).
diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c index 600c40121544..76ff2ed839d0 100644 --- a/mm/zsmalloc.c +++ b/mm/zsmalloc.c @@ -225,6 +225,12 @@ struct link_free { }; }; +struct zs_pool; + +struct zs_ops { + int (*evict)(struct zs_pool *pool, unsigned long handle); +}; + struct zs_pool { const char *name; @@ -242,6 +248,12 @@ struct zs_pool { /* List tracking the zspages in LRU order by most recently added object */ struct list_head lru; +#ifdef CONFIG_ZPOOL + const struct zs_ops *ops; + struct zpool *zpool; + const struct zpool_ops *zpool_ops; +#endif + #ifdef CONFIG_ZSMALLOC_STAT struct dentry *stat_dentry; #endif @@ -379,6 +391,18 @@ static void record_obj(unsigned long handle, unsigned long obj) #ifdef CONFIG_ZPOOL +static int zs_zpool_evict(struct zs_pool *pool, unsigned long handle) +{ + if (pool->zpool && pool->zpool_ops && pool->zpool_ops->evict) + return pool->zpool_ops->evict(pool->zpool, handle); + else + return -ENOENT; +} + +static const struct zs_ops zs_zpool_ops = { + .evict = zs_zpool_evict +}; + static void *zs_zpool_create(const char *name, gfp_t gfp, const struct zpool_ops *zpool_ops, struct zpool *zpool) @@ -388,7 +412,17 @@ static void *zs_zpool_create(const char *name, gfp_t gfp, * different contexts and its caller must provide a valid * gfp mask. */ - return zs_create_pool(name); + struct zs_pool *pool = zs_create_pool(name); + + if (pool) { + pool->zpool = zpool; + pool->zpool_ops = zpool_ops; + + if (zpool_ops) + pool->ops = &zs_zpool_ops; + } + + return pool; } static void zs_zpool_destroy(void *pool)
This adds fields to zs_pool to store evict handlers for writeback, analogous to the zbud allocator. Signed-off-by: Nhat Pham <nphamcs@gmail.com> --- mm/zsmalloc.c | 36 +++++++++++++++++++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-)