Re: Linux 6.8-rc5

[Linus Torvalds]

On Tue, 20 Feb 2024 at 11:09, Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
>
> Build results:
>         total: 155 pass: 151 fail: 4
> Failed builds:
>         csky:allmodconfig
>         openrisc:allmodconfig
>         parisc:allmodconfig
>         xtensa:allmodconfig
> Qemu test results:
>         total: 549 pass: 547 fail: 2

Grr, I was hoping things would improve, not go backwards.

> ERROR: modpost: "__umoddi3" [drivers/gpu/drm/tests/drm_buddy_test.ko] undefined!
> ERROR: modpost: "__moddi3" [drivers/gpu/drm/tests/drm_buddy_test.ko] undefined!
>
> Commit a64056bb5a32 ("drm/tests/drm_buddy: add alloc_contiguous test"):
>
> +       u64 mm_size, ps = SZ_4K, i, n_pages, total;
> ...
> +       n_pages = mm_size / ps;

WTF? This code isn't lovely, but the build error indicates a complete
lack of compiler optimizations.

As far as I can tell, 'ps' is never modified, so the compiler should
be able to just treat it as a constant.

And 'mm_size' is a constant too in this context. It's a local variable
assigned once, with a compile-time constant value.

So that

        n_pages = mm_size / ps;

should be a constant value too (and that value should be 48).

Now, the __moddi3() is a *bit* more reasonable, because I assume it comes from

                int slot = i % 3;

where 'i' is marked as u64 too. For no good reason (it goes from 0 to
47), but it too does show a certain lack of basic optimizations (but
now it's at least a slightly more *complex* optimization because it
depends on the whole value range propagation).

None of this should be 'u64'. Even if the compiler has been unusually stupid.

The 'total' variable could possibly be considered validly be u64,
although even that is very very questinable.

> This patch breaks the build on all 32-bit systems since it introduces an
> unhandled direct 64-bit divide operation.

It turns out that that commit is buggy for another reason, but it's
hidden by the fact that apparently KUNIT_ASSERT_FALSE_MSG() doesn't
check the format string.

It randomly uses '%d' or '%llu' to print out various variations of
'ps'. All garbage.

Yes, yes, drm_buddy_init() takes u64 arguments. That in itself is a
bit questionable, but whatever. It does *NOT* mean that the variables
that don't need it should then be u64.

Suggested untested patch attached.

                  Linus
 drivers/gpu/drm/tests/drm_buddy_test.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/tests/drm_buddy_test.c b/drivers/gpu/drm/tests/drm_buddy_test.c
index fee6bec757d1..3dbfa3078449 100644
--- a/drivers/gpu/drm/tests/drm_buddy_test.c
+++ b/drivers/gpu/drm/tests/drm_buddy_test.c
@@ -21,7 +21,8 @@ static inline u64 get_size(int order, u64 chunk_size)
 
 static void drm_test_buddy_alloc_contiguous(struct kunit *test)
 {
-	u64 mm_size, ps = SZ_4K, i, n_pages, total;
+	const unsigned long ps = SZ_4K, mm_size = 16 * 3 * SZ_4K;
+	unsigned long i, n_pages, total;
 	struct drm_buddy_block *block;
 	struct drm_buddy mm;
 	LIST_HEAD(left);
@@ -29,8 +30,6 @@ static void drm_test_buddy_alloc_contiguous(struct kunit *test)
 	LIST_HEAD(right);
 	LIST_HEAD(allocated);
 
-	mm_size = 16 * 3 * SZ_4K;
-
 	KUNIT_EXPECT_FALSE(test, drm_buddy_init(&mm, mm_size, ps));
 
 	/*
@@ -56,30 +55,30 @@ static void drm_test_buddy_alloc_contiguous(struct kunit *test)
 		KUNIT_ASSERT_FALSE_MSG(test,
 				       drm_buddy_alloc_blocks(&mm, 0, mm_size,
 							      ps, ps, list, 0),
-				       "buddy_alloc hit an error size=%d\n",
+				       "buddy_alloc hit an error size=%lu\n",
 				       ps);
 	} while (++i < n_pages);
 
 	KUNIT_ASSERT_TRUE_MSG(test, drm_buddy_alloc_blocks(&mm, 0, mm_size,
 							   3 * ps, ps, &allocated,
 							   DRM_BUDDY_CONTIGUOUS_ALLOCATION),
-			       "buddy_alloc didn't error size=%d\n", 3 * ps);
+			       "buddy_alloc didn't error size=%lu\n", 3 * ps);
 
 	drm_buddy_free_list(&mm, &middle);
 	KUNIT_ASSERT_TRUE_MSG(test, drm_buddy_alloc_blocks(&mm, 0, mm_size,
 							   3 * ps, ps, &allocated,
 							   DRM_BUDDY_CONTIGUOUS_ALLOCATION),
-			       "buddy_alloc didn't error size=%llu\n", 3 * ps);
+			       "buddy_alloc didn't error size=%lu\n", 3 * ps);
 	KUNIT_ASSERT_TRUE_MSG(test, drm_buddy_alloc_blocks(&mm, 0, mm_size,
 							   2 * ps, ps, &allocated,
 							   DRM_BUDDY_CONTIGUOUS_ALLOCATION),
-			       "buddy_alloc didn't error size=%llu\n", 2 * ps);
+			       "buddy_alloc didn't error size=%lu\n", 2 * ps);
 
 	drm_buddy_free_list(&mm, &right);
 	KUNIT_ASSERT_TRUE_MSG(test, drm_buddy_alloc_blocks(&mm, 0, mm_size,
 							   3 * ps, ps, &allocated,
 							   DRM_BUDDY_CONTIGUOUS_ALLOCATION),
-			       "buddy_alloc didn't error size=%llu\n", 3 * ps);
+			       "buddy_alloc didn't error size=%lu\n", 3 * ps);
 	/*
 	 * At this point we should have enough contiguous space for 2 blocks,
 	 * however they are never buddies (since we freed middle and right) so
@@ -88,13 +87,13 @@ static void drm_test_buddy_alloc_contiguous(struct kunit *test)
 	KUNIT_ASSERT_FALSE_MSG(test, drm_buddy_alloc_blocks(&mm, 0, mm_size,
 							    2 * ps, ps, &allocated,
 							    DRM_BUDDY_CONTIGUOUS_ALLOCATION),
-			       "buddy_alloc hit an error size=%d\n", 2 * ps);
+			       "buddy_alloc hit an error size=%lu\n", 2 * ps);
 
 	drm_buddy_free_list(&mm, &left);
 	KUNIT_ASSERT_FALSE_MSG(test, drm_buddy_alloc_blocks(&mm, 0, mm_size,
 							    3 * ps, ps, &allocated,
 							    DRM_BUDDY_CONTIGUOUS_ALLOCATION),
-			       "buddy_alloc hit an error size=%d\n", 3 * ps);
+			       "buddy_alloc hit an error size=%lu\n", 3 * ps);
 
 	total = 0;
 	list_for_each_entry(block, &allocated, link)