Re: Problems running DAMON kunit tests with spinlock debugging enabled

[SeongJae Park <sj@kernel.org>]

Hi Guenter,

On Wed, 4 Sep 2024 14:04:03 -0700 Guenter Roeck <linux@roeck-us.net> wrote:

[...]
> Here is another problem. With all other problems fixed, when running on a 32-bit system:
> 
>      # damon_test_feed_loop_next_input: EXPECTATION FAILED at mm/damon/core-test.h:488
>      Expected damon_feed_loop_next_input(last_input, 200) > damon_feed_loop_next_input(last_input, 2000), but
>          damon_feed_loop_next_input(last_input, 200) == 923006 (0xe157e)
>          damon_feed_loop_next_input(last_input, 2000) == 1190503 (0x122a67)
>      # damon_test_feed_loop_next_input: pass:0 fail:1 skip:0 total:1
>      not ok 16 damon_test_feed_loop_next_input
> # damon: pass:14 fail:1 skip:1 total:16
> 
> Problem is that both damon_feed_loop_next_input(900000, 200) and
> damon_feed_loop_next_input(900000, 2000) overflow on 32-bit systems.

Thank you for reporting this yet another important issue!

> 
> Also,
> 
> static unsigned long damon_feed_loop_next_input(unsigned long last_input,
>                  unsigned long score)
> {
>          const unsigned long goal = 10000;
>          unsigned long score_goal_diff = max(goal, score) - min(goal, score);
>          unsigned long score_goal_diff_bp = score_goal_diff * 10000 / goal;
>                                                            ^^^^^^^^^^^^^^^
> 
> Since goal == 10000, the last statement has no effect.

You're right, thank you.

> 
> The diff below should address the problem for the most part, though the calculations
> may still overflow for large parameter values.
> 
> Guenter
> 
> ---
> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index 7a87628b76ab..ab9c625ee2e0 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -10,6 +10,7 @@
>   #include <linux/damon.h>
>   #include <linux/delay.h>
>   #include <linux/kthread.h>
> +#include <linux/math64.h>
>   #include <linux/mm.h>
>   #include <linux/psi.h>
>   #include <linux/slab.h>
> @@ -1451,8 +1452,7 @@ static unsigned long damon_feed_loop_next_input(unsigned long last_input,
>   {
>          const unsigned long goal = 10000;
>          unsigned long score_goal_diff = max(goal, score) - min(goal, score);
> -       unsigned long score_goal_diff_bp = score_goal_diff * 10000 / goal;
> -       unsigned long compensation = last_input * score_goal_diff_bp / 10000;
> +       unsigned long compensation = div_u64((unsigned long long)last_input * score_goal_diff, 10000);
>          /* Set minimum input as 10000 to avoid compensation be zero */
>          const unsigned long min_input = 10000;

Thank you for kindly sharing this, too.  Your detailed reports of symptoms,
root cause, and possible fix help a lot.

As you also mentioned, this function may better to be significantly re-written
with overflows in mind.  However, I have no enough time and setup to
sufficiently test the fix for now, due to some personal reasons.  Also, the
issue is important, but not time-critical for now, in my humble opinion.
Please let me know if someone has different opinions.

So, I made one possible fix off the top of my head, and posted as an RFC patch:
https://lore.kernel.org/20240905172405.46995-1-sj@kernel.org

I only tested that against the kunit test on my 64bit machine.  After I get
some time and setup (maybe 1-2 weeks later), I will take more time to review
the code, do more tests, and post RFC-dropped version.


Thanks,
SJ