[PATCH 0/3] tools: add gettid to libc_compat.h

[PATCH 0/3] tools: add gettid to libc_compat.h

[Riccardo Mancini]

Hi Arnaldo,

following our previous discussion on workqueue RFC patchset, I'm sending
this patchet to add gettid to tools/include/tools/libc_compat.h.
This new definition will replace existing uses and will be used by the
workqueue code.

Thanks,
Riccardo

Riccardo Mancini (3):
  tools libc_compat: add gettid
  perf jvmti: use gettid from libc_compat
  perf test: mmap-thread-lookup: use gettid

 tools/include/tools/libc_compat.h     | 7 +++++++
 tools/perf/jvmti/jvmti_agent.c        | 9 +--------
 tools/perf/tests/mmap-thread-lookup.c | 4 ++--
 3 files changed, 10 insertions(+), 10 deletions(-)

-- 
2.31.1

[PATCH 1/3] tools libc_compat: add gettid

[Riccardo Mancini]

This patch adds gettid to libc_compat.h, since it was added in glibc
2.30 and is not available in previous versions.
The function is defined only if the HAVE_GETTID is not defined.

Signed-off-by: Riccardo Mancini <rickyman7@gmail.com>
---
 tools/include/tools/libc_compat.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/tools/include/tools/libc_compat.h b/tools/include/tools/libc_compat.h
index e907ba6f15e532b6..58762c9c49c22ef1 100644
--- a/tools/include/tools/libc_compat.h
+++ b/tools/include/tools/libc_compat.h
@@ -17,4 +17,11 @@ static inline void *reallocarray(void *ptr, size_t nmemb, size_t size)
 	return realloc(ptr, bytes);
 }
 #endif
+
+#ifndef HAVE_GETTID
+static inline pid_t gettid(void)
+{
+	return (pid_t)syscall(__NR_gettid);
+}
+#endif
 #endif
-- 
2.31.1

Re: [PATCH 1/3] tools libc_compat: add gettid

[Riccardo Mancini]

Hi Arnaldo,

as reported by the kernel test robot, I forgot to include sys/syscall.h and
unistd.h, which were included by some other headers in perf but is causing
troubles in the bpf selftest.
However, this is not all, since bpf tests are still failing since they do not
use HAVE_* macros, therefore gettid gets defined twice (in libc and
libc_compat.h).
Since, afaict, bpf is no longer using libc_compat.h [1] (perf is be the only
user atm), I would remove its dependency on libc_compat.h.
Furthermore, this made me also think whether the HAVE_* or NEED_* macros are
more suited for this case (reallocarray is using COMPAT_NEED_REALLOCARRAY,
gettid is using HAVE_GETTID). I believe HAVE_* are better since compilation
fails if they are missing or misused (as in this case), while NEED_* failures
may be more subtle (ie. only happening with older versions of libc). Therefore,
I think we should also change COMPAT_NEED_REALLOCARRAY to HAVE_REALLOCARRAY.
What do you think?

[1] https://lore.kernel.org/bpf/20200819013607.3607269-2-andriin@fb.com/

Thanks,
Riccardo

On Mon, 2021-07-26 at 06:04 +0800, kernel test robot wrote:
> Hi Riccardo,
> 
> Thank you for the patch! Yet something to improve:
> 
> [auto build test ERROR on tip/perf/core]
> [also build test ERROR on linux/master linus/master v5.14-rc2 next-20210723]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
> 
> url:   
> https://github.com/0day-ci/linux/commits/Riccardo-Mancini/tools-add-gettid-to-libc_compat-h/20210722-233601
> base:  
> https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git c76826a65f50038f0504
> 24365dbf3f97203f8710
> config: x86_64-rhel-8.3-kselftests (attached as .config)
> compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
> reproduce (this is a W=1 build):
>         #
> https://github.com/0day-ci/linux/commit/42df183984cce4c25932242bbf9133684e9425db
>         git remote add linux-review https://github.com/0day-ci/linux
>         git fetch --no-tags linux-review Riccardo-Mancini/tools-add-gettid-to-
> libc_compat-h/20210722-233601
>         git checkout 42df183984cce4c25932242bbf9133684e9425db
>         # save the attached .config to linux build tree
>         mkdir build_dir
>         make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash -C
> tools/testing/selftests/bpf install
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> 
> All errors (new ones prefixed by >>):
> 
>    In file included from main.h:15,
>                     from xlated_dumper.c:14:
>    tools/include/tools/libc_compat.h: In function 'gettid':
>    tools/include/tools/libc_compat.h:24:16: warning: implicit declaration of
> function 'syscall' [-Wimplicit-function-declaration]
>       24 |  return (pid_t)syscall(__NR_gettid);
>          |                ^~~~~~~
> > > tools/include/tools/libc_compat.h:24:24: error: '__NR_gettid' undeclared
> > > (first use in this function)
>       24 |  return (pid_t)syscall(__NR_gettid);
>          |                        ^~~~~~~~~~~
>    tools/include/tools/libc_compat.h:24:24: note: each undeclared identifier is
> reported only once for each function it appears in
> --
>    In file included from main.h:15,
>                     from common.c:27:
> > > tools/include/tools/libc_compat.h:22:21: error: static declaration of
> > > 'gettid' follows non-static declaration
>       22 | static inline pid_t gettid(void)
>          |                     ^~~~~~
>    In file included from /usr/include/unistd.h:1170,
>                     from common.c:15:
>    /usr/include/x86_64-linux-gnu/bits/unistd_ext.h:34:16: note: previous
> declaration of 'gettid' was here
>       34 | extern __pid_t gettid (void) __THROW;
>          |                ^~~~~~
>    In file included from main.h:15,
>                     from common.c:27:
>    tools/include/tools/libc_compat.h: In function 'gettid':
> > > tools/include/tools/libc_compat.h:24:24: error: '__NR_gettid' undeclared
> > > (first use in this function)
>       24 |  return (pid_t)syscall(__NR_gettid);
>          |                        ^~~~~~~~~~~
>    tools/include/tools/libc_compat.h:24:24: note: each undeclared identifier is
> reported only once for each function it appears in
> --
>    In file included from main.h:15,
>                     from btf.c:20:
>    tools/include/tools/libc_compat.h: In function 'gettid':
> > > tools/include/tools/libc_compat.h:24:24: error: '__NR_gettid' undeclared
> > > (first use in this function)
>       24 |  return (pid_t)syscall(__NR_gettid);
>          |                        ^~~~~~~~~~~
>    tools/include/tools/libc_compat.h:24:24: note: each undeclared identifier is
> reported only once for each function it appears in
> 
> 
> vim +/__NR_gettid +24 tools/include/tools/libc_compat.h
> 
>     20  
>     21  #ifndef HAVE_GETTID
>   > 22  static inline pid_t gettid(void)
>     23  {
>   > 24          return (pid_t)syscall(__NR_gettid);
> 
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[PATCH 2/3] perf jvmti: use gettid from libc_compat

[Riccardo Mancini]

This patch removes the re-definition of gettid in jvmti_agent.c, adding
the include of the libc_compat.h header, which now contains the gettid
function.

Signed-off-by: Riccardo Mancini <rickyman7@gmail.com>
---
 tools/perf/jvmti/jvmti_agent.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/tools/perf/jvmti/jvmti_agent.c b/tools/perf/jvmti/jvmti_agent.c
index 526dcaf9f07903dd..299e7e87198cc18d 100644
--- a/tools/perf/jvmti/jvmti_agent.c
+++ b/tools/perf/jvmti/jvmti_agent.c
@@ -33,7 +33,7 @@
 #include <unistd.h>
 #include <time.h>
 #include <sys/mman.h>
-#include <syscall.h> /* for gettid() */
+#include <tools/libc_compat.h> /* for gettid() */
 #include <err.h>
 #include <linux/kernel.h>
 
@@ -45,13 +45,6 @@
 static char jit_path[PATH_MAX];
 static void *marker_addr;
 
-#ifndef HAVE_GETTID
-static inline pid_t gettid(void)
-{
-	return (pid_t)syscall(__NR_gettid);
-}
-#endif
-
 static int get_e_machine(struct jitheader *hdr)
 {
 	ssize_t sret;
-- 
2.31.1

[PATCH 3/3] perf test: mmap-thread-lookup: use gettid

[Riccardo Mancini]

This patch replaces the manual syscall to get the current thread tid,
with the libc equivalent gettid.
Since it is only available from glibc 2.30, the libc_compat.h header is
included, which provides its definition in case gettid is not available.

Signed-off-by: Riccardo Mancini <rickyman7@gmail.com>
---
 tools/perf/tests/mmap-thread-lookup.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/tests/mmap-thread-lookup.c b/tools/perf/tests/mmap-thread-lookup.c
index 8d9d4cbff76d17d5..055cc850a6061798 100644
--- a/tools/perf/tests/mmap-thread-lookup.c
+++ b/tools/perf/tests/mmap-thread-lookup.c
@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <inttypes.h>
 #include <unistd.h>
-#include <sys/syscall.h>
+#include <tools/libc_compat.h> // gettid
 #include <sys/types.h>
 #include <sys/mman.h>
 #include <pthread.h>
@@ -45,7 +45,7 @@ static int thread_init(struct thread_data *td)
 	}
 
 	td->map = map;
-	td->tid = syscall(SYS_gettid);
+	td->tid = gettid();
 
 	pr_debug("tid = %d, map = %p\n", td->tid, map);
 	return 0;
-- 
2.31.1

Re: [PATCH 0/3] tools: add gettid to libc_compat.h

[Ian Rogers]

On Thu, Jul 22, 2021 at 8:34 AM Riccardo Mancini <rickyman7@gmail.com> wrote:
>
> Hi Arnaldo,
>
> following our previous discussion on workqueue RFC patchset, I'm sending
> this patchet to add gettid to tools/include/tools/libc_compat.h.
> This new definition will replace existing uses and will be used by the
> workqueue code.
>
> Thanks,
> Riccardo
>
> Riccardo Mancini (3):
>   tools libc_compat: add gettid
>   perf jvmti: use gettid from libc_compat
>   perf test: mmap-thread-lookup: use gettid
>
>  tools/include/tools/libc_compat.h     | 7 +++++++
>  tools/perf/jvmti/jvmti_agent.c        | 9 +--------
>  tools/perf/tests/mmap-thread-lookup.c | 4 ++--
>  3 files changed, 10 insertions(+), 10 deletions(-)

All 3 patches look like great cleanup, thanks!

Acked-by: Ian Rogers <irogers@google.com>

Thanks,
Ian