Messages in this thread | | | Date | Thu, 16 Nov 2023 17:28:55 +0000 | Subject | Re: [PATCH] iommufd/selftest: Fix dirty_bitmap tests | From | Joao Martins <> |
| |
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>
| |