Re: [PATCH v5 10/18] zsmalloc: factor out pool locking helpers

[Yosry Ahmed <yosry.ahmed@linux.dev>]

On Wed, Feb 12, 2025 at 04:18:14PM +0000, Yosry Ahmed wrote:
> On Wed, Feb 12, 2025 at 03:27:08PM +0900, Sergey Senozhatsky wrote:
> > We currently have a mix of migrate_{read,write}_lock() helpers
> > that lock zspages, but it's zs_pool that actually has a ->migrate_lock
> > access to which is opene-coded.  Factor out pool migrate locking
> > into helpers, zspage migration locking API will be renamed to
> > reduce confusion.
> > 
> > It's worth mentioning that zsmalloc locks sync not only migration,
> > but also compaction.
> > 
> > Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> 
> FWIW I don't see a lot of value in the helpers (renaming the lock is
> useful tho). We open-code other locks like the class lock anyway, and
> the helpers obscure the underlying lock type without adding much value
> in terms of readability/conciseness.

We use helpers for the class lock in the following change, but my point
stands for that too.

> 
> > ---
> >  mm/zsmalloc.c | 63 +++++++++++++++++++++++++++++++++++----------------
> >  1 file changed, 44 insertions(+), 19 deletions(-)
> > 
> > diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
> > index 6d0e47f7ae33..47c638df47c5 100644
> > --- a/mm/zsmalloc.c
> > +++ b/mm/zsmalloc.c
> > @@ -18,7 +18,7 @@
> >  /*
> >   * lock ordering:
> >   *	page_lock
> > - *	pool->migrate_lock
> > + *	pool->lock
> >   *	class->lock
> >   *	zspage->lock
> >   */
> > @@ -224,10 +224,35 @@ struct zs_pool {
> >  	struct work_struct free_work;
> >  #endif
> >  	/* protect page/zspage migration */
> > -	rwlock_t migrate_lock;
> > +	rwlock_t lock;
> >  	atomic_t compaction_in_progress;
> >  };
> >  
> > +static void pool_write_unlock(struct zs_pool *pool)
> > +{
> > +	write_unlock(&pool->lock);
> > +}
> > +
> > +static void pool_write_lock(struct zs_pool *pool)
> > +{
> > +	write_lock(&pool->lock);
> > +}
> > +
> > +static void pool_read_unlock(struct zs_pool *pool)
> > +{
> > +	read_unlock(&pool->lock);
> > +}
> > +
> > +static void pool_read_lock(struct zs_pool *pool)
> > +{
> > +	read_lock(&pool->lock);
> > +}
> > +
> > +static bool pool_lock_is_contended(struct zs_pool *pool)
> > +{
> > +	return rwlock_is_contended(&pool->lock);
> > +}
> > +
> >  static inline void zpdesc_set_first(struct zpdesc *zpdesc)
> >  {
> >  	SetPagePrivate(zpdesc_page(zpdesc));
> > @@ -1206,7 +1231,7 @@ void *zs_map_object(struct zs_pool *pool, unsigned long handle,
> >  	BUG_ON(in_interrupt());
> >  
> >  	/* It guarantees it can get zspage from handle safely */
> > -	read_lock(&pool->migrate_lock);
> > +	pool_read_lock(pool);
> >  	obj = handle_to_obj(handle);
> >  	obj_to_location(obj, &zpdesc, &obj_idx);
> >  	zspage = get_zspage(zpdesc);
> > @@ -1218,7 +1243,7 @@ void *zs_map_object(struct zs_pool *pool, unsigned long handle,
> >  	 * which is smaller granularity.
> >  	 */
> >  	migrate_read_lock(zspage);
> > -	read_unlock(&pool->migrate_lock);
> > +	pool_read_unlock(pool);
> >  
> >  	class = zspage_class(pool, zspage);
> >  	off = offset_in_page(class->size * obj_idx);
> > @@ -1450,16 +1475,16 @@ void zs_free(struct zs_pool *pool, unsigned long handle)
> >  		return;
> >  
> >  	/*
> > -	 * The pool->migrate_lock protects the race with zpage's migration
> > +	 * The pool->lock protects the race with zpage's migration
> >  	 * so it's safe to get the page from handle.
> >  	 */
> > -	read_lock(&pool->migrate_lock);
> > +	pool_read_lock(pool);
> >  	obj = handle_to_obj(handle);
> >  	obj_to_zpdesc(obj, &f_zpdesc);
> >  	zspage = get_zspage(f_zpdesc);
> >  	class = zspage_class(pool, zspage);
> >  	spin_lock(&class->lock);
> > -	read_unlock(&pool->migrate_lock);
> > +	pool_read_unlock(pool);
> >  
> >  	class_stat_sub(class, ZS_OBJS_INUSE, 1);
> >  	obj_free(class->size, obj);
> > @@ -1793,10 +1818,10 @@ static int zs_page_migrate(struct page *newpage, struct page *page,
> >  	pool = zspage->pool;
> >  
> >  	/*
> > -	 * The pool migrate_lock protects the race between zpage migration
> > +	 * The pool lock protects the race between zpage migration
> >  	 * and zs_free.
> >  	 */
> > -	write_lock(&pool->migrate_lock);
> > +	pool_write_lock(pool);
> >  	class = zspage_class(pool, zspage);
> >  
> >  	/*
> > @@ -1833,7 +1858,7 @@ static int zs_page_migrate(struct page *newpage, struct page *page,
> >  	 * Since we complete the data copy and set up new zspage structure,
> >  	 * it's okay to release migration_lock.
> >  	 */
> > -	write_unlock(&pool->migrate_lock);
> > +	pool_write_unlock(pool);
> >  	spin_unlock(&class->lock);
> >  	migrate_write_unlock(zspage);
> >  
> > @@ -1956,7 +1981,7 @@ static unsigned long __zs_compact(struct zs_pool *pool,
> >  	 * protect the race between zpage migration and zs_free
> >  	 * as well as zpage allocation/free
> >  	 */
> > -	write_lock(&pool->migrate_lock);
> > +	pool_write_lock(pool);
> >  	spin_lock(&class->lock);
> >  	while (zs_can_compact(class)) {
> >  		int fg;
> > @@ -1983,14 +2008,14 @@ static unsigned long __zs_compact(struct zs_pool *pool,
> >  		src_zspage = NULL;
> >  
> >  		if (get_fullness_group(class, dst_zspage) == ZS_INUSE_RATIO_100
> > -		    || rwlock_is_contended(&pool->migrate_lock)) {
> > +		    || pool_lock_is_contended(pool)) {
> >  			putback_zspage(class, dst_zspage);
> >  			dst_zspage = NULL;
> >  
> >  			spin_unlock(&class->lock);
> > -			write_unlock(&pool->migrate_lock);
> > +			pool_write_unlock(pool);
> >  			cond_resched();
> > -			write_lock(&pool->migrate_lock);
> > +			pool_write_lock(pool);
> >  			spin_lock(&class->lock);
> >  		}
> >  	}
> > @@ -2002,7 +2027,7 @@ static unsigned long __zs_compact(struct zs_pool *pool,
> >  		putback_zspage(class, dst_zspage);
> >  
> >  	spin_unlock(&class->lock);
> > -	write_unlock(&pool->migrate_lock);
> > +	pool_write_unlock(pool);
> >  
> >  	return pages_freed;
> >  }
> > @@ -2014,10 +2039,10 @@ unsigned long zs_compact(struct zs_pool *pool)
> >  	unsigned long pages_freed = 0;
> >  
> >  	/*
> > -	 * Pool compaction is performed under pool->migrate_lock so it is basically
> > +	 * Pool compaction is performed under pool->lock so it is basically
> >  	 * single-threaded. Having more than one thread in __zs_compact()
> > -	 * will increase pool->migrate_lock contention, which will impact other
> > -	 * zsmalloc operations that need pool->migrate_lock.
> > +	 * will increase pool->lock contention, which will impact other
> > +	 * zsmalloc operations that need pool->lock.
> >  	 */
> >  	if (atomic_xchg(&pool->compaction_in_progress, 1))
> >  		return 0;
> > @@ -2139,7 +2164,7 @@ struct zs_pool *zs_create_pool(const char *name)
> >  		return NULL;
> >  
> >  	init_deferred_free(pool);
> > -	rwlock_init(&pool->migrate_lock);
> > +	rwlock_init(&pool->lock);
> >  	atomic_set(&pool->compaction_in_progress, 0);
> >  
> >  	pool->name = kstrdup(name, GFP_KERNEL);
> > -- 
> > 2.48.1.502.g6dc24dfdaf-goog
> > 
>