Re: [PATCH 2/2] perf tools: Cope with differences for lib/list_sort.c copy from the kernel

[Arnaldo Carvalho de Melo]

On Mon, Sep 30, 2024 at 06:56:16PM -0700, Namhyung Kim wrote:
> On Mon, Sep 30, 2024 at 05:21:36PM -0300, Arnaldo Carvalho de Melo wrote:
> > From: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
> > 
> > With 6d74e1e371d43a7b ("tools/lib/list_sort: remove redundant code for
> > cond_resched handling") we need to use the newly added hunk based
> > exceptions when comparing the copy we carry in tools/lib/ to the
> > original file, do it by adding the hunks that we know will be the
> > expected diff.
> > 
> > If at some point the original file is updated in other parts, then we
> > should flag and check the file for update.
> > 
> > Cc: Adrian Hunter <adrian.hunter@xxxxxxxxx>
> > Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> > Cc: Ian Rogers <irogers@xxxxxxxxxx>
> > Cc: Jiri Olsa <jolsa@xxxxxxxxxx>
> > Cc: Kan Liang <kan.liang@xxxxxxxxxxxxxxx>
> > Cc: Kuan-Wei Chiu <visitorckw@xxxxxxxxx>
> > Cc: Namhyung Kim <namhyung@xxxxxxxxxx>
> > Signed-off-by: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
> 
> I'm seeing this but I think it's ok since we're adding a patch file as a
> content.
> 
>   Applying: perf tools: Cope with differences for lib/list_sort.c copy from the kernel
>   .git/rebase-apply/patch:21: space before tab in indent.
>    			struct list_head *a, struct list_head *b)
>   .git/rebase-apply/patch:23: space before tab in indent.
>    	struct list_head *tail = head;
>   .git/rebase-apply/patch:25: trailing whitespace.
>    
>   .git/rebase-apply/patch:26: space before tab in indent.
>    	for (;;) {
>   .git/rebase-apply/patch:27: space before tab in indent.
>    		/* if equal, take 'a' -- important for sort stability */
>   warning: squelched 6 whitespace errors
>   warning: 11 lines add whitespace errors.
> 
> And build doesn't show the message for lib/list_sort.c anymore. :)
> 
> I think it's nice to update for later changes, just put the diff in a

What kind of update you thinks its nice for later changes?

> file with the same name under check-header_ignore_hunks directory.

Isn't it like that already? Was the intention:

.../check-header_ignore_hunks/lib/list_sort.c | 31 +++++++++++++++++++

It is the same name as tools/lib/list_sort.c and lib/list_sort.c

Or did you mean something else?

> Good job!

Assuming all is right, thanks! 8-)

- Arnaldo
 
> Thanks,
> Namhyung
> 
> > ---
> >  .../check-header_ignore_hunks/lib/list_sort.c | 31 +++++++++++++++++++
> >  tools/perf/check-headers.sh                   |  5 ++-
> >  2 files changed, 35 insertions(+), 1 deletion(-)
> >  create mode 100644 tools/perf/check-header_ignore_hunks/lib/list_sort.c
> > 
> > diff --git a/tools/perf/check-header_ignore_hunks/lib/list_sort.c b/tools/perf/check-header_ignore_hunks/lib/list_sort.c
> > new file mode 100644
> > index 0000000000000000..32d98cb34f80a987
> > --- /dev/null
> > +++ b/tools/perf/check-header_ignore_hunks/lib/list_sort.c
> > @@ -0,0 +1,31 @@
> > +@@ -1,5 +1,6 @@
> > + // SPDX-License-Identifier: GPL-2.0
> > + #include <linux/kernel.h>
> > ++#include <linux/bug.h>
> > + #include <linux/compiler.h>
> > + #include <linux/export.h>
> > + #include <linux/string.h>
> > +@@ -52,6 +53,7 @@
> > + 			struct list_head *a, struct list_head *b)
> > + {
> > + 	struct list_head *tail = head;
> > ++	u8 count = 0;
> > + 
> > + 	for (;;) {
> > + 		/* if equal, take 'a' -- important for sort stability */
> > +@@ -77,6 +79,15 @@
> > + 	/* Finish linking remainder of list b on to tail */
> > + 	tail->next = b;
> > + 	do {
> > ++		/*
> > ++		 * If the merge is highly unbalanced (e.g. the input is
> > ++		 * already sorted), this loop may run many iterations.
> > ++		 * Continue callbacks to the client even though no
> > ++		 * element comparison is needed, so the client's cmp()
> > ++		 * routine can invoke cond_resched() periodically.
> > ++		 */
> > ++		if (unlikely(!++count))
> > ++			cmp(priv, b, b);
> > + 		b->prev = tail;
> > + 		tail = b;
> > + 		b = b->next;
> > diff --git a/tools/perf/check-headers.sh b/tools/perf/check-headers.sh
> > index 55aba47e5aec9292..f1080d4096663ba1 100755
> > --- a/tools/perf/check-headers.sh
> > +++ b/tools/perf/check-headers.sh
> > @@ -193,7 +193,6 @@ check include/uapi/linux/mman.h       '-I "^#include <\(uapi/\)*asm/mman.h>"'
> >  check include/linux/build_bug.h       '-I "^#\(ifndef\|endif\)\( \/\/\)* static_assert$"'
> >  check include/linux/ctype.h	      '-I "isdigit("'
> >  check lib/ctype.c		      '-I "^EXPORT_SYMBOL" -I "^#include <linux/export.h>" -B'
> > -check lib/list_sort.c		      '-I "^#include <linux/bug.h>"'
> >  
> >  # diff non-symmetric files
> >  check_2 tools/perf/arch/x86/entry/syscalls/syscall_32.tbl arch/x86/entry/syscalls/syscall_32.tbl
> > @@ -211,6 +210,10 @@ done
> >  check_2 tools/perf/util/hashmap.h tools/lib/bpf/hashmap.h
> >  check_2 tools/perf/util/hashmap.c tools/lib/bpf/hashmap.c
> >  
> > +# Files with larger differences
> > +
> > +check_ignore_some_hunks lib/list_sort.c
> > +
> >  cd tools/perf || exit
> >  
> >  if [ ${#FAILURES[@]} -gt 0 ]
> > -- 
> > 2.46.0
> >