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 9FADAE7D0A6 for ; Thu, 21 Sep 2023 19:36:33 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 0791E6B01F6; Thu, 21 Sep 2023 15:36:33 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 0290E6B0254; Thu, 21 Sep 2023 15:36:32 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id E32846B0257; Thu, 21 Sep 2023 15:36:32 -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 D41306B01F6 for ; Thu, 21 Sep 2023 15:36:32 -0400 (EDT) Received: from smtpin14.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id 9DC3DB3F2B for ; Thu, 21 Sep 2023 19:36:32 +0000 (UTC) X-FDA: 81261611424.14.88AC7AC Received: from ams.source.kernel.org (ams.source.kernel.org [145.40.68.75]) by imf18.hostedemail.com (Postfix) with ESMTP id B38501C0023 for ; Thu, 21 Sep 2023 19:36:30 +0000 (UTC) Authentication-Results: imf18.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=Dmdu9cca; spf=pass (imf18.hostedemail.com: domain of sj@kernel.org designates 145.40.68.75 as permitted sender) smtp.mailfrom=sj@kernel.org; dmarc=pass (policy=none) header.from=kernel.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1695324991; 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-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=6S0NtIi7ma7uXs6jR+Ju2UwpEwvKBXzhkrd04nPgoiw=; b=lBa0Gvy4GpouUBTY/IcicH2SH/YSE3OdwrLBgDOeFa8zA04vshviMCChIA3hrWNMEgfrjz /MbZGp9rxsLitx5Ec5FWKwa4x4HNfn9sbsR6n85yGSTsn32ui5RZG58h4BkL//0jFAe2LB bkCshqYxAJtSz5sMSxS7HyMAWiuGVLQ= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1695324991; a=rsa-sha256; cv=none; b=2A3m/tn9Ns91rp7qKSfKJj0lNeJY3inyuPBWsYdIp3yRCn3gs1+r5R2Qlyagd7VAhaIZm9 3uq1I34GRDQPk+R7dwOexUgsQfHYNoXxdTbXNcPRS5+l2kEYuW7wiUPEca20RKzhn4J0lP Q+cFIipT7P6eWeczvSFTgil+UWHSMXQ= ARC-Authentication-Results: i=1; imf18.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=Dmdu9cca; spf=pass (imf18.hostedemail.com: domain of sj@kernel.org designates 145.40.68.75 as permitted sender) smtp.mailfrom=sj@kernel.org; dmarc=pass (policy=none) header.from=kernel.org Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id F1D21B82130; Thu, 21 Sep 2023 19:36:28 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id F0242C433C8; Thu, 21 Sep 2023 19:36:26 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1695324987; bh=yWgtFNgkqgWjEXgwCB6SqgQ53emC6ZvfBAx69yUw5yQ=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=Dmdu9ccaG6RwZLCVtqnwk4lho3W9T0um4xSuX6SBZFvkcezG0GoeVUuDvwdVYyX1L jpPrS/Ng5UuimZZ4Q8Fjrl91XBGaUaJJSTES1g6xikPjOEmKa7VALkPLGOtMj156Nh LJunk2N4sHsrbeUQADAKcpe2iL1QypPJ39BTJCHTAnCp5wbJpuVCf8tivsEiNK5Ysv A3edupwAMRy33tMf0/y7DE74FIFZCLg2QX3qkO02/R/URkT57uLE4wd1S3G7macte3 rwtDRwTYyqk12u3zUrXSmd/fcdFcOjWm3x8gFufQxlhUIheFMzUzFidLx+zHJ7Gj6I 0q2BFIEB+PTFg== From: SeongJae Park To: Huan Yang Cc: SeongJae Park , Andrew Morton , linux-kernel@vger.kernel.org (open list), damon@lists.linux.dev (open list:DATA ACCESS MONITOR), linux-mm@kvack.org (open list:DATA ACCESS MONITOR), opensource.kernel@vivo.com Subject: Re: [RFC 0/2] Damos filter cleanup code and implement workingset Date: Thu, 21 Sep 2023 19:36:25 +0000 Message-Id: <20230921193625.99617-1-sj@kernel.org> X-Mailer: git-send-email 2.25.1 In-Reply-To: <20230919095237.26613-1-link@vivo.com> References: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Rspamd-Queue-Id: B38501C0023 X-Rspam-User: X-Stat-Signature: 3ht8pk8yg8rh7ihsuaup1feb6rocswqw X-Rspamd-Server: rspam03 X-HE-Tag: 1695324990-637271 X-HE-Meta: U2FsdGVkX1+tfAGWuWD7hWMOl59mj5NBCWfAmxskE+8nZjjU940voNvn1Nq0pT6DlWPSzs+Rk2RtdYzcN6Xa0QXjjpSyb84UWS6s0WzSYFXi7WODX1z44qMeoQukZLn7oAtyV92OUWusC2vJ6KgfWlPlpGld30/WWTzkV7r2KJabJtUCTCKjNeHSsuexsohg0aGOFbWqH2K/l1QEaBzuVS3v1bm2++yjw5BqfQA+su9soM8BvxslnYbMiBKorvGc6zzwFVqWxrRtYiEk1wG43iHnNDMkub0BWGDhv+eOPQrevBKTDzY1Et4QJ+IrskUJWVfYNGKbNphlyWApPyjFx7dPVoy/A9Ufn8ms6BRuq7woH7bIno8S+BXSipMq1JbGcPYMMidNoTjySc1KVYAW4+6lThNP6TRX3y5YUXUSp8aZXTy73L9UvgBgHV5s8SmcEz+RVvJvJzckNml7j7GGUG14oPmevCXlVbP8JqhmfKNtIxwOPlxNjUENsE6bfiDChxHnAS3wDd+9cvFBlzIBjIBFf43/nPj3ZQpqlxfGx2e/h5CzU8SrQpD5gN/nbepG1AuD03t1CwToUHGPOQyn2Uit34W8WJb4jZwyeskyQPk/6onYSuJfd7L4jURUJ0W6AHCD9IZqZPaN7hidfajRcGsy3TeLT0ZzqSYmdRhPK7fjnV+sbKSMMW5ZYkEplP0GPJWU/mk22MCrdLQ79VcadVYvl+HbSwahXf8q0CMlQDLDwuwtltkUA7RT+UwMl6k25F19XTRYBRb/kbICALfY4Eo0UpRLWGYn7GsGa2rJpNIOTjKFljk4L8cH9lcZE7cG/ZSUqzJUh1vwaACroYuw6W0ucK4vP5Szzo1wm6YPsZ/ZmfMrLBLahywd58r00ywOZGXukq5IqSn6/RzIkbB0D48tjrn1VMOb/kjpSqgDvy6nFH+jdLYvrnavTtxd1bws9j0qvlk2CFRaZCBuICq Mx4WTGzI /alUq9uU0Tf4pFEDIwnZ5V9bDKtGSLxy0DPzKeQOLqz0/iYY4ObubOyL6TB0tqGp9zGFtqWdmUBw3SNxHh6escfBqf7mipMO5oRJS2UyWU7Kjt6YIb5uQLfhjvsZSLf0mdvD+O8N/YG+dv1EXRkxJD2iU9d0hA2+w3rMfFDAUnFyoeSpHXtG3UIDRrCkz7tjxtMx/Zaz1YYSmrxR8UmZA/N9Xuw0sGFE7mBWI9VekoPoWPlmfsds3QDLQIaiFCsOROcaJ4QSUXsecmgKTzmBUhBxH2AfkDq2gkOd9ftZ3hdG2EK0+UHSPvUz5iRJDweUett6UPDXRk2ZQgY9vKAIP5D4v8QT2LDR/vtVmuP+/xDGIcpqxyN3lE0HjJj7iZrSRmbFmvuO6+2MPGz9nZW506sKXjA== 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: Hi Huan, First of all, thank you for this patch. I have some high level comments and questions as below. On Tue, 19 Sep 2023 17:52:33 +0800 Huan Yang wrote: > Now damos support filter contains two type. > The first is scheme filter which will invoke each scheme apply, > second is scheme action filter, which will filter out unwanted action. IMHO, that's not clear definition of the types. Conceptually, all the filters are same. Nonetheless, they are having different behaviors because they are handled in different layer depending on which layer is more efficient to handle those[1]. I agree this is complicated and a bit verbose explanation, but I don't feel your scheme vs action definition is making it easier to understand. > > But this implement is scattered in different implementations Indeed the implementation is scattered in core layer and the ops layer depending on the filter types. But I think this is needed for efficient handling of those. > and hard to reuse or extend. >From your first patch, which extending the filter, the essential change for the extension is adding another enum to 'enum damos_filter_type' (1 line) and addition of switch case in '__damos_pa_filter_out()' (3 lines). I don't think it looks too complicated. If you're saying it was hard to understand which part really need to be modifed, I think that makes sense. But in the case, we might need more comments rather than structural change. > > This patchset clean up those filter code, move into filter.c and add header > to expose filter func.(patch 2) Again, I agree more code cleanup is needed. But I'm not sure if this is the right way. Especially, I'm unsure if it really need to have a dedicated file. To my humble eyes, it doesn't look like making code clearly easier to read compared to the current version. This is probably because I don't feel your scheme vs action definition is clear to understand. Neither it is deduplicating code. The patch adds lines more that deleted ones, according to its diffstat. For the reason, I don't see clear benefit of this change. I might be biased, having NIH (Not Invented Here) sindrome, or missing something. Please let me know if so. > And add a new filter "workingset" to protect workingset page. Can you explain expected use cases of this new filter type in more detail? Specifically, for what scheme action and under what situation this new filter type will be needed? If you have some test data for the use case and could share it, it would be very helpful for me to understand why it is needed. > > Do we need this and cleanup it? I think I cannot answer for now, and your further clarification and patient explanation could be helpful. [1] https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git/tree/Documentation/mm/damon/design.rst?h=mm-everything-2023-09-20-19-38#n400 Thanks, SJ > > Huan Yang (2): > mm/damos/filter: Add workingset page filter > mm/damos/filter: Damos filter cleanup > > include/linux/damon.h | 62 +----------------- > mm/damon/Makefile | 2 +- > mm/damon/core-test.h | 7 ++ > mm/damon/core.c | 93 ++++----------------------- > mm/damon/filter.c | 135 +++++++++++++++++++++++++++++++++++++++ > mm/damon/filter.h | 119 ++++++++++++++++++++++++++++++++++ > mm/damon/paddr.c | 29 +++------ > mm/damon/reclaim.c | 48 +++++++++++--- > mm/damon/sysfs-schemes.c | 1 + > 9 files changed, 325 insertions(+), 171 deletions(-) > create mode 100644 mm/damon/filter.c > create mode 100644 mm/damon/filter.h > > -- > 2.34.1 > >