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 E72DDE77188 for ; Wed, 8 Jan 2025 04:17:35 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 5AC296B0088; Tue, 7 Jan 2025 23:17:35 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 55C686B0089; Tue, 7 Jan 2025 23:17:35 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 3FD0C6B008A; Tue, 7 Jan 2025 23:17:35 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id 20BA76B0088 for ; Tue, 7 Jan 2025 23:17:35 -0500 (EST) Received: from smtpin22.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id BA8FB120EAA for ; Wed, 8 Jan 2025 04:17:34 +0000 (UTC) X-FDA: 82982975628.22.F80A83E Received: from mail-qt1-f177.google.com (mail-qt1-f177.google.com [209.85.160.177]) by imf11.hostedemail.com (Postfix) with ESMTP id E74EC40003 for ; Wed, 8 Jan 2025 04:17:32 +0000 (UTC) Authentication-Results: imf11.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=aBTwWJsH; spf=pass (imf11.hostedemail.com: domain of yosryahmed@google.com designates 209.85.160.177 as permitted sender) smtp.mailfrom=yosryahmed@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1736309852; a=rsa-sha256; cv=none; b=wzvX68owIFAFPjbqvePuNUHfzJddbyvCb71l/TfLU4vc1pPY2JeOOeQSjl9mKcCQTCsy0i AJt+goYJqOAaDcu1TlnOgGgvVm+VCa3/1N88DRmPm2VqY6Uv0qEqgPPG+nMPAsSGqbbjMg vzXxBEv2Ilsj3qGb27vzpVpsJN4yOHg= ARC-Authentication-Results: i=1; imf11.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=aBTwWJsH; spf=pass (imf11.hostedemail.com: domain of yosryahmed@google.com designates 209.85.160.177 as permitted sender) smtp.mailfrom=yosryahmed@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1736309852; 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: in-reply-to:in-reply-to:references:references:dkim-signature; bh=/Yc9mZAfT6V6ArAXeLUQVlPLk9ybss1V+MmISZuZ1RI=; b=7l8RiaFbjx8NYmM+Bb9Iy37ZfSOatd0IwLUSg09LMAFIw06N/fPjJkaIV0Uka+DB8koqzE 9jhthPlepvp6Ti11BQRyRLJHlyzqOKEBRxpSokN/70j4L8t8JCysB7lX4b3cvMxLQJfTba izXUIGPX7gc8zDWRqH3d3EWqn18OeD4= Received: by mail-qt1-f177.google.com with SMTP id d75a77b69052e-467a3f1e667so96098601cf.0 for ; Tue, 07 Jan 2025 20:17:32 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1736309852; x=1736914652; darn=kvack.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=/Yc9mZAfT6V6ArAXeLUQVlPLk9ybss1V+MmISZuZ1RI=; b=aBTwWJsHcvol18KqcdJiEI7j9TcV6AdlT76EfWCCcr47TIfe1T3T8V2IqlnikZgPU+ Lbsr6YJIf7i+01HCmIi7ovcuj0teIvPUuPC/KdaQUfigOIQsqgnQ6/amGJlDKmRywBT8 4r3h5Rr0w3gBA0b9kOp1WQsWkbqK3z9imsEPZ4OfcI+JgWwsLQ6Ns6dmUALj2p+TInFX KBCHdbeGLAq15pqJ/x9In3Ict2W0morGxdBtuomlLSOgeSlWNdf7bwzI+/zc9xB572ar HQUTXrbirji1J3DStOP2/nwnsdekz6xXnpHHtDaz3A40rGqsYOVVMpew/Y8ybadd/y3f MWJA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1736309852; x=1736914652; h=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=/Yc9mZAfT6V6ArAXeLUQVlPLk9ybss1V+MmISZuZ1RI=; b=iOwNG/4wjgH+Qe0SSZcjeSBLaY0BHIjWPWSGRCEc12mBWqr0J80HXA6w5K/d6fufas 0YfP0CZu0i/bNeC6nGTik+7LPbAzJYnk3qNvS2RmtU/WNAIIf10GxXGpkav5bQHKP7gN 0wCieujajIwE2ZZ4An7mQqw8aU1oAJMmbB+BzbuTfXKf9ahaTC9IKrkY3TmDWrsbJRAt j7Jy777qH/cpaB3cFpqCWgfzDT3ufYpYQBstK3teXxbS4nms1pD779X7YRkp73CVYdsv DY4TLT5jZBADqSW9Kuig8uNfmboMWhTNuGBmvI6dgbrh9peUsyzAmdnUe4+H6Qtyupe5 8cUA== X-Forwarded-Encrypted: i=1; AJvYcCWoRrgHfol4VAmSZOWNNaRyF9pAOgl4Kt93ISFUNRhZ8YdSmpNkQWRB5wS9SLDmVQjkCH69NIF4HQ==@kvack.org X-Gm-Message-State: AOJu0YxCifI9T7gSDw2ylm1cBqMJ4Neql5wgnWzKtKDQzBwZrV4CWQ14 is+CFDuqgTufY72LL9FQy/IBULeFMIlZQuTeriXQ2hrOt7qqpzcR8vE6Okv62kveD9U981FimGB +RIMHWvu7yjvFEASt+ybcCpFcH9HZzsnz0JTfShO23Nx3A/t1pVSJ X-Gm-Gg: ASbGncvGlhk9X1KA5lNYiMlzfBfY2sXcWTNdYHGFUvYB5XlPgh8L4QnoqSnuit7wqh4 xEcYv9eMyREKJHnxnkhoN0nvYenu81Jv7MOI= X-Google-Smtp-Source: AGHT+IHubh8JdlZy8enWlyA4v+Thqive2415xKAhHBXZciV2IK35dqZnjO15cZfeq2i95UJI4vfbjffy/yWdqgjY4ro= X-Received: by 2002:a05:622a:294:b0:467:87f6:383 with SMTP id d75a77b69052e-46c710f9ff0mr27571611cf.52.1736309851846; Tue, 07 Jan 2025 20:17:31 -0800 (PST) MIME-Version: 1.0 References: <20241221063119.29140-1-kanchana.p.sridhar@intel.com> <20241221063119.29140-11-kanchana.p.sridhar@intel.com> In-Reply-To: From: Yosry Ahmed Date: Tue, 7 Jan 2025 20:16:55 -0800 X-Gm-Features: AbW1kvajCmHZRI_eSFFMKODBssd2ouTiX22XclorxGeJUv0xa9N1AP9Ex1X-T1M Message-ID: Subject: Re: [PATCH v5 10/12] mm: zswap: Allocate pool batching resources if the crypto_alg supports batching. To: "Sridhar, Kanchana P" Cc: "linux-kernel@vger.kernel.org" , "linux-mm@kvack.org" , "hannes@cmpxchg.org" , "nphamcs@gmail.com" , "chengming.zhou@linux.dev" , "usamaarif642@gmail.com" , "ryan.roberts@arm.com" , "21cnbao@gmail.com" <21cnbao@gmail.com>, "akpm@linux-foundation.org" , "linux-crypto@vger.kernel.org" , "herbert@gondor.apana.org.au" , "davem@davemloft.net" , "clabbe@baylibre.com" , "ardb@kernel.org" , "ebiggers@google.com" , "surenb@google.com" , "Accardi, Kristen C" , "Feghali, Wajdi K" , "Gopal, Vinodh" Content-Type: text/plain; charset="UTF-8" X-Rspamd-Queue-Id: E74EC40003 X-Stat-Signature: ugttxecoi6hnhnmfyr8fbzco8ngem9cf X-Rspam-User: X-Rspamd-Server: rspam09 X-HE-Tag: 1736309852-605395 X-HE-Meta: U2FsdGVkX1/uumAHwJ9L13lYlYuetW670Gvj6YfRPQGidWu9mE3MvwBmVz53O3nwzyct41PY8GzWiE49+y5xExAABRWbdbeZLjyltqEVtdmRtUUXrtGlmxQZ/1bilBXnNReqAMItB1uWRwUqLN5cSN3ARIyW6KvzPPFCrUnqEJ+yNdmBLLdXLJ7piOjJ3j3EjSbU2KYi7ItwJjtj1RHtcWnQNb7JPqk9EWc/fd3Kx4bK2A9is3fei8clD+CJKXdyGDyd1nZn3D2l/ytEgmSxakg6Ui17BEO/Q7nd8AMDjk7ZynQVbBLDfd1NZTvWKzGzRyI9ObvbjevoyItQpvXad9lvBjsGywtpMtL3VXp3sFKUsvkqWD2VHppugEgsDmVn8TbkNctzCAhlIGX6HKPRTwsrAvuCTaGyCSpW5FsWoW66avT73HU0Tfbig7Y2Ii/Zl5oYhoPXjNjAlKf0l++rADOBZ3tWT6kC5/86ivgeBar2m207MioL2xL9+bdomQGEaDfQKZkmCPeU/4s2c7VC/4sJA0//xCz8oAISbT35wFAmVs4AZ0Tk+UT+XBt/ezs/tLTFXV92TbevVB9nLOImduwLLX0Qyg/zDAzPTf4Bzv/3Lcr0GQeg5m7vwohS10VV5GVV4vYXF8d4a4Z27WBxKnImR0K7G1CoKsLyiq1a4L8N46UMJj3AAv2RzbbsIuJuISwcvPq8IVVJPDPdL1IKEdD9NaDSEsGx0LBYPke4xcWmrTcCk+Q4KxYGuwUrE2ZXl7KShsJtJe1YDbCQxtaTx/R+hFGAnmR9zXoIgnEpf94hDpN+xzMvpC6ensuj0YXwQftb9WltcpQWNs8atpNKtSD28xQE7H6ogbto51CRbg9+kMJFt8zWLUZg4eusD3gH3pgRghJuTPpCLudd/ijsM0mLKPDFL9pyNAqbk0xet5LtFq8oeA3Hw6suGsP6NFG06AJo/Myo6YCOynr0FTj sHozd5Ns V8uixeGNWhIhMIad+fNUHY83q0si6E7hDw6RFawDXBekGgw1otWNC+ddNji+U+oQ/oUfZdpi23kOzQneuvjqFegkhH0GWx+LxkS5MZefOu33itR+0eVCa0ApQUlFcyuOijdKN68c6a/QNyFC9yrFMpLHlf/Hnph2FfikSbkczoieUjoz1T/vpFL6Vnx3IEG4mKkIB8tJYjYdx2yNVs0upDP8Cg9gFjRHS1XrBydmqISBW9AiD1GIQ1aTjbdcjpWxFMYi6EUQWVCcq80faqeCk0lQ+nLvmQt5dOaaK 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: List-Subscribe: List-Unsubscribe: [..] > > > > > + } > > > + > > > + acomp_ctx->buffers = kmalloc_node(nr_reqs * sizeof(u8 *), > > GFP_KERNEL, cpu_to_node(cpu)); > > > > Can we use kcalloc_node() here? > > I was wondering if the performance penalty of the kcalloc_node() is acceptable > because the cpu onlining happens infrequently? If so, it appears zero-initializing > the allocated memory will help in the cleanup code suggestion in your subsequent > comment. I don't think zeroing in this path would be a problem. > > > > > > + if (!acomp_ctx->buffers) > > > + goto buf_fail; > > > + > > > + for (i = 0; i < nr_reqs; ++i) { > > > + acomp_ctx->buffers[i] = kmalloc_node(PAGE_SIZE * 2, > > GFP_KERNEL, cpu_to_node(cpu)); > > > + if (!acomp_ctx->buffers[i]) { > > > + for (j = 0; j < i; ++j) > > > + kfree(acomp_ctx->buffers[j]); > > > + kfree(acomp_ctx->buffers); > > > + ret = -ENOMEM; > > > + goto buf_fail; > > > + } > > > + } > > > + > > > + acomp_ctx->reqs = kmalloc_node(nr_reqs * sizeof(struct acomp_req > > *), GFP_KERNEL, cpu_to_node(cpu)); > > > > Ditto. > > Sure. > > > > > > + if (!acomp_ctx->reqs) > > > goto req_fail; > > > + > > > + for (i = 0; i < nr_reqs; ++i) { > > > + acomp_ctx->reqs[i] = acomp_request_alloc(acomp_ctx->acomp); > > > + if (!acomp_ctx->reqs[i]) { > > > + pr_err("could not alloc crypto acomp_request reqs[%d] > > %s\n", > > > + i, pool->tfm_name); > > > + for (j = 0; j < i; ++j) > > > + acomp_request_free(acomp_ctx->reqs[j]); > > > + kfree(acomp_ctx->reqs); > > > + ret = -ENOMEM; > > > + goto req_fail; > > > + } > > > } > > > - acomp_ctx->req = req; > > > > > > + /* > > > + * The crypto_wait is used only in fully synchronous, i.e., with scomp > > > + * or non-poll mode of acomp, hence there is only one "wait" per > > > + * acomp_ctx, with callback set to reqs[0], under the assumption that > > > + * there is at least 1 request per acomp_ctx. > > > + */ > > > crypto_init_wait(&acomp_ctx->wait); > > > /* > > > * if the backend of acomp is async zip, crypto_req_done() will wakeup > > > * crypto_wait_req(); if the backend of acomp is scomp, the callback > > > * won't be called, crypto_wait_req() will return without blocking. > > > */ > > > - acomp_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG, > > > + acomp_request_set_callback(acomp_ctx->reqs[0], > > CRYPTO_TFM_REQ_MAY_BACKLOG, > > > crypto_req_done, &acomp_ctx->wait); > > > > > > + acomp_ctx->nr_reqs = nr_reqs; > > > return 0; > > > > > > req_fail: > > > + for (i = 0; i < nr_reqs; ++i) > > > + kfree(acomp_ctx->buffers[i]); > > > + kfree(acomp_ctx->buffers); > > > > The cleanup code is all over the place. Sometimes it's done in the > > loops allocating the memory and sometimes here. It's a bit hard to > > follow. Please have all the cleanups here. You can just initialize the > > arrays to 0s, and then if the array is not-NULL you can free any > > non-NULL elements (kfree() will handle NULLs gracefully). > > Sure, if performance of kzalloc_node() is an acceptable trade-off for the > cleanup code simplification. > > > > > There may be even potential for code reuse with zswap_cpu_comp_dead(). > > I assume the reuse will be through copy-and-paste the same lines of code as > against a common procedure being called by zswap_cpu_comp_prepare() > and zswap_cpu_comp_dead()? Well, I meant we can possibly introduce the helper that will be used by both zswap_cpu_comp_prepare() and zswap_cpu_comp_dead() (for example see __mem_cgroup_free() called from both the freeing path and the allocation path to do cleanup). I didn't look too closely into it though, maybe it's best to keep them separate, depending on how the code ends up looking like.