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 65151C433F5 for ; Tue, 12 Apr 2022 02:16:07 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id E2C8B6B0071; Mon, 11 Apr 2022 22:16:06 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id DB3A86B0073; Mon, 11 Apr 2022 22:16:06 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id C05B76B0074; Mon, 11 Apr 2022 22:16:06 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (relay.a.hostedemail.com [64.99.140.24]) by kanga.kvack.org (Postfix) with ESMTP id AA36C6B0071 for ; Mon, 11 Apr 2022 22:16:06 -0400 (EDT) Received: from smtpin05.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id 64BD561E5A for ; Tue, 12 Apr 2022 02:16:06 +0000 (UTC) X-FDA: 79346611932.05.8BB8644 Received: from ams.source.kernel.org (ams.source.kernel.org [145.40.68.75]) by imf08.hostedemail.com (Postfix) with ESMTP id B6C75160005 for ; Tue, 12 Apr 2022 02:16:05 +0000 (UTC) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id CF659B81895; Tue, 12 Apr 2022 02:16:03 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 7E5F1C385A4; Tue, 12 Apr 2022 02:16:01 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=linux-foundation.org; s=korg; t=1649729762; bh=tni5PvWyZxUvzvzTtCOxpTSc4Z5BiuG+j4AAUMjSjLw=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=ypvt4LtLI/wxKQQOhAHailuFJouqrOwDHQdOxKPFUl0+7Q9WuFLM3+RQ0vokvusr7 G1Nozi6lKTTm+nfFda9SoF2eedu2cD736RrVrFD96DVmyx49cXTpjMjJWqoYggHVPg NTal9bWOGMDNrGM/13809C+0fKJ4TqG1VRF/+dA4= Date: Mon, 11 Apr 2022 19:15:59 -0700 From: Andrew Morton To: Yu Zhao Cc: Stephen Rothwell , linux-mm@kvack.org, Andi Kleen , Aneesh Kumar , Barry Song <21cnbao@gmail.com>, Catalin Marinas , Dave Hansen , Hillf Danton , Jens Axboe , Jesse Barnes , Johannes Weiner , Jonathan Corbet , Linus Torvalds , Matthew Wilcox , Mel Gorman , Michael Larabel , Michal Hocko , Mike Rapoport , Rik van Riel , Vlastimil Babka , Will Deacon , Ying Huang , linux-arm-kernel@lists.infradead.org, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, page-reclaim@google.com, x86@kernel.org Subject: Re: [PATCH v10 00/14] Multi-Gen LRU Framework Message-Id: <20220411191559.a844c7140faeba2e68d842b7@linux-foundation.org> In-Reply-To: <20220407031525.2368067-1-yuzhao@google.com> References: <20220407031525.2368067-1-yuzhao@google.com> X-Mailer: Sylpheed 3.7.0 (GTK+ 2.24.33; x86_64-pc-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-Rspamd-Server: rspam09 X-Rspam-User: X-Stat-Signature: sj316597fh94e5gxgetjhbtik4hcz7ab Authentication-Results: imf08.hostedemail.com; dkim=pass header.d=linux-foundation.org header.s=korg header.b=ypvt4LtL; dmarc=none; spf=pass (imf08.hostedemail.com: domain of akpm@linux-foundation.org designates 145.40.68.75 as permitted sender) smtp.mailfrom=akpm@linux-foundation.org X-Rspamd-Queue-Id: B6C75160005 X-HE-Tag: 1649729765-47028 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: It's pretty clear from the results and from the test coverage to date that Linux wants this. I do think additional review is needed. For the usual code quality reasons, but also to help others get up to speed with the proposed changes and to ensure that we have something which is well maintainable going forward. The code at present is unnecessarily difficult to review and that review will be less effective than it might be, because of its lack of commentary. I'll merge the v10 series into -mm and -next for additional runtime exposure, but I'll be asking for a broad update. The data structures appear to be adequately documented (this is rare) but the code itself is lacking. Please go through each and every new function and ask "is this function so self-evident that it can be presented to a newcomer with no explanatory comments at all". If "yes" then check again. If still "yes" then fine. If "no" then let's craft comments which tell the reader things which are not evident from the code itself. Things which are helpful to that reader. Design concepts, side-effects, preconditions, unobvious traps, etc. Please ensure that the current group of mglru developers review these comment additions as closely as they review code changes. Alas, it's late in the process to be adding these things. But it can be made better. I'll make some high-level comments on the patches themselves now, but I see little point in attempting detailed review when a better-explained version is hopefully forthcoming. A few gratuitous notebook entries: - lru_gen_struct.timestamps works in jiffies? Why? jiffies can vary based on platform and config - why add inter-platform and inter-Kconfig variability like this? Time is measured in seconds! And the amount of work which can be performed in one second varies by, guess, 100x on machines which we care about. This also has potential to introduce tremendous inter-platform variability in the behaviour of this feature. - did lru_gen_use_mm() really need to test nodes_full()? Is that likely enough to be a net benefit? - seq_is_valid() sounds like it belongs to the seq_file() subsystem - does get_nr_evictable() need to return and use signed types (long)? It sums the contents of lru_gen_struct.nr_pages[][][], which is ulong, so I think "no".