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 Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 70DBDC001DC for ; Thu, 27 Jul 2023 18:30:06 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id E9FC16B0075; Thu, 27 Jul 2023 14:30:05 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id E4F6A6B0078; Thu, 27 Jul 2023 14:30:05 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id D17AE6B007B; Thu, 27 Jul 2023 14:30:05 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id C17C26B0075 for ; Thu, 27 Jul 2023 14:30:05 -0400 (EDT) Received: from smtpin08.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id 6F98640511 for ; Thu, 27 Jul 2023 18:30:05 +0000 (UTC) X-FDA: 81058231170.08.34783E7 Received: from mail-ej1-f42.google.com (mail-ej1-f42.google.com [209.85.218.42]) by imf05.hostedemail.com (Postfix) with ESMTP id 5F780100029 for ; Thu, 27 Jul 2023 18:30:02 +0000 (UTC) Authentication-Results: imf05.hostedemail.com; dkim=pass header.d=google.com header.s=20221208 header.b=lxY8Opux; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf05.hostedemail.com: domain of yosryahmed@google.com designates 209.85.218.42 as permitted sender) smtp.mailfrom=yosryahmed@google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1690482602; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=gQuUTbCVCGcnG+7JdbJB28lXA8NxfAQJ6tDUsY15cRw=; b=EHjm5NX35+eTwYOTq3WDo6+zoqoXVX55M89fYSLo5yoVMGnIgsDB6y3S5+6Doa6QdNq1dj 1CRPSSk5IxrOyWONQe8cxRJN+Uz/ifQDO8MC/iHsjA6/Aa9oyHONkhROyjwX389gOh7H+L pzJG+BXnMIJ3CHzZCimvI8pqy5tTtz8= ARC-Authentication-Results: i=1; imf05.hostedemail.com; dkim=pass header.d=google.com header.s=20221208 header.b=lxY8Opux; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf05.hostedemail.com: domain of yosryahmed@google.com designates 209.85.218.42 as permitted sender) smtp.mailfrom=yosryahmed@google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1690482602; a=rsa-sha256; cv=none; b=ubIWJ6VbhLoUZqvpQsQq4jXXZ7Re68BwJW6aw6Uwi9Bocb4nNdGLcEmo+h4j0gRLjqzVJN z3U8ku5Fw/ihYpx1AjFpQ2AzVyx0Y7h2gqzpe4r4iUlFrQ1nrpFM2mPkWWwcdcfzVU+JVH MHIbItJbevoa7sbKbahZ7figlWBSUCQ= Received: by mail-ej1-f42.google.com with SMTP id a640c23a62f3a-98df3dea907so169288666b.3 for ; Thu, 27 Jul 2023 11:30:01 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20221208; t=1690482601; x=1691087401; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=gQuUTbCVCGcnG+7JdbJB28lXA8NxfAQJ6tDUsY15cRw=; b=lxY8OpuxyOl+aLXedM/7YSYB5xsPDvxBBgSCoal9QVpCqjXJu0vDvSLXSlAkgQ+knH vp6JJeN6C6tHemFkC2E5R86qmswabSEG8Tw/xKlntiEZVwCUUX3CcnTJ95DcIzJ5jZDH h1nNDfdKoVAAvKzXmkwd/6LwvJCYVoBY4jotE27is864RugaIoNNdjKLfnH4PHrncbro 8hxAQT+Qrh3de9u7hE/YK28Tp8qpRU+ncMIx6on8qjsyZtRPZdZTwiWIN9Jyce+YQsOb +xdlsW9DUxE002/zAPopJ+x63wqQIyuENJhWj7MU2Pdbjl4U1Dvbb3wgfOrIhMgEIbPP y91Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1690482601; x=1691087401; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=gQuUTbCVCGcnG+7JdbJB28lXA8NxfAQJ6tDUsY15cRw=; b=V9DAabL3NmvImV6LCkSpoj9g7C0K/3awYbPJcLMSWqdGqsOvIQgvlMdppVP79UcvHV Vxqmxvc91AWv5B0oj9hIeR4gsd3UuP/d72fwT4qHYaIszhWENwTQGL+dJY33n1cbNkrE 3eALB4KAWO86cPAPvFsGfOOpAIn0uYypgjxVi9xZXXhNczwG17IDlXecpFphqh5ynDx1 PNStLrgs6veQ+yP56SGKij2A+kI47Otz4cTUIg9EcCDGMUtJRb8/z+1jbOeOW+HFBYY5 0xn651T75XMuLyyn+ekWfGFXIrfQ8H30hK6NjGaxHDOc7eM8k4jeaCp9I7BHfWYdfqj0 ElPw== X-Gm-Message-State: ABy/qLbj2NgCWYoFSKuJ16inCH+KHcoV+Cj3xPpfxFHAQ7o0hAw2ZMvP yU8oPL4N3qOiGODk2WsENMiNOFdDWOcSRuTPQPF92Q== X-Google-Smtp-Source: APBJJlFPEsT8C6iMwpUThAHnHk70lIkWwoq2M5crzE+bNa7W/QtZewT74DMVt0WzVSWjfhDht/ItI/D+MJbQON76Phw= X-Received: by 2002:a17:906:3f0c:b0:99b:44aa:fae0 with SMTP id c12-20020a1709063f0c00b0099b44aafae0mr19137ejj.21.1690482600566; Thu, 27 Jul 2023 11:30:00 -0700 (PDT) MIME-Version: 1.0 References: <20230727162343.1415598-1-hannes@cmpxchg.org> <20230727162343.1415598-4-hannes@cmpxchg.org> In-Reply-To: <20230727162343.1415598-4-hannes@cmpxchg.org> From: Yosry Ahmed Date: Thu, 27 Jul 2023 11:29:24 -0700 Message-ID: Subject: Re: [PATCH 3/3] mm: zswap: kill zswap_get_swap_cache_page() To: Johannes Weiner Cc: Andrew Morton , Nhat Pham , Domenico Cerasuolo , linux-mm@kvack.org, linux-kernel@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: 5F780100029 X-Rspam-User: X-Rspamd-Server: rspam04 X-Stat-Signature: nw5466azuro5kurbh9rnrfdsmw6dezjf X-HE-Tag: 1690482601-322860 X-HE-Meta: U2FsdGVkX1/yket6eriToZwXc4W6qV1+qdBgOHnM30+ZMq+VgxpV8HRfdboE30H6GdY83HCZFhnRI6lyeEchoI6xbof1jTHb0VI4+OLGpNLWVQKZPYxObmlNjc5sN2zyTVqYFofAYgM9umH4LNVrDceMC4fyHZXfwIeSoqDNDbnnKbgajPyzBcgYp2rFlb2XzBSgohRYTaS4jT4v9jZMXadVyMBsrlDu8GBP1vOvCUFlkMQUBmNfYgO0P66vnCGcjaNK5OT7GVaCOMJuhr7n0rNERyvlaMOajz+2wyD8/6JoCEhWiFd8X5oFTW1ZhY4NNjGP4YTyV+wZQZ2lS/Db2uLhurEmb2tRczr09Eec97eJf0jTlS173dSgzvGnRlt9PO5jE4rScag20rJxaCXApLEwwgcrTP8fP4ZOXghfLmh/L2XjcO06pRm1AhyKHUx+vVrtlJKP31imnDTKz83YvbAbhplTPtNy5Uip/xgvomOHlkCg8arUicdM+fWfVbOguANkdpcRKhsiXHmwBIFr773qpe17GNSmDjUmRCdMW2v2HV4m94Rf5lzG3JK3tad1z8jS6cVMPK/2YeYRRUF3DuIizA1Dk+5cXVZcB3pAtB2Fq0zX9qhoTC5W6Qf/CgnVgH2MNvw3N/AdyWl5gWUegeYFUse1ROITRdZ2uFdOczEpuGR6GeQMpQPb5Z393MZQYWTZprjfdmiRPrCiDsUI+qdTp28m/HYnN4gGuGhH5YgQmHYCq8iCKb2IUVHQrJWYT3yEETKRH1L11zfj7cgQ+t8W+w6MzfXwo9ShAmTlzIwGT1f8Y8TqOButNrlzmNsH5fRhoGCOQ7QVglQ9EZwSePq4TZcgZcano4kZicrcDgopce2WxDRN8QXRgM3uQTEM67GGX1JQMGGqE1Hwv6waKtTz3zJwyTdkX9Q1ipIdSAj2jvbpgSKjtUuyb6Zljh8MSDVNhluLDZKfSpNsO9a mcvesGtd 3FL/Pqy+FeKYbSDymPfYvSL2RDx/C4DJJkHSLnmKvf12Js8NDpnJMIZXkPwzSlFHYfmsJSNxSVhi2xmbvvx0fqrahDlbLGlTYfqvyoVCn6d3aoJIGh71I8+JLHZOquFBAzvxUGJ5YvIwpHPF60BKkLRP1orkIhVsNHpAnICKzcM3bY/jJlZYaIi24j19IUT9QA73BUH1yEIKs+vKO2lErp62ICkxcuQvuX8gW7GBSGI81EQhzDnkfTeKtzmfN6Tc8kY7BDICElWH/+BUC92Gr4Un12XM59ROwtpgXwNcNL0kaqLffQ3F0VEofnDwBfJEeHFAFFQhHrJT1n8PmBMhfQUtqkyfH8dTNimRmSjeLHUBHWSpZzSLvcvSaI0WewXuIDsMa 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, Jul 27, 2023 at 9:23=E2=80=AFAM Johannes Weiner wrote: > > The __read_swap_cache_async() interface isn't more difficult to > understand than what the helper abstracts. Save the indirection and a > level of indentation for the primary work of the writeback func. > > Signed-off-by: Johannes Weiner Arguably any abstraction to the swap code is better than dealing with the swap code, but I am not against this :P The diffstat looks nice and the code looks correct to me. Reviewed-by: Yosry Ahmed > --- > mm/zswap.c | 142 ++++++++++++++++++++--------------------------------- > 1 file changed, 53 insertions(+), 89 deletions(-) > > diff --git a/mm/zswap.c b/mm/zswap.c > index e34ac89e6098..bba4ead672be 100644 > --- a/mm/zswap.c > +++ b/mm/zswap.c > @@ -1016,43 +1016,6 @@ static int zswap_enabled_param_set(const char *val= , > /********************************* > * writeback code > **********************************/ > -/* return enum for zswap_get_swap_cache_page */ > -enum zswap_get_swap_ret { > - ZSWAP_SWAPCACHE_NEW, > - ZSWAP_SWAPCACHE_EXIST, > - ZSWAP_SWAPCACHE_FAIL, > -}; > - > -/* > - * zswap_get_swap_cache_page > - * > - * This is an adaption of read_swap_cache_async() > - * > - * This function tries to find a page with the given swap entry > - * in the swapper_space address space (the swap cache). If the page > - * is found, it is returned in retpage. Otherwise, a page is allocated, > - * added to the swap cache, and returned in retpage. > - * > - * If success, the swap cache page is returned in retpage > - * Returns ZSWAP_SWAPCACHE_EXIST if page was already in the swap cache > - * Returns ZSWAP_SWAPCACHE_NEW if the new page needs to be populated, > - * the new page is added to swapcache and locked > - * Returns ZSWAP_SWAPCACHE_FAIL on error > - */ > -static int zswap_get_swap_cache_page(swp_entry_t entry, > - struct page **retpage) > -{ > - bool page_was_allocated; > - > - *retpage =3D __read_swap_cache_async(entry, GFP_KERNEL, > - NULL, 0, &page_was_allocated); > - if (page_was_allocated) > - return ZSWAP_SWAPCACHE_NEW; > - if (!*retpage) > - return ZSWAP_SWAPCACHE_FAIL; > - return ZSWAP_SWAPCACHE_EXIST; > -} > - > /* > * Attempts to free an entry by adding a page to the swap cache, > * decompressing the entry data into the page, and issuing a > @@ -1073,7 +1036,7 @@ static int zswap_writeback_entry(struct zswap_entry= *entry, > struct scatterlist input, output; > struct crypto_acomp_ctx *acomp_ctx; > struct zpool *pool =3D entry->pool->zpool; > - > + bool page_was_allocated; > u8 *src, *tmp =3D NULL; > unsigned int dlen; > int ret; > @@ -1088,65 +1051,66 @@ static int zswap_writeback_entry(struct zswap_ent= ry *entry, > } > > /* try to allocate swap cache page */ > - switch (zswap_get_swap_cache_page(swpentry, &page)) { > - case ZSWAP_SWAPCACHE_FAIL: /* no memory or invalidate happened */ > + page =3D __read_swap_cache_async(swpentry, GFP_KERNEL, NULL, 0, > + &page_was_allocated); > + if (!page) { > ret =3D -ENOMEM; > goto fail; > + } > > - case ZSWAP_SWAPCACHE_EXIST: > - /* page is already in the swap cache, ignore for now */ > + /* Found an existing page, we raced with load/swapin */ > + if (!page_was_allocated) { > put_page(page); > ret =3D -EEXIST; > goto fail; > + } > > - case ZSWAP_SWAPCACHE_NEW: /* page is locked */ > - /* > - * Having a local reference to the zswap entry doesn't ex= clude > - * swapping from invalidating and recycling the swap slot= . Once > - * the swapcache is secured against concurrent swapping t= o and > - * from the slot, recheck that the entry is still current= before > - * writing. > - */ > - spin_lock(&tree->lock); > - if (zswap_rb_search(&tree->rbroot, swp_offset(entry->swpe= ntry)) !=3D entry) { > - spin_unlock(&tree->lock); > - delete_from_swap_cache(page_folio(page)); > - ret =3D -ENOMEM; > - goto fail; > - } > + /* > + * Page is locked, and the swapcache is now secured against > + * concurrent swapping to and from the slot. Verify that the > + * swap entry hasn't been invalidated and recycled behind our > + * backs (our zswap_entry reference doesn't prevent that), to > + * avoid overwriting a new swap page with old compressed data. > + */ > + spin_lock(&tree->lock); > + if (zswap_rb_search(&tree->rbroot, swp_offset(entry->swpentry)) != =3D entry) { > spin_unlock(&tree->lock); > + delete_from_swap_cache(page_folio(page)); > + ret =3D -ENOMEM; > + goto fail; > + } > + spin_unlock(&tree->lock); > > - /* decompress */ > - acomp_ctx =3D raw_cpu_ptr(entry->pool->acomp_ctx); > - dlen =3D PAGE_SIZE; > + /* decompress */ > + acomp_ctx =3D raw_cpu_ptr(entry->pool->acomp_ctx); > + dlen =3D PAGE_SIZE; > > - src =3D zpool_map_handle(pool, entry->handle, ZPOOL_MM_RO= ); > - if (!zpool_can_sleep_mapped(pool)) { > - memcpy(tmp, src, entry->length); > - src =3D tmp; > - zpool_unmap_handle(pool, entry->handle); > - } > + src =3D zpool_map_handle(pool, entry->handle, ZPOOL_MM_RO); > + if (!zpool_can_sleep_mapped(pool)) { > + memcpy(tmp, src, entry->length); > + src =3D tmp; > + zpool_unmap_handle(pool, entry->handle); > + } > > - mutex_lock(acomp_ctx->mutex); > - sg_init_one(&input, src, entry->length); > - sg_init_table(&output, 1); > - sg_set_page(&output, page, PAGE_SIZE, 0); > - acomp_request_set_params(acomp_ctx->req, &input, &output,= entry->length, dlen); > - ret =3D crypto_wait_req(crypto_acomp_decompress(acomp_ctx= ->req), &acomp_ctx->wait); > - dlen =3D acomp_ctx->req->dlen; > - mutex_unlock(acomp_ctx->mutex); > - > - if (!zpool_can_sleep_mapped(pool)) > - kfree(tmp); > - else > - zpool_unmap_handle(pool, entry->handle); > + mutex_lock(acomp_ctx->mutex); > + sg_init_one(&input, src, entry->length); > + sg_init_table(&output, 1); > + sg_set_page(&output, page, PAGE_SIZE, 0); > + acomp_request_set_params(acomp_ctx->req, &input, &output, entry->= length, dlen); > + ret =3D crypto_wait_req(crypto_acomp_decompress(acomp_ctx->req), = &acomp_ctx->wait); > + dlen =3D acomp_ctx->req->dlen; > + mutex_unlock(acomp_ctx->mutex); > + > + if (!zpool_can_sleep_mapped(pool)) > + kfree(tmp); > + else > + zpool_unmap_handle(pool, entry->handle); > > - BUG_ON(ret); > - BUG_ON(dlen !=3D PAGE_SIZE); > + BUG_ON(ret); > + BUG_ON(dlen !=3D PAGE_SIZE); > > - /* page is up to date */ > - SetPageUptodate(page); > - } > + /* page is up to date */ > + SetPageUptodate(page); > > /* move it to the tail of the inactive list after end_writeback *= / > SetPageReclaim(page); > @@ -1157,16 +1121,16 @@ static int zswap_writeback_entry(struct zswap_ent= ry *entry, > zswap_written_back_pages++; > > return ret; > + > fail: > if (!zpool_can_sleep_mapped(pool)) > kfree(tmp); > > /* > - * if we get here due to ZSWAP_SWAPCACHE_EXIST > - * a load may be happening concurrently. > - * it is safe and okay to not free the entry. > - * it is also okay to return !0 > - */ > + * If we get here because the page is already in swapcache, a > + * load may be happening concurrently. It is safe and okay to > + * not free the entry. It is also okay to return !0. > + */ > return ret; > } > > -- > 2.41.0 >