Re: [PATCH v4 4/5] zsmalloc: Add ops fields to zs_pool to store evict handlers

[Johannes Weiner]

On Thu, Nov 17, 2022 at 02:25:52PM -0800, Minchan Kim wrote:
> On Thu, Nov 17, 2022 at 08:38:38AM -0800, 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@xxxxxxxxx>
> > ---
> >  mm/zsmalloc.c | 35 ++++++++++++++++++++++++++++++++++-
> >  1 file changed, 34 insertions(+), 1 deletion(-)
> > 
> > diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
> > index 2557b55ec767..776d0e15a401 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,9 @@ struct zs_pool {
> >  #ifdef CONFIG_ZPOOL
> >  	/* List tracking the zspages in LRU order by most recently added object */
> >  	struct list_head lru;
> > +	const struct zs_ops *ops;
> > +	struct zpool *zpool;
> > +	const struct zpool_ops *zpool_ops;
> >  #endif
> > 
> >  #ifdef CONFIG_ZSMALLOC_STAT
> > @@ -385,6 +394,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)
> > @@ -394,7 +415,19 @@ 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)
> 
> I lost. When do we have zpool_ops as NULL?

It actually can't be as of today, it's just the zpool API that
pretends it can be, and so zsmalloc follows it here.

Actually, this is pretty trivial to clean up across zpool and the
drivers. How about the below patch? Nhat, would you mind picking that
up into your series? It should be easy to update 4/5 in the same way.

---