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 1669ED3DEA6 for ; Fri, 18 Oct 2024 18:33:18 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 836F56B00AC; Fri, 18 Oct 2024 14:33:17 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 7C0A86B00AD; Fri, 18 Oct 2024 14:33:17 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 6611B6B00AE; Fri, 18 Oct 2024 14:33:17 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id 4732A6B00AC for ; Fri, 18 Oct 2024 14:33:17 -0400 (EDT) Received: from smtpin30.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id 2444E8094D for ; Fri, 18 Oct 2024 18:33:06 +0000 (UTC) X-FDA: 82687570056.30.A4E395F Received: from nyc.source.kernel.org (nyc.source.kernel.org [147.75.193.91]) by imf26.hostedemail.com (Postfix) with ESMTP id 3EDE3140008 for ; Fri, 18 Oct 2024 18:33:06 +0000 (UTC) Authentication-Results: imf26.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=McAj2UUm; dmarc=pass (policy=quarantine) header.from=kernel.org; spf=pass (imf26.hostedemail.com: domain of sj@kernel.org designates 147.75.193.91 as permitted sender) smtp.mailfrom=sj@kernel.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1729276275; a=rsa-sha256; cv=none; b=HSmTGB9rIJJILtedXZs2Nh/NhFTwWTTcsA20sddBt1t1UQmaOxBsF72yZjR1rw1RzjN1Ag 22x2Rg3ji4uRj7HT1MbAoyZ33XDFSe0/vt4nkvh6Sb/fCu2VZ8euR3WHm4WhovIb84jUn7 0d5dHGc8A/NLuE4GKXfEj9q4G2jIC0k= ARC-Authentication-Results: i=1; imf26.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=McAj2UUm; dmarc=pass (policy=quarantine) header.from=kernel.org; spf=pass (imf26.hostedemail.com: domain of sj@kernel.org designates 147.75.193.91 as permitted sender) smtp.mailfrom=sj@kernel.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1729276275; 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=8/nFQBjKKrA98VryRhrl/uygg6SDN3qZfNW9NbF/pEw=; b=v2AmMxmvrUjYk6oKYZUmwH+xnhvx1bNoWqQeIb5hlzauVBBD4GgpIDIqQPOabVWBqXmMLn RhsJB6B6R3z86RisQfA6aTnHQHATQi0d3SKdm0Oh1AkpEvY/JYB8EfDvnVw4U40S+jLzkN edX3DO5gU1SyCHP7jkP0qr5Y6en41YY= Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by nyc.source.kernel.org (Postfix) with ESMTP id CE4AFA448D3; Fri, 18 Oct 2024 18:33:04 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 4AA92C4CEC3; Fri, 18 Oct 2024 18:33:13 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1729276393; bh=JTS7l00ywra+1THYcJZQsN2BffW5M/t8l3w1Xebv1OM=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=McAj2UUmemN7Ro4awQUL87JqeBbXfmKjq2cM8Fwd7q1LREln2bEm3wx7q57aMals4 GxZhlCzr+MgJQQ90mW2t+ztjMqF9QqARyIhzrbpoIGsbN4kIG+BBCeallm2Db5QJRL zmJmxFEddNqSjQcSpdiJKr4jhjE/d+1GZdPK818Vud4dIInqnGRM5k3xzgz3CAAzdp MzEKZFo6JkFyDFwpoAJRIy5rZ+ytsNCIBXLajDmjvD3M5dBr/epwJ8srxEShQg34BI /xGkU54J96atDcVKRSxml40m57fWnVKGty2Q85cfj6sQRLZtpuxtGGvPv3rXZJNDW1 WKcJbdpvbolqw== From: SeongJae Park To: Zheng Yejian Cc: SeongJae Park , akpm@linux-foundation.org, sieberf@amazon.com, shakeel.butt@linux.dev, foersleo@amazon.de, damon@lists.linux.dev, linux-mm@kvack.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] mm/damon/vaddr: Fix issue in damon_va_evenly_split_region() Date: Fri, 18 Oct 2024 11:33:10 -0700 Message-Id: <20241018183310.115850-1-sj@kernel.org> X-Mailer: git-send-email 2.39.5 In-Reply-To: <20241018035304.1050135-1-zhengyejian@huaweicloud.com> References: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Rspamd-Server: rspam12 X-Rspamd-Queue-Id: 3EDE3140008 X-Stat-Signature: 9grqq1guz7oi53hbeb31nma7fdygr497 X-Rspam-User: X-HE-Tag: 1729276386-201834 X-HE-Meta: U2FsdGVkX1+8oImqLTGKxhyWasR2xUFye0J2Vl0ou9xLuZLfguSgV8MQPhatxXVrUoKCK/JoKnElaW4KrCa230vEWykQBUKeHIZEwb6su5iHUhMX2bfBMav8qn98vOyO3xDamOv7bfGG7pGWEwRemcvP8M5KhyWF9e9O12Job8OrCpV6ef/1MlmbotUHyg50as/uBlVoVG9k/gIvtUmAGZ4bzoXv7Oe99yJTv8bZc3Oa7H5koKt+Jt83igKCU/D6tBLkDQnLKrhV5rDcaNiMaH4LKp3PLFOE6i57qoax63ZeHLwGpvhMPSTP2MmfP6RKXORiMrzuF24Pt/B9TXcIx0g4lBvNdilLe8H4PVHdcjGSK1wzdDC0obgJmgfgF/Jbk850fzM/5p6EY8ngkjog0mMsH4zKONNaZOrldgYHflE4QgmNyfB05N2MNMOf3Ra0bzIawoXaCSZYame4rs/88cBfWo28asgPZng/k7NQP5aZF9BwZI9DejemhvKHynCjgq2tAuCEZ+UXBky+wTv6XLGbQFh8eLbOyLPxr5z7/dLhr7vJn1GPGYkGsqn/i9QtWvUHEEo8mJ60Znswz0LQxH/Z4hFe8KNGmKgse3esosIHbyyIcf8MbYjildiiTHD4uAguNjMeyM5u/7tchfXpk4DzHlGTVh+HhsLMg/sYOONdfV/BLhnay7GqVC/DsklzUi3HR/LqIEIFp/FyXZBX+V17qWC2iBfkwydPSVd/KvnyukuotUNeRyq/+eJOg88FLgzcGxYUD6Yj9bzdEV5LmVXdX+u1SUsRasm+S+tdT6fYxE7QnXP7enrrGvyXSkIbsqpmdv2CU+p0OOiONY39hkccDBoPfKnzd2C5CZpnm9CVA0RfgXemUSjmj4QRIuhur4Eat+fcne5RxiHNSWrwAD5yJYVwzG5P0hNM1Crs+Y54ETygvxCPSb1T5CVoq9be4v9R69Wo1XRHJxgszuA iyeypWfc OcoJv8Hiwgj0hzj+SUP8fb4LBaWE/qKCtWK6FjUTjxTffF/9LLLeVs1rcCONAyfn+vykN1FOI97LjQ6grko/Kfjxchu+ulSs3IIVB1Wyi3jZwuRo4WnH8LXSFHzD2jwdEakfO15+3WcuK3TbIy7XyIsxr6tTC8B/g1pORV3bGLMHhNWv6Z+6TXdR94C6xQ5CZqp/PV/Aceetmz04UyVyzYw9M8kadC3100LTOUarII00bChnPcGDrp2NSfnoFiodmb+mYctdXOE5HbD81+G0ZyOb6Hl22d0cR92IIaEG7awSTmSZuLAHD/f2lwHyhG/gi7CQ3 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: Hi Zheng, Thank you for sharing this nice finding and fix! I have a few comments below. On Fri, 18 Oct 2024 11:53:04 +0800 Zheng Yejian wrote: > According to the logic of damon_va_evenly_split_region(), currently at > least following split cases would not meet the expectation: > > Suppose DAMON_MIN_REGION=0x1000, > Case1: Split [0x0, 0x1100) into 1 pieces, then the result would be > acutually [0x0, 0x1000), but NOT the expected [0x0, 0x1100) !!! Nice finding! However, as long as DAMON_MIN_REGION is respected, [0x0, 0x1100] region could not be created. So, the problematic case cannot happen in real? Please let me know if I'm missing something. And, why would someone call the function with nr_pieces 1? > Case2: Split [0x0, 0x3000) into 2 pieces, then the result would be > acutually 3 regions: > [0x0, 0x1000), [0x1000, 0x2000), [0x2000, 0x3000) > but NOT the expected 2 regions: > [0x0, 0x1000), [0x1000, 0x3000) !!! Nice finding! > > The root cause is that when calculating size of each split piece in > damon_va_evenly_split_region(): > > `sz_piece = ALIGN_DOWN(sz_orig / nr_pieces, DAMON_MIN_REGION);` > > both the dividing and the ALIGN_DOWN may cause loss of precision, > then each time split one piece of size 'sz_piece' from origin 'start' to > 'end' would cause: > 1. For the above Case1, the 'end' value of the split 1 piece is > aligned but not updated!!! > 2. For the above Case2, more pieces are split out than expected!!! > > To fix it, in this patch: > - As for the expect to split 1 piece, just return 0; As mentioned above, I think this is not needed, since the problematic case is unreal. > - Count for each piece split and make sure no more than 'nr_pieces'; > - Add above two cases into damon_test_split_evenly(). Thank you for adding tests! > > BTW, currently when running kunit test, DAMON_MIN_REGION is redefined > as 1, then above ALIGN_DOWN cases may not be test, since every int > value is ALIGN-ed to 1. > > After this patch, damon-operations test passed: > > # ./tools/testing/kunit/kunit.py run damon-operations > [...] > ============== damon-operations (6 subtests) =============== > [PASSED] damon_test_three_regions_in_vmas > [PASSED] damon_test_apply_three_regions1 > [PASSED] damon_test_apply_three_regions2 > [PASSED] damon_test_apply_three_regions3 > [PASSED] damon_test_apply_three_regions4 > [PASSED] damon_test_split_evenly > ================ [PASSED] damon-operations ================= > > Fixes: 3f49584b262c ("mm/damon: implement primitives for the virtual memory address spaces") > Signed-off-by: Zheng Yejian > --- > mm/damon/tests/vaddr-kunit.h | 2 ++ > mm/damon/vaddr.c | 13 +++++++++---- > 2 files changed, 11 insertions(+), 4 deletions(-) > > diff --git a/mm/damon/tests/vaddr-kunit.h b/mm/damon/tests/vaddr-kunit.h > index a339d117150f..b9a03e4e29e5 100644 > --- a/mm/damon/tests/vaddr-kunit.h > +++ b/mm/damon/tests/vaddr-kunit.h > @@ -300,6 +300,8 @@ static void damon_test_split_evenly(struct kunit *test) > damon_test_split_evenly_fail(test, 0, 100, 0); > damon_test_split_evenly_succ(test, 0, 100, 10); > damon_test_split_evenly_succ(test, 5, 59, 5); > + damon_test_split_evenly_succ(test, 4, 6, 1); If my above assumption (the first problem is unreal) is not wrong, maybe this test is not needed? > + damon_test_split_evenly_succ(test, 0, 3, 2); Nice. > damon_test_split_evenly_fail(test, 5, 6, 2); > } > > diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c > index 08cfd22b5249..1f3cebd20829 100644 > --- a/mm/damon/vaddr.c > +++ b/mm/damon/vaddr.c > @@ -67,10 +67,14 @@ static int damon_va_evenly_split_region(struct damon_target *t, > unsigned long sz_orig, sz_piece, orig_end; > struct damon_region *n = NULL, *next; > unsigned long start; > + int i; Purpose of this variable is counting the number of splitted regions, and comparing it against 'nr_pieces', right? Because nr_pieces is 'unsigned int', let's make this 'unsigned int' type, too. > > if (!r || !nr_pieces) > return -EINVAL; > > + if (nr_pieces == 1) > + return 0; > + As mentioned above, I don't think this is not needed. > orig_end = r->ar.end; > sz_orig = damon_sz_region(r); > sz_piece = ALIGN_DOWN(sz_orig / nr_pieces, DAMON_MIN_REGION); > @@ -79,9 +83,11 @@ static int damon_va_evenly_split_region(struct damon_target *t, > return -EINVAL; > > r->ar.end = r->ar.start + sz_piece; > + /* origin region will be updated as the first one after splitting */ I don't think this comment is easy to understand. Let's just remove it. > + i = 1; > + n = r; Why we need this? for 'nr_pieces == 1' case? If so, I don't think we need to take care about the case for the above mentioned reason. Please let me know if I'm missing something. > next = damon_next_region(r); > - for (start = r->ar.end; start + sz_piece <= orig_end; > - start += sz_piece) { > + for (start = r->ar.end; i < nr_pieces; start += sz_piece, i++) { > n = damon_new_region(start, start + sz_piece); > if (!n) > return -ENOMEM; > @@ -89,8 +95,7 @@ static int damon_va_evenly_split_region(struct damon_target *t, > r = n; > } > /* complement last region for possible rounding error */ > - if (n) > - n->ar.end = orig_end; > + n->ar.end = orig_end; Maybe this change is related with the above 'n = r' line? But, I don't think we need that, as commented there. > > return 0; > } > -- > 2.25.1 Thanks, SJ