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

[Sebastian Andrzej Siewior <bigeasy@linutronix.de>]

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?

Sebastian