[PATCH 0/2] check_headers.sh with hunk exceptions (lib/list_sort.c tools/ copy)

[PATCH 0/2] check_headers.sh with hunk exceptions (lib/list_sort.c tools/ copy)

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

Hi,

	Please take a look, as per tools/include/uapi/README we carry
copies of kernel files for various reasons and check when it drifts, in
this case we need another way to accept diffs while checking, its all
spelled out in the individual patches, please ack.

- Arnaldo

Arnaldo Carvalho de Melo (2):
  tools check_headers.sh: Add check variant that excludes some hunks
  perf tools: Cope with differences for lib/list_sort.c copy from the kernel

 .../check-header_ignore_hunks/lib/list_sort.c | 31 +++++++++++++++++++
 tools/perf/check-headers.sh                   | 29 ++++++++++++++++-
 2 files changed, 59 insertions(+), 1 deletion(-)
 create mode 100644 tools/perf/check-header_ignore_hunks/lib/list_sort.c

-- 
2.46.0

[PATCH 1/2] tools check_headers.sh: Add check variant that excludes some hunks

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

With 6d74e1e371d43a7b ("tools/lib/list_sort: remove redundant code for
cond_resched handling") we end up with a multi-line variation in the
merge_final() implementation, one that the simple line based exceptions
we had so far can't cope.

Thus this check has been failing:

  Warning: Kernel ABI header differences:
    diff -u tools/lib/list_sort.c lib/list_sort.c

So add a new check routine that uses grep -vf to exclude some hunks that
we store in the tools/perf/check-header_ignore_hunks/ directory.

This first patch is just the new check routine, the next one will use it
to check lib/list_sort.c.

Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Ian Rogers <irogers@google.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Kan Liang <kan.liang@linux.intel.com>
Cc: Kuan-Wei Chiu <visitorckw@gmail.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/check-headers.sh | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/tools/perf/check-headers.sh b/tools/perf/check-headers.sh
index 714c78e5da07c163..55aba47e5aec9292 100755
--- a/tools/perf/check-headers.sh
+++ b/tools/perf/check-headers.sh
@@ -136,6 +136,30 @@ beauty_check () {
   check_2 "tools/perf/trace/beauty/$file" "$file" "$@"
 }
 
+check_ignore_some_hunks () {
+  orig_file="$1"
+  tools_file="tools/$orig_file"
+  hunks_to_ignore="tools/perf/check-header_ignore_hunks/$orig_file"
+
+  if [ ! -f "$hunks_to_ignore" ]; then
+    echo "$hunks_to_ignore not found. Skipping $orig_file check."
+    FAILURES+=(
+      "$tools_file $orig_file"
+    )
+    return
+  fi
+
+  cmd="diff -u \"$tools_file\" \"$orig_file\" | grep -vf \"$hunks_to_ignore\" | wc -l | grep -qw 0"
+
+  if [ -f "$orig_file" ] && ! eval "$cmd"
+  then
+    FAILURES+=(
+      "$tools_file $orig_file"
+    )
+  fi
+}
+
+
 # Check if we have the kernel headers (tools/perf/../../include), else
 # we're probably on a detached tarball, so no point in trying to check
 # differences.
-- 
2.46.0

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

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

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@intel.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Ian Rogers <irogers@google.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Kan Liang <kan.liang@linux.intel.com>
Cc: Kuan-Wei Chiu <visitorckw@gmail.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 .../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

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

[Namhyung Kim]

On Mon, Sep 30, 2024 at 05:21:36PM -0300, Arnaldo Carvalho de Melo wrote:
> From: Arnaldo Carvalho de Melo <acme@redhat.com>
> 
> 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@intel.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Ian Rogers <irogers@google.com>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: Kan Liang <kan.liang@linux.intel.com>
> Cc: Kuan-Wei Chiu <visitorckw@gmail.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

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
file with the same name under check-header_ignore_hunks directory.
Good job!

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
> 

Re: [PATCH 0/2] check_headers.sh with hunk exceptions (lib/list_sort.c tools/ copy)

[Kuan-Wei Chiu]

On Mon, Sep 30, 2024 at 05:21:34PM -0300, Arnaldo Carvalho de Melo wrote:
> From: Arnaldo Carvalho de Melo <acme@redhat.com>
> 
> Hi,
> 
> 	Please take a look, as per tools/include/uapi/README we carry
> copies of kernel files for various reasons and check when it drifts, in
> this case we need another way to accept diffs while checking, its all
> spelled out in the individual patches, please ack.
> 
> - Arnaldo
> 
> Arnaldo Carvalho de Melo (2):
>   tools check_headers.sh: Add check variant that excludes some hunks
>   perf tools: Cope with differences for lib/list_sort.c copy from the kernel
>
LGTM. For the series:

Acked-by: Kuan-Wei Chiu <visitorckw@gmail.com>

While reviewing the patches, I noticed that there was already a
difference between lib/list_sort.c and tools/lib/list_sort.c regarding
an additional #include <linux/bug.h>. This prompted me to investigate
the reason for this discrepancy. From what I can see, both files only
seem to require only three headers:

#include <linux/compiler.h> /* for likely() macro */
#include <linux/export.h> /* for EXPORT_SYMBOL() macro */
#include <linux/list_sort.h> /* for list_sort() and linux/types.h */

I'll check the git history and run build tests to confirm. If only
these headers are needed, I'll submit a cleanup patch.

Regards,
Kuan-Wei

>  .../check-header_ignore_hunks/lib/list_sort.c | 31 +++++++++++++++++++
>  tools/perf/check-headers.sh                   | 29 ++++++++++++++++-
>  2 files changed, 59 insertions(+), 1 deletion(-)
>  create mode 100644 tools/perf/check-header_ignore_hunks/lib/list_sort.c
> 
> -- 
> 2.46.0
> 

Re: [PATCH 0/2] check_headers.sh with hunk exceptions (lib/list_sort.c tools/ copy)

[Arnaldo Carvalho de Melo]

On Thu, Oct 03, 2024 at 01:21:02AM +0800, Kuan-Wei Chiu wrote:
> On Mon, Sep 30, 2024 at 05:21:34PM -0300, Arnaldo Carvalho de Melo wrote:
> > From: Arnaldo Carvalho de Melo <acme@redhat.com>
> > 	Please take a look, as per tools/include/uapi/README we carry
> > copies of kernel files for various reasons and check when it drifts, in
> > this case we need another way to accept diffs while checking, its all
> > spelled out in the individual patches, please ack.

> > Arnaldo Carvalho de Melo (2):
> >   tools check_headers.sh: Add check variant that excludes some hunks
> >   perf tools: Cope with differences for lib/list_sort.c copy from the kernel

> LGTM. For the series:
 
> Acked-by: Kuan-Wei Chiu <visitorckw@gmail.com>

Adding it to that cset before pushing to the written in stone
perf-tools/perf-tools branch.
 
> While reviewing the patches, I noticed that there was already a
> difference between lib/list_sort.c and tools/lib/list_sort.c regarding
> an additional #include <linux/bug.h>. This prompted me to investigate
> the reason for this discrepancy. From what I can see, both files only
> seem to require only three headers:

> #include <linux/compiler.h> /* for likely() macro */
> #include <linux/export.h> /* for EXPORT_SYMBOL() macro */
> #include <linux/list_sort.h> /* for list_sort() and linux/types.h */
 
> I'll check the git history and run build tests to confirm. If only
> these headers are needed, I'll submit a cleanup patch.

tools/ is a sidecar or sorts for the kernel, that tries to add value to
kernel developers while not getting in their way.

Sometimes things we discover while using more widely things that were
designed for use in the kernel source may be of help to kernel
developers, if this is one such case, great!

But IIRC that linux/bug.h discrepancy, without further checking, was
something already somehow accepted via:

+check lib/list_sort.c                '-I "^#include <linux/bug.h>"'

in the cset that introduced the copy:

  92ec3cc94c2cb60d ("tools lib: Adopt list_sort() from the kernel sources")

- Arnaldo

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@redhat.com>
> > 
> > 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@intel.com>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Ian Rogers <irogers@google.com>
> > Cc: Jiri Olsa <jolsa@kernel.org>
> > Cc: Kan Liang <kan.liang@linux.intel.com>
> > Cc: Kuan-Wei Chiu <visitorckw@gmail.com>
> > Cc: Namhyung Kim <namhyung@kernel.org>
> > Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> 
> 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
> > 

Re: [PATCH 0/2] check_headers.sh with hunk exceptions (lib/list_sort.c tools/ copy)

[Namhyung Kim]

Hi Arnaldo,

On Wed, Oct 02, 2024 at 02:29:52PM -0300, Arnaldo Carvalho de Melo wrote:
> On Thu, Oct 03, 2024 at 01:21:02AM +0800, Kuan-Wei Chiu wrote:
> > On Mon, Sep 30, 2024 at 05:21:34PM -0300, Arnaldo Carvalho de Melo wrote:
> > > From: Arnaldo Carvalho de Melo <acme@redhat.com>
> > > 	Please take a look, as per tools/include/uapi/README we carry
> > > copies of kernel files for various reasons and check when it drifts, in
> > > this case we need another way to accept diffs while checking, its all
> > > spelled out in the individual patches, please ack.
> 
> > > Arnaldo Carvalho de Melo (2):
> > >   tools check_headers.sh: Add check variant that excludes some hunks
> > >   perf tools: Cope with differences for lib/list_sort.c copy from the kernel
> 
> > LGTM. For the series:
>  
> > Acked-by: Kuan-Wei Chiu <visitorckw@gmail.com>
> 
> Adding it to that cset before pushing to the written in stone
> perf-tools/perf-tools branch.

Oh, I thought you wanted to have it in the perf-tools-next branch.
But it's ok to go through perf-tools.  I'll drop this patchset from
tmp.perf-tools-next.

Thanks,
Namhyung

>  
> > While reviewing the patches, I noticed that there was already a
> > difference between lib/list_sort.c and tools/lib/list_sort.c regarding
> > an additional #include <linux/bug.h>. This prompted me to investigate
> > the reason for this discrepancy. From what I can see, both files only
> > seem to require only three headers:
> 
> > #include <linux/compiler.h> /* for likely() macro */
> > #include <linux/export.h> /* for EXPORT_SYMBOL() macro */
> > #include <linux/list_sort.h> /* for list_sort() and linux/types.h */
>  
> > I'll check the git history and run build tests to confirm. If only
> > these headers are needed, I'll submit a cleanup patch.
> 
> tools/ is a sidecar or sorts for the kernel, that tries to add value to
> kernel developers while not getting in their way.
> 
> Sometimes things we discover while using more widely things that were
> designed for use in the kernel source may be of help to kernel
> developers, if this is one such case, great!
> 
> But IIRC that linux/bug.h discrepancy, without further checking, was
> something already somehow accepted via:
> 
> +check lib/list_sort.c                '-I "^#include <linux/bug.h>"'
> 
> in the cset that introduced the copy:
> 
>   92ec3cc94c2cb60d ("tools lib: Adopt list_sort() from the kernel sources")
> 
> - Arnaldo

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

[Namhyung Kim]

On Wed, Oct 02, 2024 at 02:54:53PM -0300, Arnaldo Carvalho de Melo wrote:
> 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@redhat.com>
> > > 
> > > 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@intel.com>
> > > Cc: Andrew Morton <akpm@linux-foundation.org>
> > > Cc: Ian Rogers <irogers@google.com>
> > > Cc: Jiri Olsa <jolsa@kernel.org>
> > > Cc: Kan Liang <kan.liang@linux.intel.com>
> > > Cc: Kuan-Wei Chiu <visitorckw@gmail.com>
> > > Cc: Namhyung Kim <namhyung@kernel.org>
> > > Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> > 
> > 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?

I meant it's good. :)  It'd be convenient if we have some changes that
need to be handled separately like this in the future.

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

No, I just said about possible future work and glad to have this.

> 
> > Good job!
> 
> Assuming all is right, thanks! 8-)

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