From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 7A5E6C433E0 for ; Thu, 14 Jan 2021 19:05:42 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id BE4E523A58 for ; Thu, 14 Jan 2021 19:05:41 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org BE4E523A58 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=konsulko.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 06B848D010D; Thu, 14 Jan 2021 14:05:41 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 01B118D00F0; Thu, 14 Jan 2021 14:05:40 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id E4B898D010D; Thu, 14 Jan 2021 14:05:40 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0093.hostedemail.com [216.40.44.93]) by kanga.kvack.org (Postfix) with ESMTP id CEBCF8D00F0 for ; Thu, 14 Jan 2021 14:05:40 -0500 (EST) Received: from smtpin24.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay01.hostedemail.com (Postfix) with ESMTP id 84C21180AD817 for ; Thu, 14 Jan 2021 19:05:40 +0000 (UTC) X-FDA: 77705309640.24.lake39_4611ce827529 Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin24.hostedemail.com (Postfix) with ESMTP id 5D1241A4AA for ; Thu, 14 Jan 2021 19:05:40 +0000 (UTC) X-HE-Tag: lake39_4611ce827529 X-Filterd-Recvd-Size: 12581 Received: from mail-lf1-f47.google.com (mail-lf1-f47.google.com [209.85.167.47]) by imf26.hostedemail.com (Postfix) with ESMTP for ; Thu, 14 Jan 2021 19:05:38 +0000 (UTC) Received: by mail-lf1-f47.google.com with SMTP id b26so9574960lff.9 for ; Thu, 14 Jan 2021 11:05:38 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=konsulko.com; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=O8X+EBB4o8D8/xYxTLG1XFh6Tk4zUL/YgSglq01mj9k=; b=t+4ZJjXdB0/jZ1xXOOstCkV9IyRuDceevK3cmDBAjFSE7ux5rTapHT8K5Xr681VUW0 YHbzWuSz1BuPW13eG5hBXH2N+uP1aBwxQnDRZQZg51smarw+85P7xmqgzTtX7V2tr4Yi QxGPRPNwRvrz4Ne7K/sA9ho44SEL6x9tI9xZI= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=O8X+EBB4o8D8/xYxTLG1XFh6Tk4zUL/YgSglq01mj9k=; b=YZ5SeIX/48+f3EInt85iCf8enJXpP2ryVSfVkYGTYTsVbJavOL8svoMYeWOokDB20i Pg+huNE++Ny+aQ5LqGlVS38/R9Sfb2c8Rr+lfM6wbmucE3jldoYiHZeSA88Ay6d0N8ch 71zJHnJ2D1aga0YrY4MEUn1Bh4NpacoWKDnJbhGAXIsgbEknjCjb3fyZSwX07foY/FuH d00FAoGqf1hXHG4OM87tDP5G05syWAa73aTZAr8wpiHZiFlaBajpIh+qaR4piC3K+N12 DcCbr5T1sJn+wkupMdRGGfrYxYpMtUVtni0jwhErUsvhrPjaHI6xU9mXq+jeQ4QevVKA /VYA== X-Gm-Message-State: AOAM530ha+voaeNlDGDz73KYI82EYaRJlX/qzuXxyQyUoT51l0alwtv3 34BGeIOoX1kBDefBqt8yEgm89awBpiOyowPaw2Jl5g== X-Google-Smtp-Source: ABdhPJykyEJfqjP3Xh7ti2qjKqpLwyu2hPwjLeSdp3hBRG2LL3Xn5C2bIX9fxEY8NyDoj0qxVNVILVRcDPTl+y40aQ0= X-Received: by 2002:a05:6512:2009:: with SMTP id a9mr3662650lfb.575.1610651137419; Thu, 14 Jan 2021 11:05:37 -0800 (PST) MIME-Version: 1.0 References: <1608894171-54174-1-git-send-email-tiantao6@hisilicon.com> <1608894171-54174-2-git-send-email-tiantao6@hisilicon.com> In-Reply-To: From: Vitaly Wool Date: Thu, 14 Jan 2021 20:05:25 +0100 Message-ID: Subject: Re: [RFC mm/zswap 1/2] mm/zswap: add the flag can_sleep_mapped To: Minchan Kim Cc: Tian Tao , Seth Jennings , Dan Streetman , Andrew Morton , Barry Song , Linux-MM , Shakeel Butt , Sergey Senozhatsky Content-Type: text/plain; charset="UTF-8" X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Thu, Jan 14, 2021 at 7:56 PM Minchan Kim wrote: > > On Thu, Jan 14, 2021 at 07:40:50PM +0100, Vitaly Wool wrote: > > On Thu, Jan 14, 2021 at 7:29 PM Minchan Kim wrote: > > > > > > On Fri, Dec 25, 2020 at 07:02:50PM +0800, Tian Tao wrote: > > > > add a flag to zpool, named is "can_sleep_mapped", and have it set true > > > > for zbud/z3fold, set false for zsmalloc. Then zswap could go the current > > > > path if the flag is true; and if it's false, copy data from src to a > > > > temporary buffer, then unmap the handle, take the mutex, process the > > > > buffer instead of src to avoid sleeping function called from atomic > > > > context. > > > > > > > > Signed-off-by: Tian Tao > > > > --- > > > > include/linux/zpool.h | 3 +++ > > > > mm/zpool.c | 13 +++++++++++++ > > > > mm/zswap.c | 50 +++++++++++++++++++++++++++++++++++++++++++++----- > > > > 3 files changed, 61 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/include/linux/zpool.h b/include/linux/zpool.h > > > > index 51bf430..e899701 100644 > > > > --- a/include/linux/zpool.h > > > > +++ b/include/linux/zpool.h > > > > @@ -73,6 +73,7 @@ u64 zpool_get_total_size(struct zpool *pool); > > > > * @malloc: allocate mem from a pool. > > > > * @free: free mem from a pool. > > > > * @shrink: shrink the pool. > > > > + * @sleep_mapped: whether zpool driver can sleep during map. > > > > > > I don't think it's a good idea. It just breaks zpool abstraction > > > in that it exposes internal implementation to user to avoid issue > > > zswap recently introduced. It also conflicts zpool_map_handle's > > > semantic. > > > > > > Rather than introducing another break in zpool due to the new > > > zswap feature recenlty introduced, zswap could introduce > > > CONFIG_ZSWAP_HW_COMPRESSOR. Once it's configured, zsmalloc could > > > be disabled. And with disabling CONFIG_ZSWAP_HW_COMPRESSOR, zswap > > > doesn't need to make any bounce buffer copy so that no existing > > > zsmalloc user will see performance regression. > > > > I believe it won't help that much -- the new compressor API presumes > > that the caller may sleep during compression and that will be an > > accident waiting to happen as long as we use it and keep the handle > > mapped in zsmalloc case. > > > > Or maybe I interpreted you wrong and you are suggesting re-introducing > > calls to the old API under this #ifdef, is that the case? > > Yub. zswap could abstract that part under #ifdef to keep old behavior. We can reconsider this option when zsmalloc implements reclaim callback. So far it's obviously too much a mess for a reason so weak. > > > > Best regards, > > Vitaly > > > > > > > > > * @map: map a handle. > > > > * @unmap: unmap a handle. > > > > * @total_size: get total size of a pool. > > > > @@ -100,6 +101,7 @@ struct zpool_driver { > > > > int (*shrink)(void *pool, unsigned int pages, > > > > unsigned int *reclaimed); > > > > > > > > + bool sleep_mapped; > > > > void *(*map)(void *pool, unsigned long handle, > > > > enum zpool_mapmode mm); > > > > void (*unmap)(void *pool, unsigned long handle); > > > > @@ -112,5 +114,6 @@ void zpool_register_driver(struct zpool_driver *driver); > > > > int zpool_unregister_driver(struct zpool_driver *driver); > > > > > > > > bool zpool_evictable(struct zpool *pool); > > > > +bool zpool_can_sleep_mapped(struct zpool *pool); > > > > > > > > #endif > > > > diff --git a/mm/zpool.c b/mm/zpool.c > > > > index 3744a2d..5ed7120 100644 > > > > --- a/mm/zpool.c > > > > +++ b/mm/zpool.c > > > > @@ -23,6 +23,7 @@ struct zpool { > > > > void *pool; > > > > const struct zpool_ops *ops; > > > > bool evictable; > > > > + bool can_sleep_mapped; > > > > > > > > struct list_head list; > > > > }; > > > > @@ -183,6 +184,7 @@ struct zpool *zpool_create_pool(const char *type, const char *name, gfp_t gfp, > > > > zpool->pool = driver->create(name, gfp, ops, zpool); > > > > zpool->ops = ops; > > > > zpool->evictable = driver->shrink && ops && ops->evict; > > > > + zpool->can_sleep_mapped = driver->sleep_mapped; > > > > > > > > if (!zpool->pool) { > > > > pr_err("couldn't create %s pool\n", type); > > > > @@ -393,6 +395,17 @@ bool zpool_evictable(struct zpool *zpool) > > > > return zpool->evictable; > > > > } > > > > > > > > +/** > > > > + * zpool_can_sleep_mapped - Test if zpool can sleep when do mapped. > > > > + * @zpool: The zpool to test > > > > + * > > > > + * Returns: true if zpool can sleep; false otherwise. > > > > + */ > > > > +bool zpool_can_sleep_mapped(struct zpool *zpool) > > > > +{ > > > > + return zpool->can_sleep_mapped; > > > > +} > > > > + > > > > MODULE_LICENSE("GPL"); > > > > MODULE_AUTHOR("Dan Streetman "); > > > > MODULE_DESCRIPTION("Common API for compressed memory storage"); > > > > diff --git a/mm/zswap.c b/mm/zswap.c > > > > index 182f6ad..67d4555 100644 > > > > --- a/mm/zswap.c > > > > +++ b/mm/zswap.c > > > > @@ -935,13 +935,20 @@ static int zswap_writeback_entry(struct zpool *pool, unsigned long handle) > > > > struct scatterlist input, output; > > > > struct crypto_acomp_ctx *acomp_ctx; > > > > > > > > - u8 *src; > > > > + u8 *src, *tmp; > > > > unsigned int dlen; > > > > int ret; > > > > struct writeback_control wbc = { > > > > .sync_mode = WB_SYNC_NONE, > > > > }; > > > > > > > > + if (!zpool_can_sleep_mapped(pool)) { > > > > + > > > > + tmp = kmalloc(entry->length, GFP_ATOMIC); > > > > + if (!tmp) > > > > + return -ENOMEM; > > > > + } > > > > + > > > > /* extract swpentry from data */ > > > > zhdr = zpool_map_handle(pool, handle, ZPOOL_MM_RO); > > > > swpentry = zhdr->swpentry; /* here */ > > > > @@ -979,6 +986,14 @@ static int zswap_writeback_entry(struct zpool *pool, unsigned long handle) > > > > dlen = PAGE_SIZE; > > > > src = (u8 *)zhdr + sizeof(struct zswap_header); > > > > > > > > + if (!zpool_can_sleep_mapped(pool)) { > > > > + > > > > + memcpy(tmp, src, entry->length); > > > > + src = tmp; > > > > + > > > > + zpool_unmap_handle(pool, handle); > > > > + } > > > > + > > > > mutex_lock(acomp_ctx->mutex); > > > > sg_init_one(&input, src, entry->length); > > > > sg_init_table(&output, 1); > > > > @@ -1033,7 +1048,11 @@ static int zswap_writeback_entry(struct zpool *pool, unsigned long handle) > > > > spin_unlock(&tree->lock); > > > > > > > > end: > > > > - zpool_unmap_handle(pool, handle); > > > > + if (zpool_can_sleep_mapped(pool)) > > > > + zpool_unmap_handle(pool, handle); > > > > + else > > > > + kfree(tmp); > > > > + > > > > return ret; > > > > } > > > > > > > > @@ -1235,7 +1254,7 @@ static int zswap_frontswap_load(unsigned type, pgoff_t offset, > > > > struct zswap_entry *entry; > > > > struct scatterlist input, output; > > > > struct crypto_acomp_ctx *acomp_ctx; > > > > - u8 *src, *dst; > > > > + u8 *src, *dst, *tmp; > > > > unsigned int dlen; > > > > int ret; > > > > > > > > @@ -1256,12 +1275,29 @@ static int zswap_frontswap_load(unsigned type, pgoff_t offset, > > > > goto freeentry; > > > > } > > > > > > > > + if (!zpool_can_sleep_mapped(entry->pool->zpool)) { > > > > + > > > > + tmp = kmalloc(entry->length, GFP_ATOMIC); > > > > + if (!tmp) { > > > > + ret = -ENOMEM; > > > > + goto freeentry; > > > > + } > > > > + } > > > > + > > > > /* decompress */ > > > > dlen = PAGE_SIZE; > > > > src = zpool_map_handle(entry->pool->zpool, entry->handle, ZPOOL_MM_RO); > > > > if (zpool_evictable(entry->pool->zpool)) > > > > src += sizeof(struct zswap_header); > > > > > > > > + if (!zpool_can_sleep_mapped(entry->pool->zpool)) { > > > > + > > > > + memcpy(tmp, src, entry->length); > > > > + src = tmp; > > > > + > > > > + zpool_unmap_handle(entry->pool->zpool, entry->handle); > > > > + } > > > > + > > > > acomp_ctx = raw_cpu_ptr(entry->pool->acomp_ctx); > > > > mutex_lock(acomp_ctx->mutex); > > > > sg_init_one(&input, src, entry->length); > > > > @@ -1271,7 +1307,11 @@ static int zswap_frontswap_load(unsigned type, pgoff_t offset, > > > > ret = crypto_wait_req(crypto_acomp_decompress(acomp_ctx->req), &acomp_ctx->wait); > > > > mutex_unlock(acomp_ctx->mutex); > > > > > > > > - zpool_unmap_handle(entry->pool->zpool, entry->handle); > > > > + if (zpool_can_sleep_mapped(entry->pool->zpool)) > > > > + zpool_unmap_handle(entry->pool->zpool, entry->handle); > > > > + else > > > > + kfree(tmp); > > > > + > > > > BUG_ON(ret); > > > > > > > > freeentry: > > > > @@ -1279,7 +1319,7 @@ static int zswap_frontswap_load(unsigned type, pgoff_t offset, > > > > zswap_entry_put(tree, entry); > > > > spin_unlock(&tree->lock); > > > > > > > > - return 0; > > > > + return ret; > > > > } > > > > > > > > /* frees an entry in zswap */ > > > > -- > > > > 2.7.4 > > > > > > > >