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=-17.1 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_1 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 17E35C433DB for ; Fri, 25 Dec 2020 02:34:04 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 4E745207F7 for ; Fri, 25 Dec 2020 02:34:02 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 4E745207F7 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=huawei.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 5010A8D007F; Thu, 24 Dec 2020 21:34:02 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 4B19E8D0061; Thu, 24 Dec 2020 21:34:02 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 3C73E8D007F; Thu, 24 Dec 2020 21:34:02 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0222.hostedemail.com [216.40.44.222]) by kanga.kvack.org (Postfix) with ESMTP id 25B4F8D0061 for ; Thu, 24 Dec 2020 21:34:02 -0500 (EST) Received: from smtpin20.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay01.hostedemail.com (Postfix) with ESMTP id DEC3E180AD815 for ; Fri, 25 Dec 2020 02:34:01 +0000 (UTC) X-FDA: 77630234682.20.cat86_6000f7527476 Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin20.hostedemail.com (Postfix) with ESMTP id C49B1180C07AB for ; Fri, 25 Dec 2020 02:34:01 +0000 (UTC) X-HE-Tag: cat86_6000f7527476 X-Filterd-Recvd-Size: 8955 Received: from szxga05-in.huawei.com (szxga05-in.huawei.com [45.249.212.191]) by imf26.hostedemail.com (Postfix) with ESMTP for ; Fri, 25 Dec 2020 02:34:00 +0000 (UTC) Received: from DGGEMS407-HUB.china.huawei.com (unknown [172.30.72.59]) by szxga05-in.huawei.com (SkyGuard) with ESMTP id 4D29w70d4KzM8ck; Fri, 25 Dec 2020 10:32:55 +0800 (CST) Received: from [127.0.0.1] (10.40.188.144) by DGGEMS407-HUB.china.huawei.com (10.3.19.207) with Microsoft SMTP Server id 14.3.498.0; Fri, 25 Dec 2020 10:33:46 +0800 Subject: Re: [PATCH] mm/zswap: Fix the compatibility of zsmalloc and zswap To: Vitaly Wool , Tian Tao CC: Dan Streetman , Seth Jennings , Andrew Morton , Minchan Kim , NitinGupta , Linux-MM References: <1608812141-20238-1-git-send-email-tiantao6@hisilicon.com> From: "tiantao (H)" Message-ID: <4d981b16-42a2-ac56-402e-e130b3a6314a@huawei.com> Date: Fri, 25 Dec 2020 10:33:46 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format=flowed X-Originating-IP: [10.40.188.144] X-CFilter-Loop: Reflected Content-Transfer-Encoding: quoted-printable 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: =E5=9C=A8 2020/12/24 22:16, Vitaly Wool =E5=86=99=E9=81=93: > On Thu, Dec 24, 2020 at 1:16 PM 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 curre= nt >> 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 | 2 ++ >> mm/z3fold.c | 1 + >> mm/zbud.c | 1 + >> mm/zpool.c | 13 +++++++++++++ >> mm/zsmalloc.c | 1 + >> mm/zswap.c | 23 +++++++++++++++++++++-- >> 6 files changed, 39 insertions(+), 2 deletions(-) >> >> diff --git a/include/linux/zpool.h b/include/linux/zpool.h >> index 51bf430..a354f4fe 100644 >> --- a/include/linux/zpool.h >> +++ b/include/linux/zpool.h >> @@ -82,6 +82,7 @@ u64 zpool_get_total_size(struct zpool *pool); >> */ >> struct zpool_driver { >> char *type; >> + bool sleep_mapped; > Could you please add this somewhat further down, close to map / unmap > callbacks, and add a description of the field above, too? >> struct module *owner; >> atomic_t refcount; >> struct list_head list; >> @@ -112,5 +113,6 @@ void zpool_register_driver(struct zpool_driver *dr= iver); >> 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/z3fold.c b/mm/z3fold.c >> index dacb0d7..234b46f 100644 >> --- a/mm/z3fold.c >> +++ b/mm/z3fold.c >> @@ -1778,6 +1778,7 @@ static u64 z3fold_zpool_total_size(void *pool) >> >> static struct zpool_driver z3fold_zpool_driver =3D { >> .type =3D "z3fold", >> + .sleep_mapped =3D true, >> .owner =3D THIS_MODULE, >> .create =3D z3fold_zpool_create, >> .destroy =3D z3fold_zpool_destroy, >> diff --git a/mm/zbud.c b/mm/zbud.c >> index c49966e..7ec5f27 100644 >> --- a/mm/zbud.c >> +++ b/mm/zbud.c >> @@ -203,6 +203,7 @@ static u64 zbud_zpool_total_size(void *pool) >> >> static struct zpool_driver zbud_zpool_driver =3D { >> .type =3D "zbud", >> + .sleep_mapped =3D true, >> .owner =3D THIS_MODULE, >> .create =3D zbud_zpool_create, >> .destroy =3D zbud_zpool_destroy, >> 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 =3D driver->create(name, gfp, ops, zpool); >> zpool->ops =3D ops; >> zpool->evictable =3D driver->shrink && ops && ops->evict; >> + zpool->can_sleep_mapped =3D 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/zsmalloc.c b/mm/zsmalloc.c >> index 7289f50..a991d3c 100644 >> --- a/mm/zsmalloc.c >> +++ b/mm/zsmalloc.c >> @@ -440,6 +440,7 @@ static u64 zs_zpool_total_size(void *pool) >> >> static struct zpool_driver zs_zpool_driver =3D { >> .type =3D "zsmalloc", >> + .sleep_mapped =3D false, > You don't have to add this, it will be false if not explicitly specifie= d. > >> .owner =3D THIS_MODULE, >> .create =3D zs_zpool_create, >> .destroy =3D zs_zpool_destroy, >> diff --git a/mm/zswap.c b/mm/zswap.c >> index 182f6ad..51b033a 100644 >> --- a/mm/zswap.c >> +++ b/mm/zswap.c >> @@ -1235,7 +1235,7 @@ static int zswap_frontswap_load(unsigned type, p= goff_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 +1256,27 @@ static int zswap_frontswap_load(unsigned type,= pgoff_t offset, >> goto freeentry; >> } >> >> + if (!zpool_can_sleep_mapped(entry->pool->zpool)) { >> + >> + tmp =3D kzalloc(entry->length, GFP_KERNEL); > Do we really need kzalloc here? the buffer will be written to in a few > moments anyway. > Also, we allocate a small buffer for a short while, and this is likely > to be a performance critical path, so GFP_ATOMIC looks more > appropriate. > >> + if (!tmp) >> + return -ENOMEM; > You can't just return here, you need to call zswap_entry_put() first. > > Since this is obviously not the final version of your patch, could you > please split it in 2 the next time? I'd prefer setting flags to > specific backends in a separate patch. Thanks for helping to review the code. I will post a new patch of my understanding according to you advice, if=20 I do not understand the right,=C2=A0 you can=C2=A0 help point out, I continue to m= odify > > Best regards, > Vitaly > >> + } >> + >> /* decompress */ >> dlen =3D PAGE_SIZE; >> src =3D zpool_map_handle(entry->pool->zpool, entry->handle, Z= POOL_MM_RO); >> if (zpool_evictable(entry->pool->zpool)) >> src +=3D sizeof(struct zswap_header); >> >> + if (!zpool_can_sleep_mapped(entry->pool->zpool)) { >> + >> + memcpy(tmp, src, entry->length); >> + src =3D tmp; >> + >> + zpool_unmap_handle(entry->pool->zpool, entry->handle); >> + } >> + >> acomp_ctx =3D raw_cpu_ptr(entry->pool->acomp_ctx); >> mutex_lock(acomp_ctx->mutex); >> sg_init_one(&input, src, entry->length); >> @@ -1271,7 +1286,11 @@ static int zswap_frontswap_load(unsigned type, = pgoff_t offset, >> ret =3D crypto_wait_req(crypto_acomp_decompress(acomp_ctx->re= q), &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: >> -- >> 2.7.4 >> > . >