Re: FAILED: patch "[PATCH] mm/damon/core: merge regions aggressively when max_nr_regions" failed to apply to 6.1-stable tree

[Greg KH <gregkh@linuxfoundation.org>]

On Mon, Jul 15, 2024 at 12:59:45PM -0700, SeongJae Park wrote:
> On Mon, 15 Jul 2024 19:12:29 +0200 Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> > On Mon, Jul 15, 2024 at 09:57:17AM -0700, SeongJae Park wrote:
> > > Hi Greg,
> > > 
> > > On Mon, 15 Jul 2024 13:34:48 +0200 <gregkh@linuxfoundation.org> wrote:
> > > 
> > > > 
> > > > The patch below does not apply to the 6.1-stable tree.
> > > > If someone wants it applied there, or to any other stable or longterm
> > > > tree, then please email the backport, including the original git commit
> > > > id to <stable@vger.kernel.org>.
> > > > 
> > > > To reproduce the conflict and resubmit, you may use the following commands:
> > > > 
> > > > git fetch https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/ linux-6.1.y
> > > > git checkout FETCH_HEAD
> > > > git cherry-pick -x 310d6c15e9104c99d5d9d0ff8e5383a79da7d5e6
> > > 
> > > But this doesn't reproduce the failure on my machine, like below?
> > > 
> > >     $ git fetch https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/ linux-6.1.y
> > >     [...]
> > >     $ git checkout FETCH_HEAD
> > >     [...]
> > >     HEAD is now at cac15753b8ce Linux 6.1.99
> > >     $ git cherry-pick -x 310d6c15e9104c99d5d9d0ff8e5383a79da7d5e6
> > >     Auto-merging mm/damon/core.c
> > >     [detached HEAD ecd04159c5f3] mm/damon/core: merge regions aggressively when max_nr_regions is unmet
> > >      Date: Mon Jun 24 10:58:14 2024 -0700
> > >      1 file changed, 19 insertions(+), 2 deletions(-)
> > 
> > Try building it:
> > 
> >   DESCEND objtool
> >   CALL    scripts/checksyscalls.sh
> >   CC      mm/damon/core.o
> > In file included from ./include/linux/kernel.h:26,
> >                  from ./arch/x86/include/asm/percpu.h:27,
> >                  from ./arch/x86/include/asm/preempt.h:6,
> >                  from ./include/linux/preempt.h:79,
> >                  from ./include/linux/spinlock.h:56,
> >                  from ./include/linux/swait.h:7,
> >                  from ./include/linux/completion.h:12,
> >                  from ./include/linux/damon.h:11,
> >                  from mm/damon/core.c:10:
> > mm/damon/core.c: In function \u2018kdamond_merge_regions\u2019:
> > ./include/linux/minmax.h:20:35: error: comparison of distinct pointer types lacks a cast [-Werror]
> >    20 |         (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
> >       |                                   ^~
> > ./include/linux/minmax.h:26:18: note: in expansion of macro \u2018__typecheck\u2019
> >    26 |                 (__typecheck(x, y) && __no_side_effects(x, y))
> >       |                  ^~~~~~~~~~~
> > ./include/linux/minmax.h:36:31: note: in expansion of macro \u2018__safe_cmp\u2019
> >    36 |         __builtin_choose_expr(__safe_cmp(x, y), \
> >       |                               ^~~~~~~~~~
> > ./include/linux/minmax.h:52:25: note: in expansion of macro \u2018__careful_cmp\u2019
> >    52 | #define max(x, y)       __careful_cmp(x, y, >)
> >       |                         ^~~~~~~~~~~~~
> > mm/damon/core.c:946:29: note: in expansion of macro \u2018max\u2019
> >   946 |                 threshold = max(1, threshold * 2);
> >       |                             ^~~
> > cc1: all warnings being treated as errors
> > make[3]: *** [scripts/Makefile.build:250: mm/damon/core.o] Error 1
> > make[2]: *** [scripts/Makefile.build:503: mm/damon] Error 2
> > make[1]: *** [scripts/Makefile.build:503: mm] Error 2
> 
> Thank you for sharing this.
> 
> I found the issue can be fixed in two ways.  I'd like to know what way you'd
> prefer.
> 
> The first way is adding below simple fix to mm/damon/core.c file.
> 
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -943,7 +943,7 @@ static void kdamond_merge_regions(struct damon_ctx *c, unsigned int threshold,
>                         damon_merge_regions_of(t, threshold, sz_limit);
>                         nr_regions += damon_nr_regions(t);
>                 }
> -               threshold = max(1, threshold * 2);
> +               threshold = max(1U, threshold * 2);
>         } while (nr_regions > c->attrs.max_nr_regions &&
>                         threshold / 2 < max_thres);
>  }
> 
> The second way is adding upstream commits that avoids the warning of DAMON code
> on >=6.6 kernels.  Specifically, commit 867046cc7027 ("minmax: relax check to
> allow comparison between unsigned arguments and signed constants") is needed.
> However, the commit cannot cleanly cherry-picked on its own.  Cherry-picking
> the commit together with below commits (listed latest one first) made all
> commits cleanly be picked and the warning disappears.
> 
> 4ead534fba42 minmax: allow comparisons of 'int' against 'unsigned char/short'
> d03eba99f5bf minmax: allow min()/max()/clamp() if the arguments have the same signedness.
> 2122e2a4efc2 minmax: clamp more efficiently by avoiding extra comparison
> 5efcecd9a3b1 minmax: sanity check constant bounds when clamping

I just tried the above, and then added your commit, but I still get the
same build error.  Did you try it?

I would love to get the minmax stuff properly backported to 6.1 (and
older if possible), as we have run into this same issue with many
changes over the years.

> Same warning happens on 5.15.y.  In the case, adding the minmax.h upstream
> commits only adds more build errors.  To remove those, yet another upstream
> commit, namely commit a49a64b5bf19 ("tracing: Define the is_signed_type() macro
> once") need to be cherry-picked.
> 
> IMHO, the second way is more complex but right for long term, since future
> commits for stable tree may also have similar issue.  It is not a strong
> opinion, and either ways work for me.
> 
> So, Greg, what would you prefer?

I would prefer the second way, IF it works.  In my limited testing right
now, I couldn't get it to work at all.  Can you send a series of
backported patches that work for you so that I ensure that I'm not just
doing something stupid on my end?

thanks,

greg k-h