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 67944C27C75 for ; Tue, 11 Jun 2024 15:21:28 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id E75686B00A4; Tue, 11 Jun 2024 11:21:27 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id E24706B00A6; Tue, 11 Jun 2024 11:21:27 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id CECDC6B00A7; Tue, 11 Jun 2024 11:21:27 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id AE8FE6B00A4 for ; Tue, 11 Jun 2024 11:21:27 -0400 (EDT) Received: from smtpin10.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id 4A90A1A0B5F for ; Tue, 11 Jun 2024 15:21:27 +0000 (UTC) X-FDA: 82218971814.10.4AF32AB Received: from mail-yb1-f178.google.com (mail-yb1-f178.google.com [209.85.219.178]) by imf14.hostedemail.com (Postfix) with ESMTP id 893CF100016 for ; Tue, 11 Jun 2024 15:21:25 +0000 (UTC) Authentication-Results: imf14.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=YhvPuc85; spf=pass (imf14.hostedemail.com: domain of flintglass@gmail.com designates 209.85.219.178 as permitted sender) smtp.mailfrom=flintglass@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1718119285; a=rsa-sha256; cv=none; b=T9UDlgSLzp/+qmXzthF1xBkpKdN4PLgHhg67ACQAYFn8LaF6aUp1n1vCnBwbLfXwSTwPJ2 Ws5e9/KfbDGbD0EmFSS36g2Bjr0Cas2lMxaZ3BMKY48Co2MeK4ibIHJL30IqYfoQLBsbnS U6cmRHtS3Q/FUMCqtMbGpX4e7sK+RPk= ARC-Authentication-Results: i=1; imf14.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=YhvPuc85; spf=pass (imf14.hostedemail.com: domain of flintglass@gmail.com designates 209.85.219.178 as permitted sender) smtp.mailfrom=flintglass@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1718119285; 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=J1kYTsVFHvV3hNTS3A7aiLsXfJGmt9YdVx0hwX8mjlg=; b=39NK9s/6h4iEYwdrTc33SuZtntBsJBZVilcpmt04V5rQqpBHl40r2vt1+oujcnukqqdbax kzr99XFsQ7nuuwINOc4szubKEnmVeogEUtjgpAhWT7Byp/trp0NQFOt7SZpr18AwGL36tw dXr18BqkY9MCXXN+ym1BbxQOtevmKdk= Received: by mail-yb1-f178.google.com with SMTP id 3f1490d57ef6-dfa48f505a3so1346849276.1 for ; Tue, 11 Jun 2024 08:21:25 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1718119284; x=1718724084; 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=J1kYTsVFHvV3hNTS3A7aiLsXfJGmt9YdVx0hwX8mjlg=; b=YhvPuc85DvkVTozhGAmVWlgI3NkqiWScxTtwLGV7KG0cLcXTYq4nFg5fbSTa6l+fvx lE31l4wWolSXGZHpOSxUwKdofDIegjGem1EldUpvTFQw+gDKgx2g18JBOUMOdBrzhr41 RyRs7TVlQeL58MMcDfncndc8ugQC50GmKPOZxUBlnnNURb1r1Vh3puAOMvDQ4BABWcP0 ND+8zvfU7G6nKcJoIEQ25xeifPPk0c0AqgNJ05CMD2UUIyOfOhX36g9wweojJNgjNeVN D2ycrJXUH+Ee4xREMAe71Sv3wr+dpP+xjOevB5/4vNTr9CYxUVat29PMd1seRhMxfMX7 Fowg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1718119284; x=1718724084; 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=J1kYTsVFHvV3hNTS3A7aiLsXfJGmt9YdVx0hwX8mjlg=; b=tsGHVzwZeZv3002fTQDSFuveWu1Y/ZIX0PrSzrILkhGo1pHorofZokL7lI+91fAE+G k+YcAccSVIJf4D5Q8EoftdP1lx95EPMjVu7WG3zEkCQReOZO9PjLv7L4Yblo/ULll6yE l3FAfkjVKhdM7gvBmIkdzRSKaF8GgP3yYSKDL2sbHw3hovpXBiaT8j8Hzwrzlp8CavPU M/C4UwVm1eqo6pshrLeUuZT9+mvdT1xrqlzjmHKLLrSZTbUZepsbLvD8ds/ufxMtu3Xz h67P6yZuiwbvb4/JsIWSRU72Rd2uzClXU5isidk6oxyZbqFbKwLDRbIgTtTYma6974nu bX0A== X-Forwarded-Encrypted: i=1; AJvYcCUJbOCZf08muFnnUjlU5Koc+SQM2SqlWQb1WDbAyNCEy6Ams0sRc7q3Y692UoHVKRYqNIQsOAMA71hsnghSX5Csfy8= X-Gm-Message-State: AOJu0Yzf7IsqZLXuA0qv8REiXaZ6U4e2NA3/AaxLSRtLroId2VnLZCW/ yJASLva9/cRB0r4BQfGAN4yaaJTUgrr7hhUO1muT6hiVcRpt9pdoj4Lkj9TBT027WyGudsgJ16k Mba6AjEVdFejuhKk9gVz0s2oQY2Q= X-Google-Smtp-Source: AGHT+IFqOX8dyvxGqZZ4ezrnpN/DGORkRIJkUVTMjkZlrD7I9/dvJuLZhiP27fCx9HyGLrYhprvuvCKtFYnXNJoSX8I= X-Received: by 2002:a25:660d:0:b0:dfb:1460:b5b1 with SMTP id 3f1490d57ef6-dfb1460b9e3mr7565693276.1.1718119284571; Tue, 11 Jun 2024 08:21:24 -0700 (PDT) MIME-Version: 1.0 References: <20240608155316.451600-1-flintglass@gmail.com> <20240608155316.451600-3-flintglass@gmail.com> In-Reply-To: From: Takero Funaki Date: Wed, 12 Jun 2024 00:21:13 +0900 Message-ID: Subject: Re: [PATCH v1 2/3] mm: zswap: fix global shrinker error handling logic To: Yosry Ahmed Cc: Johannes Weiner , Nhat Pham , Chengming Zhou , Jonathan Corbet , Andrew Morton , Domenico Cerasuolo , linux-mm@kvack.org, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org Content-Type: text/plain; charset="UTF-8" X-Stat-Signature: ny8qnwbp15aqkuggb77b9wbeoynwr5ut X-Rspamd-Queue-Id: 893CF100016 X-Rspam-User: X-Rspamd-Server: rspam06 X-HE-Tag: 1718119285-258308 X-HE-Meta: U2FsdGVkX1+TOS1j77WjZ4X4uuQQPBf98EVyKWH5Pb7yxcfp2cafWwURx/hBiGz4hoS1Uf5KorF6cFgMTgJUcoGQx9DQeQjpRDhbn+8l+9sVPBbUbVUlhpWGRYTIOBJFHb65OmknVWWXxuHC3TH+Ll3uTR0Qv1zdw3z7qa4M8AqtLRrXfDwpkCWC3scHnDgHn+H8VvfNZSmH5z6PltQd/RtLW8lm2o9U15Qag3m7QtwWNwq/qH8QcWou4bz/gtglv/ZX6uqcLfE3PhOWMPL9EK+Pacgj88LpbHW8H8XZa4imvw6dTI9idjxspIb+EO1HD6BsHa6V7s7mQPkRkDWwf1cYpPxrR2B3xazGVZCis9dqfMzwVRKc9nvwHWRcxlvg+k1x2SWmtI6j95+xJSTimfbqzdlpYtFxQrDtvkzAv6Y5bpdqhGL4rOGdcxnuP+tSJT/9vBijuKDdi6dBARqsp2O6pYqDVCGQvl+52xQ9oJf9n+5oZs1lYHz3ZS1o0C2iRZcqdugoJonffTl7wAlL1TMZ0pMp9DZcv1HnpM4/Sjuu9XXMLZvI1FdP7Na6mzMAHY23oPYWA71JffLHz3FnAzkOwOztrPVIm/X/Ls7owQonb2thSA+T28x/uyD+KhAnIwodRxUkzSSLTbpR1Q4aTYtNi2tkV6g+0LNADoYekmqseAGVlymVRrJcSjEW6ZlKsoEdsC5A7W9zkvfwfngrIE2CZESbH+nJNluTJ8F3sBEU/5s93PbJfNVhK9zlouI3wMEEGjseMmdOA75Ds/8ujJwqsI3iThxx8elP6fFnKKwZLp1g4PROtV5MGj8wiwRSj1qxK26SPgWKvjbosGFfARn1U/u88QcavJ8pGTgvJ/k02/5FODzOhvmsvdCejwxbw5w31lQVuqqXVxnpguXswZxqhKNFQ18AXmlZ/GXainueBZrdscftG516iDH5YmpIzvEdN9aq7Vp9b+EbUOB 4WbVxt0v CA4mseiWL4RCpUCltoR6BB+FtwEcFA5dnKeVk56w/USzHrs0+MUoxRyuHFCGxf1/adIro/nWIYzuRvy2LlWjeL25r2bqL0TwHMoB0FLn4KUOFsvBQZlE95X/CC8qTjVUW3wwYT6cjXaRjnsg/xNsXOTU/RAXjNdNqsGVshtxLa42Lr18O9l86oAAamodkTQLvBxHjNU7yZd2A2x0Uelz8tJglubAhlhSopushlyvQ8KzO8qT51orRbn6PLyBfyT/XnhT/ 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: Thanks a lot for your comments. On 2024/06/11 5:27, Yosry Ahmed wrote: >> unsigned long nr_to_walk = 1; >> >> + if (!list_lru_count_one(&zswap_list_lru, nid, memcg)) >> + continue; >> + ++stored; >> shrunk += list_lru_walk_one(&zswap_list_lru, nid, memcg, >> &shrink_memcg_cb, NULL, &nr_to_walk); >> } >> + >> + if (!stored) >> + return -ENOENT; >> + > > Can't we just check nr_to_walk here and return -ENOENT if it remains as 1? > > Something like: > > if (nr_to_walk) > return -ENOENT; > if (!shrunk) > return -EAGAIN; > return 0; > ah, the counting step can be removed. I will change it in v2. >> return shrunk ? 0 : -EAGAIN; >> } >> >> @@ -1418,12 +1425,18 @@ static void shrink_worker(struct work_struct *w) >> { >> struct mem_cgroup *memcg = NULL; >> struct mem_cgroup *next_memcg; >> - int ret, failures = 0; >> + int ret, failures = 0, progress; >> unsigned long thr; >> >> /* Reclaim down to the accept threshold */ >> thr = zswap_accept_thr_pages(); >> >> + /* >> + * We might start from the last memcg. >> + * That is not a failure. >> + */ >> + progress = 1; >> + >> /* global reclaim will select cgroup in a round-robin fashion. >> * >> * We save iteration cursor memcg into zswap_next_shrink, >> @@ -1461,9 +1474,12 @@ static void shrink_worker(struct work_struct *w) >> */ >> if (!memcg) { >> spin_unlock(&zswap_shrink_lock); >> - if (++failures == MAX_RECLAIM_RETRIES) >> + >> + /* tree walk completed but no progress */ >> + if (!progress && ++failures == MAX_RECLAIM_RETRIES) >> break; > > It seems like we may keep iterating the entire hierarchy a lot of > times as long as we are making any type of progress. This doesn't seem > right. > Since shrink_worker evicts only one page per tree walk when there is only one memcg using zswap, I believe this is the intended behavior. Even if we choose to break the loop more aggressively, it would only be postponing the problem because pool_limit_hit will trigger the worker again. I agree the existing approach is inefficient. It might be better to change the 1 page in a round-robin strategy. >> >> + progress = 0; >> goto resched; >> } >> >> @@ -1493,10 +1509,15 @@ static void shrink_worker(struct work_struct *w) >> /* drop the extra reference */ >> mem_cgroup_put(memcg); >> >> - if (ret == -EINVAL) >> - break; >> + /* not a writeback candidate memcg */ >> + if (ret == -EINVAL || ret == -ENOENT) >> + continue; >> + > > We should probably return -ENOENT for memcg with writeback disabled as well. > >> if (ret && ++failures == MAX_RECLAIM_RETRIES) >> break; >> + >> + ++progress; >> + /* reschedule as we performed some IO */ >> resched: >> cond_resched(); >> } while (zswap_total_pages() > thr); >> -- >> 2.43.0 >>