Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 3 Nov 2022 12:45:53 -0400
From:   Johannes Weiner <hannes@cmpxchg.org>
To:     Sergey Senozhatsky <senozhatsky@chromium.org>
Cc:     Nhat Pham <nphamcs@gmail.com>, akpm@linux-foundation.org,
        linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        minchan@kernel.org, ngupta@vflare.org, sjenning@redhat.com,
        ddstreet@ieee.org, vitaly.wool@konsulko.com
Subject: Re: [PATCH v2 5/5] zsmalloc: Implement writeback mechanism for
 zsmalloc
Message-ID: <Y2PwQTk016ngDKhc@cmpxchg.org>
References: <202210272158.7swYwd23-lkp@intel.com>
 <20221027182736.513530-1-nphamcs@gmail.com>
 <Y2HuhiyorAHFO+Ss@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Y2HuhiyorAHFO+Ss@google.com>
Precedence: bulk

On Wed, Nov 02, 2022 at 01:13:58PM +0900, Sergey Senozhatsky wrote:
> On (22/10/27 11:27), Nhat Pham wrote:
> > +
> > +static int zs_zpool_shrink(void *pool, unsigned int pages,
> > +			unsigned int *reclaimed)
> > +{
> > +	unsigned int total = 0;
> > +	int ret = -EINVAL;
> > +
> > +	while (total < pages) {
> > +		ret = zs_reclaim_page(pool, 8);
> > +		if (ret < 0)
> > +			break;
> > +		total++;
> > +	}
> > +
> > +	if (reclaimed)
> > +		*reclaimed = total;
> > +
> > +	return ret;
> > +}
> 
> The name collides with shrinker callbacks (compaction). That's a bit
> confusing, took me some time.

Yeah this tripped us up too.

Unfortunately, this is inherited from the zpool API:

> > @@ -482,6 +504,7 @@ static struct zpool_driver zs_zpool_driver = {
> >  	.malloc_support_movable = true,
> >  	.malloc =		  zs_zpool_malloc,
> >  	.free =			  zs_zpool_free,
> > +	.shrink =     zs_zpool_shrink,

There is another terminology collision around "compaction" and
"migration": There is zs_page_migrate() which is called from physical
page migration and compaction to switch out backing struct pages of
the zspage. Then there is migrate_zspage() which is called from a
*shrinker* through *zs_compact()*, and it consolidates the objects
from partially used zspages into full ones.

We're collecting these issues and want to clean them up separately. It
seems this codebase hasn't had a lot of TLC recently, which makes it
harder to make needed changes :/ We try to clean up as we go, but some
of these issues (like the .shrink callback) run through all backends,
zpool and zswap, so they'd be better off in separate cleanup patches.

The out-of-control locking complexity in zsmalloc is another one of
these issues that we hoped we could clean up along the way, as you
have noticed...