Message ID | 90e083045243ef407dd592bb1deec89cd1f4ddf2.1700153535.git.robin.murphy@arm.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:b909:0:b0:403:3b70:6f57 with SMTP id t9csp3343255vqg; Thu, 16 Nov 2023 08:52:44 -0800 (PST) X-Google-Smtp-Source: AGHT+IF54dl1wFuHDkMD9EoHae/Yctbk6Hosr/vghOPD8BO3WOEGpiWU/56r8wYvruo48py1QfIR X-Received: by 2002:a05:6a20:8e27:b0:187:5fe9:3046 with SMTP id y39-20020a056a208e2700b001875fe93046mr6207205pzj.0.1700153564596; Thu, 16 Nov 2023 08:52:44 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1700153564; cv=none; d=google.com; s=arc-20160816; b=0Zcv3xDN/hQJYs1ahCGCeTge86CrdksKXTp4+hhviPu3ENJY04WGP89ulUiwbSKBh2 wp2j4bstDUocEMXEbqfi07pGsdaaRef/gf/7sUHg/Q+THmwTGpuKwTJZ47v/G2klFwic l9+UQr6rjqgcDvOiXYINjJWNR7ZW44VLhy6jpHiDrkeeiRjcxZwx/NbobA4gIfnIWe4b daO5yhYzaTzUM0Y6HO3/pF2D/rIe2YSbDQXjAEff1nAI4A1Y5umNzhtZ01Ye0U18MYCB ldy4FdYkzPbNg8TQ1OC7NurqytfJ1a5kUYJb0+mO6Trb8PPGtvxYML3hHrhuTneQVCmC YoOQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from; bh=nIqaptNlM2ZR2epG+YLXGWYJVvIIS3w2xWlP5bLLy8M=; fh=F02dF0BpMYLsXbgOFvt2Hl0mEOaagWk6RW0T4uGF2Zs=; b=CHC8jPCODabhhqADswwWXe5xt+Uw4cd+2bAQqAgEc/u7wD6269n9iOX8K9faJvecw2 0m3QpZT4Zb4t4WlvNosao/JNgZN4vaX04DjQ8S6B5ghF0V+qrKay5nbnqlItZaR1vwZF gHgeXSYcDNdNEFZuNZYcg4GuldesQ/UHDOiosAP7bZqSeYGVQ3aaYd95mNddFdQLAw5T AeT4HWAMWD5VtYTaxkvRTY4rjnoJhdOLo69JIaEO7ibgWY4cKTKw6wCKGo2xB5S6wK1i vNYb2REeEwlkCwZD3x6vlGsF2how/Mw4XnIMpfKj3bwpi6OfFFplLiHKSNShumTNpQX7 77wA== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=arm.com Received: from fry.vger.email (fry.vger.email. [2620:137:e000::3:8]) by mx.google.com with ESMTPS id p6-20020a056a000b4600b006be0559d029si13340383pfo.109.2023.11.16.08.52.44 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 16 Nov 2023 08:52:44 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=arm.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by fry.vger.email (Postfix) with ESMTP id 6D2AE80A28E3; Thu, 16 Nov 2023 08:52:42 -0800 (PST) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.11 at fry.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1344652AbjKPQw3 (ORCPT <rfc822;jaysivo@gmail.com> + 30 others); Thu, 16 Nov 2023 11:52:29 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:39586 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229841AbjKPQw2 (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 16 Nov 2023 11:52:28 -0500 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id 9B2D31A8 for <linux-kernel@vger.kernel.org>; Thu, 16 Nov 2023 08:52:24 -0800 (PST) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 8ABAB1595; Thu, 16 Nov 2023 08:53:10 -0800 (PST) Received: from e121345-lin.cambridge.arm.com (e121345-lin.cambridge.arm.com [10.1.196.40]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPA id 9F54E3F6C4; Thu, 16 Nov 2023 08:52:23 -0800 (PST) From: Robin Murphy <robin.murphy@arm.com> To: kevin.tian@intel.com, jgg@ziepe.ca Cc: joao.m.martins@oracle.com, iommu@lists.linux.dev, linux-kernel@vger.kernel.org Subject: [PATCH] iommufd/selftest: Fix dirty_bitmap tests Date: Thu, 16 Nov 2023 16:52:15 +0000 Message-Id: <90e083045243ef407dd592bb1deec89cd1f4ddf2.1700153535.git.robin.murphy@arm.com> X-Mailer: git-send-email 2.39.2.101.g768bb238c484.dirty MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-0.8 required=5.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on fry.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (fry.vger.email [0.0.0.0]); Thu, 16 Nov 2023 08:52:42 -0800 (PST) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1782740223842509578 X-GMAIL-MSGID: 1782740223842509578 |
Series |
iommufd/selftest: Fix dirty_bitmap tests
|
|
Commit Message
Robin Murphy
Nov. 16, 2023, 4:52 p.m. UTC
The ASSERT_EQ() macro sneakily expands to two statements, so the loop
here needs braces to ensure it captures both and actually terminates the
test upon failure. Where these tests are currently failing on my arm64
machine, this reduces the number of logged lines from a rather
unreasonable ~197,000 down to 10. While we're at it, we can also clean
up the tautologous "count" calculations whose assertions can never fail
unless mathematics and/or the C language become fundamentally broken.
Fixes: a9af47e382a4 ("iommufd/selftest: Test IOMMU_HWPT_GET_DIRTY_BITMAP")
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
tools/testing/selftests/iommu/iommufd_utils.h | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
Comments
On 16/11/2023 16:52, Robin Murphy wrote: > The ASSERT_EQ() macro sneakily expands to two statements, so the loop > here needs braces to ensure it captures both and actually terminates the > test upon failure. Ugh > Where these tests are currently failing on my arm64 > machine, this reduces the number of logged lines from a rather > unreasonable ~197,000 down to 10. While we're at it, we can also clean > up the tautologous "count" calculations whose assertions can never fail > unless mathematics and/or the C language become fundamentally broken. > > Fixes: a9af47e382a4 ("iommufd/selftest: Test IOMMU_HWPT_GET_DIRTY_BITMAP") > Signed-off-by: Robin Murphy <robin.murphy@arm.com> I was going to say that the second assert is useful, but we are already test the number of bits we set against what the mock domain set after mock_domain_set_dirty(). So the second is redundantly testing the same, and can be removed as you are doing. Thanks for fixing this. I would suggest the subject to: iommufd/selftest: Fix _test_mock_dirty_bitmaps() Because dirty-bitmap tests seems to imply the whole fixture, which covers more than the bitmaps. > --- > tools/testing/selftests/iommu/iommufd_utils.h | 13 ++++++------- > 1 file changed, 6 insertions(+), 7 deletions(-) > > diff --git a/tools/testing/selftests/iommu/iommufd_utils.h b/tools/testing/selftests/iommu/iommufd_utils.h > index 050e9751321c..ad9202335656 100644 > --- a/tools/testing/selftests/iommu/iommufd_utils.h > +++ b/tools/testing/selftests/iommu/iommufd_utils.h > @@ -293,15 +293,13 @@ static int _test_mock_dirty_bitmaps(int fd, __u32 hwpt_id, size_t length, > __u64 bitmap_size, __u32 flags, > struct __test_metadata *_metadata) > { > - unsigned long i, count, nbits = bitmap_size * BITS_PER_BYTE; > + unsigned long i, nbits = bitmap_size * BITS_PER_BYTE; > unsigned long nr = nbits / 2; > __u64 out_dirty = 0; > > /* Mark all even bits as dirty in the mock domain */ > - for (count = 0, i = 0; i < nbits; count += !(i % 2), i++) > - if (!(i % 2)) > - set_bit(i, (unsigned long *)bitmap); > - ASSERT_EQ(nr, count); > + for (i = 0; i < nbits; i += 2) > + set_bit(i, (unsigned long *)bitmap); > > test_cmd_mock_domain_set_dirty(fd, hwpt_id, length, iova, page_size, > bitmap, &out_dirty); > @@ -311,9 +309,10 @@ static int _test_mock_dirty_bitmaps(int fd, __u32 hwpt_id, size_t length, > memset(bitmap, 0, bitmap_size); > test_cmd_get_dirty_bitmap(fd, hwpt_id, length, iova, page_size, bitmap, > flags); > - for (count = 0, i = 0; i < nbits; count += !(i % 2), i++) > + /* Beware ASSERT_EQ() is two statements -- braces are not redundant! */ > + for (i = 0; i < nbits; i++) { > ASSERT_EQ(!(i % 2), test_bit(i, (unsigned long *)bitmap)); > - ASSERT_EQ(count, out_dirty); > + } > > memset(bitmap, 0, bitmap_size); > test_cmd_get_dirty_bitmap(fd, hwpt_id, length, iova, page_size, bitmap, Reviewed-by: Joao Martins <joao.m.martins@oracle.com> Tested-by: Joao Martins <joao.m.martins@oracle.com>
On 16/11/2023 5:28 pm, Joao Martins wrote: > On 16/11/2023 16:52, Robin Murphy wrote: >> The ASSERT_EQ() macro sneakily expands to two statements, so the loop >> here needs braces to ensure it captures both and actually terminates the >> test upon failure. > > Ugh > >> Where these tests are currently failing on my arm64 >> machine, this reduces the number of logged lines from a rather >> unreasonable ~197,000 down to 10. While we're at it, we can also clean >> up the tautologous "count" calculations whose assertions can never fail >> unless mathematics and/or the C language become fundamentally broken. >> >> Fixes: a9af47e382a4 ("iommufd/selftest: Test IOMMU_HWPT_GET_DIRTY_BITMAP") >> Signed-off-by: Robin Murphy <robin.murphy@arm.com> > > I was going to say that the second assert is useful, but we are already test the > number of bits we set against what the mock domain set after > mock_domain_set_dirty(). So the second is redundantly testing the same, and can > be removed as you are doing. Thanks for fixing this. Yeah, it's still effectively just counting half the number of loop iterations executed, but since there's no control flow that could exit the loop early and still reach the assertion, it must always be true following the previous assertion that out_dirty == nr == nbits/2. > I would suggest the subject to: > > iommufd/selftest: Fix _test_mock_dirty_bitmaps() > > Because dirty-bitmap tests seems to imply the whole fixture, which covers more > than the bitmaps. Sure, that sounds reasonable. Jason, Kevin, would you want a v2 for that or could it be fixed up when applying? >> --- >> tools/testing/selftests/iommu/iommufd_utils.h | 13 ++++++------- >> 1 file changed, 6 insertions(+), 7 deletions(-) >> >> diff --git a/tools/testing/selftests/iommu/iommufd_utils.h b/tools/testing/selftests/iommu/iommufd_utils.h >> index 050e9751321c..ad9202335656 100644 >> --- a/tools/testing/selftests/iommu/iommufd_utils.h >> +++ b/tools/testing/selftests/iommu/iommufd_utils.h >> @@ -293,15 +293,13 @@ static int _test_mock_dirty_bitmaps(int fd, __u32 hwpt_id, size_t length, >> __u64 bitmap_size, __u32 flags, >> struct __test_metadata *_metadata) >> { >> - unsigned long i, count, nbits = bitmap_size * BITS_PER_BYTE; >> + unsigned long i, nbits = bitmap_size * BITS_PER_BYTE; >> unsigned long nr = nbits / 2; >> __u64 out_dirty = 0; >> >> /* Mark all even bits as dirty in the mock domain */ >> - for (count = 0, i = 0; i < nbits; count += !(i % 2), i++) >> - if (!(i % 2)) >> - set_bit(i, (unsigned long *)bitmap); >> - ASSERT_EQ(nr, count); >> + for (i = 0; i < nbits; i += 2) >> + set_bit(i, (unsigned long *)bitmap); >> >> test_cmd_mock_domain_set_dirty(fd, hwpt_id, length, iova, page_size, >> bitmap, &out_dirty); >> @@ -311,9 +309,10 @@ static int _test_mock_dirty_bitmaps(int fd, __u32 hwpt_id, size_t length, >> memset(bitmap, 0, bitmap_size); >> test_cmd_get_dirty_bitmap(fd, hwpt_id, length, iova, page_size, bitmap, >> flags); >> - for (count = 0, i = 0; i < nbits; count += !(i % 2), i++) >> + /* Beware ASSERT_EQ() is two statements -- braces are not redundant! */ >> + for (i = 0; i < nbits; i++) { >> ASSERT_EQ(!(i % 2), test_bit(i, (unsigned long *)bitmap)); >> - ASSERT_EQ(count, out_dirty); >> + } >> >> memset(bitmap, 0, bitmap_size); >> test_cmd_get_dirty_bitmap(fd, hwpt_id, length, iova, page_size, bitmap, > > Reviewed-by: Joao Martins <joao.m.martins@oracle.com> > Tested-by: Joao Martins <joao.m.martins@oracle.com> Thanks! Robin.
> From: Robin Murphy <robin.murphy@arm.com> > Sent: Friday, November 17, 2023 1:44 AM > > On 16/11/2023 5:28 pm, Joao Martins wrote: > > On 16/11/2023 16:52, Robin Murphy wrote: > >> The ASSERT_EQ() macro sneakily expands to two statements, so the loop > >> here needs braces to ensure it captures both and actually terminates the > >> test upon failure. > > > > Ugh > > > >> Where these tests are currently failing on my arm64 > >> machine, this reduces the number of logged lines from a rather > >> unreasonable ~197,000 down to 10. While we're at it, we can also clean > >> up the tautologous "count" calculations whose assertions can never fail > >> unless mathematics and/or the C language become fundamentally broken. > >> > >> Fixes: a9af47e382a4 ("iommufd/selftest: Test > IOMMU_HWPT_GET_DIRTY_BITMAP") > >> Signed-off-by: Robin Murphy <robin.murphy@arm.com> > > > > I was going to say that the second assert is useful, but we are already test > the > > number of bits we set against what the mock domain set after > > mock_domain_set_dirty(). So the second is redundantly testing the same, > and can > > be removed as you are doing. Thanks for fixing this. > > Yeah, it's still effectively just counting half the number of loop > iterations executed, but since there's no control flow that could exit > the loop early and still reach the assertion, it must always be true > following the previous assertion that out_dirty == nr == nbits/2. > > > I would suggest the subject to: > > > > iommufd/selftest: Fix _test_mock_dirty_bitmaps() > > > > Because dirty-bitmap tests seems to imply the whole fixture, which covers > more > > than the bitmaps. > > Sure, that sounds reasonable. Jason, Kevin, would you want a v2 for that > or could it be fixed up when applying? > Jason can help fix it when applying. Reviewed-by: Kevin Tian <kevin.tian@intel.com>
On Fri, Nov 17, 2023 at 03:15:25AM +0000, Tian, Kevin wrote: > > From: Robin Murphy <robin.murphy@arm.com> > > Sent: Friday, November 17, 2023 1:44 AM > > > > On 16/11/2023 5:28 pm, Joao Martins wrote: > > > On 16/11/2023 16:52, Robin Murphy wrote: > > >> The ASSERT_EQ() macro sneakily expands to two statements, so the loop > > >> here needs braces to ensure it captures both and actually terminates the > > >> test upon failure. > > > > > > Ugh > > > > > >> Where these tests are currently failing on my arm64 > > >> machine, this reduces the number of logged lines from a rather > > >> unreasonable ~197,000 down to 10. While we're at it, we can also clean > > >> up the tautologous "count" calculations whose assertions can never fail > > >> unless mathematics and/or the C language become fundamentally broken. > > >> > > >> Fixes: a9af47e382a4 ("iommufd/selftest: Test > > IOMMU_HWPT_GET_DIRTY_BITMAP") > > >> Signed-off-by: Robin Murphy <robin.murphy@arm.com> > > > > > > I was going to say that the second assert is useful, but we are already test > > the > > > number of bits we set against what the mock domain set after > > > mock_domain_set_dirty(). So the second is redundantly testing the same, > > and can > > > be removed as you are doing. Thanks for fixing this. > > > > Yeah, it's still effectively just counting half the number of loop > > iterations executed, but since there's no control flow that could exit > > the loop early and still reach the assertion, it must always be true > > following the previous assertion that out_dirty == nr == nbits/2. > > > > > I would suggest the subject to: > > > > > > iommufd/selftest: Fix _test_mock_dirty_bitmaps() > > > > > > Because dirty-bitmap tests seems to imply the whole fixture, which covers > > more > > > than the bitmaps. > > > > Sure, that sounds reasonable. Jason, Kevin, would you want a v2 for that > > or could it be fixed up when applying? > > > > Jason can help fix it when applying. > > Reviewed-by: Kevin Tian <kevin.tian@intel.com> Done Thanks, Jason
diff --git a/tools/testing/selftests/iommu/iommufd_utils.h b/tools/testing/selftests/iommu/iommufd_utils.h index 050e9751321c..ad9202335656 100644 --- a/tools/testing/selftests/iommu/iommufd_utils.h +++ b/tools/testing/selftests/iommu/iommufd_utils.h @@ -293,15 +293,13 @@ static int _test_mock_dirty_bitmaps(int fd, __u32 hwpt_id, size_t length, __u64 bitmap_size, __u32 flags, struct __test_metadata *_metadata) { - unsigned long i, count, nbits = bitmap_size * BITS_PER_BYTE; + unsigned long i, nbits = bitmap_size * BITS_PER_BYTE; unsigned long nr = nbits / 2; __u64 out_dirty = 0; /* Mark all even bits as dirty in the mock domain */ - for (count = 0, i = 0; i < nbits; count += !(i % 2), i++) - if (!(i % 2)) - set_bit(i, (unsigned long *)bitmap); - ASSERT_EQ(nr, count); + for (i = 0; i < nbits; i += 2) + set_bit(i, (unsigned long *)bitmap); test_cmd_mock_domain_set_dirty(fd, hwpt_id, length, iova, page_size, bitmap, &out_dirty); @@ -311,9 +309,10 @@ static int _test_mock_dirty_bitmaps(int fd, __u32 hwpt_id, size_t length, memset(bitmap, 0, bitmap_size); test_cmd_get_dirty_bitmap(fd, hwpt_id, length, iova, page_size, bitmap, flags); - for (count = 0, i = 0; i < nbits; count += !(i % 2), i++) + /* Beware ASSERT_EQ() is two statements -- braces are not redundant! */ + for (i = 0; i < nbits; i++) { ASSERT_EQ(!(i % 2), test_bit(i, (unsigned long *)bitmap)); - ASSERT_EQ(count, out_dirty); + } memset(bitmap, 0, bitmap_size); test_cmd_get_dirty_bitmap(fd, hwpt_id, length, iova, page_size, bitmap,