Re: [RFC PATCH 7/7] mm: zswap: Use acomp virtual address interface

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

On Fri, Feb 28, 2025 at 05:54:53PM +0800, Herbert Xu wrote:
> On Fri, Feb 28, 2025 at 04:13:08PM +0800, Herbert Xu wrote:
> >
> > I'll respin this.
> 
> FWIW this is what the interface looks like.  Does it look OK?
> Longer term hardware offload drivers should handle these non-DMA
> pointers directly by having their own buffers.  For the time being
> I'm simply redirecting these to a software fallback.
> 
> diff --git a/mm/zswap.c b/mm/zswap.c
> index 2b5a2398a9be..2fd241c65f80 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -994,30 +994,16 @@ static void zswap_decompress(struct zswap_entry *entry, struct folio *folio)
>  
>  	acomp_ctx = acomp_ctx_get_cpu_lock(entry->pool);
>  	src = zpool_map_handle(zpool, entry->handle, ZPOOL_MM_RO);
> -	/*
> -	 * If zpool_map_handle is atomic, we cannot reliably utilize its mapped buffer
> -	 * to do crypto_acomp_decompress() which might sleep. In such cases, we must
> -	 * resort to copying the buffer to a temporary one.
> -	 * Meanwhile, zpool_map_handle() might return a non-linearly mapped buffer,
> -	 * such as a kmap address of high memory or even ever a vmap address.
> -	 * However, sg_init_one is only equipped to handle linearly mapped low memory.
> -	 * In such cases, we also must copy the buffer to a temporary and lowmem one.
> -	 */
> -	if ((acomp_ctx->is_sleepable && !zpool_can_sleep_mapped(zpool)) ||
> -	    !virt_addr_valid(src)) {
> -		memcpy(acomp_ctx->buffer, src, entry->length);
> -		src = acomp_ctx->buffer;
> -		zpool_unmap_handle(zpool, entry->handle);
> -	}
> -
>  	dst = kmap_local_folio(folio, 0);
> -	acomp_request_set_virt(acomp_ctx->req, src, dst, entry->length, PAGE_SIZE);
> +	if (!zpool_can_sleep_mapped(zpool) || !virt_addr_valid(src))

Why is the acomp_ctx->is_sleepable check no longer needed?

Also, the zpool_can_sleep_mapped() cases will go away soon-ish, so I was
kinda hoping that the !virt_addr_valid() case goes away too and is
handled internally in the crypto library. Right now the problem is that
virt_to_page() is used to get the underlying page, which doesn't work
for kmap addresses.

> +		acomp_request_set_nondma(acomp_ctx->req, src, dst, entry->length, PAGE_SIZE);
> +	else
> +		acomp_request_set_virt(acomp_ctx->req, src, dst, entry->length, PAGE_SIZE);
>  	BUG_ON(crypto_wait_req(crypto_acomp_decompress(acomp_ctx->req), &acomp_ctx->wait));
>  	kunmap_local(dst);
>  	BUG_ON(acomp_ctx->req->dlen != PAGE_SIZE);
>  
> -	if (src != acomp_ctx->buffer)
> -		zpool_unmap_handle(zpool, entry->handle);
> +	zpool_unmap_handle(zpool, entry->handle);
>  	acomp_ctx_put_unlock(acomp_ctx);
>  }
> 
> -- 
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt