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 X-Spam-Level: X-Spam-Status: No, score=-2.3 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_2 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5BD8FC18E5B for ; Tue, 10 Mar 2020 15:55:17 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 2D42F21D7E for ; Tue, 10 Mar 2020 15:55:17 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 2D42F21D7E Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=Huawei.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id D23BF6B0005; Tue, 10 Mar 2020 11:55:16 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id CD46F6B0006; Tue, 10 Mar 2020 11:55:16 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id C10826B0007; Tue, 10 Mar 2020 11:55:16 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0065.hostedemail.com [216.40.44.65]) by kanga.kvack.org (Postfix) with ESMTP id A5D4A6B0005 for ; Tue, 10 Mar 2020 11:55:16 -0400 (EDT) Received: from smtpin16.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id 6FF3C87F3 for ; Tue, 10 Mar 2020 15:55:16 +0000 (UTC) X-FDA: 76579901832.16.clock81_682a94c61860e X-HE-Tag: clock81_682a94c61860e X-Filterd-Recvd-Size: 5849 Received: from huawei.com (lhrrgout.huawei.com [185.176.76.210]) by imf25.hostedemail.com (Postfix) with ESMTP for ; Tue, 10 Mar 2020 15:55:15 +0000 (UTC) Received: from lhreml706-cah.china.huawei.com (unknown [172.18.7.108]) by Forcepoint Email with ESMTP id 8A3E3C6E21A78A430E07; Tue, 10 Mar 2020 15:55:13 +0000 (GMT) Received: from lhreml710-chm.china.huawei.com (10.201.108.61) by lhreml706-cah.china.huawei.com (10.201.108.47) with Microsoft SMTP Server (TLS) id 14.3.408.0; Tue, 10 Mar 2020 15:55:13 +0000 Received: from localhost (10.202.226.57) by lhreml710-chm.china.huawei.com (10.201.108.61) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1713.5; Tue, 10 Mar 2020 15:55:12 +0000 Date: Tue, 10 Mar 2020 15:55:10 +0000 From: Jonathan Cameron To: SeongJae Park CC: , SeongJae Park , , , , , , , , , , , , , , , , , , , , , , , , , , , , , Subject: Re: [PATCH v6 02/14] mm/damon: Implement region based sampling Message-ID: <20200310155510.000025d2@Huawei.com> In-Reply-To: <20200310115233.23246-1-sjpark@amazon.com> References: <20200310085721.00000a0f@Huawei.com> <20200310115233.23246-1-sjpark@amazon.com> Organization: Huawei Technologies Research and Development (UK) Ltd. X-Mailer: Claws Mail 3.17.4 (GTK+ 2.24.32; i686-w64-mingw32) MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.202.226.57] X-ClientProxiedBy: lhreml715-chm.china.huawei.com (10.201.108.66) To lhreml710-chm.china.huawei.com (10.201.108.61) X-CFilter-Loop: Reflected 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: On Tue, 10 Mar 2020 12:52:33 +0100 SeongJae Park wrote: > Added replies to your every comment in line below. I agree to your whole > opinions, will apply those in next spin! :) > One additional question inline that came to mind. Using a single statistic to monitor huge page and normal page hits is going to give us problems I think. Perhaps I'm missing something? > > > +/* > > > + * Check whether the given region has accessed since the last check > > > > Should also make clear that this sets us up for the next access check at > > a different memory address it the region. > > > > Given the lack of connection between activities perhaps just split this into > > two functions that are always called next to each other. > > Will make the description more clearer as suggested. > > Also, I found that I'm not clearing *pte and *pmd before going 'mkold', thanks > to this comment. Will fix it, either. > > > > > > + * > > > + * mm 'mm_struct' for the given virtual address space > > > + * r the region to be checked > > > + */ > > > +static void kdamond_check_access(struct damon_ctx *ctx, > > > + struct mm_struct *mm, struct damon_region *r) > > > +{ > > > + pte_t *pte = NULL; > > > + pmd_t *pmd = NULL; > > > + spinlock_t *ptl; > > > + > > > + if (follow_pte_pmd(mm, r->sampling_addr, NULL, &pte, &pmd, &ptl)) > > > + goto mkold; > > > + > > > + /* Read the page table access bit of the page */ > > > + if (pte && pte_young(*pte)) > > > + r->nr_accesses++; > > > +#ifdef CONFIG_TRANSPARENT_HUGEPAGE > > > > Is it worth having this protection? Seems likely to have only a very small > > influence on performance and makes it a little harder to reason about the code. > > It was necessary for addressing 'implicit declaration' problem of 'pmd_young()' > and 'pmd_mkold()' for build of DAMON on several architectures including User > Mode Linux. > > Will modularize the code for better readability. > > > > > > + else if (pmd && pmd_young(*pmd)) > > > + r->nr_accesses++; So we increment a region count by one if we have an access in a huge page, or in a normal page. If we get a region that has a mixture of the two, this seems likely to give a bad approximation. Assume the region is accessed 'evenly' but each " 4k page" is only hit 10% of the time (where a hit is in one check period) If our address in a page, then we'll hit 10% of the time, but if it is in a 2M huge page then we'll hit a much higher percentage of the time. 1 - (0.9^512) ~= 1 Should we look to somehow account for this? > > > +#endif /* CONFIG_TRANSPARENT_HUGEPAGE */ > > > + > > > + spin_unlock(ptl); > > > + > > > +mkold: > > > + /* mkold next target */ > > > + r->sampling_addr = damon_rand(ctx, r->vm_start, r->vm_end); > > > + > > > + if (follow_pte_pmd(mm, r->sampling_addr, NULL, &pte, &pmd, &ptl)) > > > + return; > > > + > > > + if (pte) { > > > + if (pte_young(*pte)) { > > > + clear_page_idle(pte_page(*pte)); > > > + set_page_young(pte_page(*pte)); > > > + } > > > + *pte = pte_mkold(*pte); > > > + } > > > +#ifdef CONFIG_TRANSPARENT_HUGEPAGE > > > + else if (pmd) { > > > + if (pmd_young(*pmd)) { > > > + clear_page_idle(pmd_page(*pmd)); > > > + set_page_young(pmd_page(*pmd)); > > > + } > > > + *pmd = pmd_mkold(*pmd); > > > + } > > > +#endif > > > + > > > + spin_unlock(ptl); > > > +} > > > +