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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 9D88CC433F5 for ; Tue, 12 Apr 2022 02:16:09 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S243630AbiDLCSX (ORCPT ); Mon, 11 Apr 2022 22:18:23 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:47918 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230271AbiDLCSU (ORCPT ); Mon, 11 Apr 2022 22:18:20 -0400 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 0A8092A24A; Mon, 11 Apr 2022 19:16:03 -0700 (PDT) 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 dfw.source.kernel.org (Postfix) with ESMTPS id 4D1996168C; 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 Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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". 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 bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 3C4BBC433EF for ; Tue, 12 Apr 2022 02:17:40 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:Mime-Version:References:In-Reply-To: Message-Id:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=eEsBIvuNPxSEqgv62EoLHx4MPmnEM1YpGVWpO9QXwDg=; b=Ct7MoNPAjPU5LV Rt118dY1UI7ke7RTCxu+8UYjUIoxsFzR1EbTimY5dbKp7H8oegdhwJWh62QpBy9K1h3c57m5RjhdG cZklRXPuvPWjC9tmF8qLz/31WEOWG105RdIHtsTpbXvrlcppekD1Ihsy7dpo0o0znbGlKy9YRxx5i H6/Wxc9ILRPW9xGE10BVvDid9lpkOYtIj02dMvbVl+eqpMVBTMpSaGfBnmYA1uKvGNbMqfM6zDSci VjW48+JxLbDXSz9exTb/f9O3bujqH8BTro/M7aB6/JacCSiiwRtDXzljTgwdgKiCTOujsYGrtvt6l GKqJ5YzXYjvbVrmpcz0g==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1ne64m-00BErt-9Q; Tue, 12 Apr 2022 02:16:16 +0000 Received: from ams.source.kernel.org ([2604:1380:4601:e00::1]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1ne64j-00BEpO-8t for linux-arm-kernel@lists.infradead.org; Tue, 12 Apr 2022 02:16:14 +0000 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 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220411_191613_637871_A6260013 X-CRM114-Status: GOOD ( 20.47 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org 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". _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel