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

[SeongJae Park <sj@kernel.org>]

On Tue, 16 Jul 2024 09:16:19 +0200 Greg KH <gregkh@linuxfoundation.org> wrote:

> 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:
> > > 
[...]
> > > 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?

Yes, I tried and confirmed that on my machine.  I'm wondering if you applied
only the four patches on the above list?  To fix the warning, the commit
867046cc7027 ("minmax: relax check to allow comparison between unsigned
arguments and signed constants") should also be applied on top of those.

Nonetheless, with more testing, I found it causes yet another error on my kunit
build.  It required further picking commit f6e9d38f8eb0 ("minmax: fix header
inclusions") to fix 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.

Yes, I agree.

> 
> > 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?

Thank you for the kind and clear answer.  I just posted the patches:
https://lore.kernel.org/20240716175205.51280-1-sj@kernel.org

Please let me know if it still not works.


Thanks,
SJ

[...]