Re: [PATCH 7/8] zsmalloc: replace per zpage lock with pool->migrate_lock

[Minchan Kim <minchan@kernel.org>]

On Fri, Nov 12, 2021 at 08:31:57AM +0100, Sebastian Andrzej Siewior wrote:
> On 2021-11-11 15:11:34 [-0800], Minchan Kim wrote:
> > > > @@ -2077,8 +2043,13 @@ static unsigned long __zs_compact(struct zs_pool *pool,
> > > >  	struct zspage *dst_zspage = NULL;
> > > >  	unsigned long pages_freed = 0;
> > > >  
> > > > +	/* protect the race between zpage migration and zs_free */
> > > > +	write_lock(&pool->migrate_lock);
> > > > +	/* protect zpage allocation/free */
> > > >  	spin_lock(&class->lock);
> > > >  	while ((src_zspage = isolate_zspage(class, true))) {
> > > > +		/* protect someone accessing the zspage(i.e., zs_map_object) */
> > > > +		migrate_write_lock(src_zspage);
> > > >  
> > > >  		if (!zs_can_compact(class))
> > > >  			break;
> > > > @@ -2087,6 +2058,8 @@ static unsigned long __zs_compact(struct zs_pool *pool,
> > > >  		cc.s_page = get_first_page(src_zspage);
> > > >  
> > > >  		while ((dst_zspage = isolate_zspage(class, false))) {
> > > > +			migrate_write_lock_nested(dst_zspage);
> > > > +
> > > >  			cc.d_page = get_first_page(dst_zspage);
> > > >  			/*
> > > >  			 * If there is no more space in dst_page, resched
> > > 
> > > Looking at the these two chunks, the page here comes from a list, you
> > > remove that page from that list and this ensures that you can't lock the
> > > very same pages in reverse order as in:
> > > 
> > >    migrate_write_lock(dst_zspage);
> > >    …
> > >    	migrate_write_lock(src_zspage);
> > > 
> > > right?
> > 
> > Sure.
> 
> Out of curiosity: why do you need to lock it then if you grab it from
> the list and there is no other reference to it? Is it because the page
> might be referenced by other means but only by a reader?

Yub, zs_map_object.