public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/3] perf tools: Fix test "perf probe of function from different CU"
@ 2024-02-20  4:29 Chaitanya S Prakash
  2024-02-20 10:16 ` James Clark
       [not found] ` <20240220042957.2022391-3-ChaitanyaS.Prakash@arm.com>
  0 siblings, 2 replies; 6+ messages in thread
From: Chaitanya S Prakash @ 2024-02-20  4:29 UTC (permalink / raw)
  To: linux-perf-users
  Cc: anshuman.khandual, Chaitanya S Prakash, Peter Zijlstra,
	Ingo Molnar, Arnaldo Carvalho de Melo, Namhyung Kim, Mark Rutland,
	Alexander Shishkin, Jiri Olsa, Ian Rogers, Adrian Hunter,
	James Clark, Georg Müller, Masami Hiramatsu, Liam Howlett,
	Athira Rajeev, linux-kernel

From: Chaitanya S Prakash <chaitanyas.prakash@arm.com>

Defconfig doesn't provide all the necessary configs required for the
test "perf probe of function from different CU" to run successfully on
all platforms. Therefore the required configs have been added to
config fragments to resolve this issue. On further investigation it was
seen that the Perf treated all files beginning with "/tmp/perf-" as a
map file despite them always ending in ".map", this caused the test to
fail when Perf was built with NO_DWARF=1. As the file was parsed as a
map file, the probe...--funcs command output garbage values instead of
listing the functions in the binary. After fixing the issue an
additional check to test the output of the probe...--funcs command has
been added.

This patch series has been tested on 6.8-rc3 mainline kernerl, both on
arm64 and x86 platforms.

Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Ian Rogers <irogers@google.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: James Clark <james.clark@arm.com>
Cc: Georg Müller <georgmueller@gmx.net>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Liam Howlett <liam.howlett@oracle.com>
Cc: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
Cc: linux-perf-users@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

Chaitanya S Prakash (3):
  perf tools: Enable configs required for
    test_uprobe_from_different_cu.sh
  perf tools: Only treat files as map files when they have the extension
    .map
  perf test: Check output of the probe ... --funcs command

 tools/perf/builtin-script.c                    | 15 +--------------
 tools/perf/tests/config-fragments/config       |  3 +++
 .../shell/test_uprobe_from_different_cu.sh     |  2 +-
 tools/perf/util/string.c                       | 18 ++++++++++++++++++
 tools/perf/util/string2.h                      |  1 +
 tools/perf/util/symbol.c                       |  5 ++++-
 6 files changed, 28 insertions(+), 16 deletions(-)

-- 
2.34.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 0/3] perf tools: Fix test "perf probe of function from different CU"
  2024-02-20  4:29 [PATCH 0/3] perf tools: Fix test "perf probe of function from different CU" Chaitanya S Prakash
@ 2024-02-20 10:16 ` James Clark
       [not found] ` <20240220042957.2022391-3-ChaitanyaS.Prakash@arm.com>
  1 sibling, 0 replies; 6+ messages in thread
From: James Clark @ 2024-02-20 10:16 UTC (permalink / raw)
  To: Chaitanya S Prakash, linux-perf-users
  Cc: anshuman.khandual, Peter Zijlstra, Ingo Molnar,
	Arnaldo Carvalho de Melo, Namhyung Kim, Mark Rutland,
	Alexander Shishkin, Jiri Olsa, Ian Rogers, Adrian Hunter,
	Georg Müller, Masami Hiramatsu, Liam Howlett, Athira Rajeev,
	linux-kernel



On 20/02/2024 04:29, Chaitanya S Prakash wrote:
> From: Chaitanya S Prakash <chaitanyas.prakash@arm.com>
> 
> Defconfig doesn't provide all the necessary configs required for the
> test "perf probe of function from different CU" to run successfully on
> all platforms. Therefore the required configs have been added to
> config fragments to resolve this issue. On further investigation it was
> seen that the Perf treated all files beginning with "/tmp/perf-" as a
> map file despite them always ending in ".map", this caused the test to
> fail when Perf was built with NO_DWARF=1. As the file was parsed as a
> map file, the probe...--funcs command output garbage values instead of
> listing the functions in the binary. After fixing the issue an
> additional check to test the output of the probe...--funcs command has
> been added.
> 
> This patch series has been tested on 6.8-rc3 mainline kernerl, both on
> arm64 and x86 platforms.
> 
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: Ian Rogers <irogers@google.com>
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: James Clark <james.clark@arm.com>
> Cc: Georg Müller <georgmueller@gmx.net>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: Liam Howlett <liam.howlett@oracle.com>
> Cc: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
> Cc: linux-perf-users@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> 
> Chaitanya S Prakash (3):
>   perf tools: Enable configs required for
>     test_uprobe_from_different_cu.sh
>   perf tools: Only treat files as map files when they have the extension
>     .map
>   perf test: Check output of the probe ... --funcs command
> 
>  tools/perf/builtin-script.c                    | 15 +--------------
>  tools/perf/tests/config-fragments/config       |  3 +++
>  .../shell/test_uprobe_from_different_cu.sh     |  2 +-
>  tools/perf/util/string.c                       | 18 ++++++++++++++++++
>  tools/perf/util/string2.h                      |  1 +
>  tools/perf/util/symbol.c                       |  5 ++++-
>  6 files changed, 28 insertions(+), 16 deletions(-)
> 

Reviewed-by: James Clark <james.clark@arm.com>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/3] perf tools: Only treat files as map files when they have the extension .map
       [not found]   ` <CAP-5=fUFmeoTjLuZTgcaV23iGQU1AdddG+7Rw=d6buMU007+1Q@mail.gmail.com>
@ 2024-02-21 14:58     ` Arnaldo Carvalho de Melo
  2024-02-23 12:10       ` Chaitanya S Prakash
  0 siblings, 1 reply; 6+ messages in thread
From: Arnaldo Carvalho de Melo @ 2024-02-21 14:58 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Chaitanya S Prakash, linux-perf-users, anshuman.khandual,
	Linux Kernel Mailing List

On Tue, Feb 20, 2024 at 06:40:47AM -0800, Ian Rogers wrote:
> On Mon, Feb 19, 2024 at 8:30 PM Chaitanya S Prakash <ChaitanyaS.Prakash@arm.com> wrote:
> > +++ b/tools/perf/util/string2.h
> > @@ -40,5 +40,6 @@ char *strdup_esc(const char *str);
> >
> >  unsigned int hex(char c);
> >  char *strreplace_chars(char needle, const char *haystack, const char *replace);
> > +const char *ends_with(const char *str, const char *suffix);
> 
> nit: string2.h is an extension of linux's string.h. The tools copy of
> that is missing functions in the kernel version:
> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/include/linux/string.h?h=perf-tools-next
> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/include/linux/string.h?h=perf-tools-next#n398
> specifically str_has_prefix.
> 
> The naming ends_with makes sense but there is also strstarts and
> str_has_prefix, perhaps str_has_suffix would be the most consistent
> and intention revealing name?
> 
> Also, we have strtailcmp which behaves like a reverse strcmp that
> doesn't compare the lengths of the strings. It seems all uses of
> strtailcmp are just for a "str_has_suffix". It would make sense to me
> to remove that function and switch to a common str_has_suffix function
> which I think is a more intention revealing way of naming what the
> code is doing.

So far in perf we try to just reuse whatever function the kernel has for
the purpose at hand, right now the kernel has:

/**
 * strstarts - does @str start with @prefix?
 * @str: string to examine
 * @prefix: prefix to look for.
 */
static inline bool strstarts(const char *str, const char *prefix)
{
        return strncmp(str, prefix, strlen(prefix)) == 0;
}

And:

/**
 * str_has_prefix - Test if a string has a given prefix
 * @str: The string to test
 * @prefix: The string to see if @str starts with
 *
 * A common way to test a prefix of a string is to do:
 *  strncmp(str, prefix, sizeof(prefix) - 1)
 *
 * But this can lead to bugs due to typos, or if prefix is a pointer
 * and not a constant. Instead use str_has_prefix().
 *
 * Returns:
 * * strlen(@prefix) if @str starts with @prefix
 * * 0 if @str does not start with @prefix
 */
static __always_inline size_t str_has_prefix(const char *str, const char *prefix)
{
        size_t len = strlen(prefix);
        return strncmp(str, prefix, len) == 0 ? len : 0;
}

The later seems to give more bang for the buck, so to say, returning the
prefix lenght.

It is a new addition:

72921427d46bf9731 (Steven Rostedt (VMware) 2018-12-21 18:10:14 -0500 398) static __always_inline size_t str_has_prefix(const char *str, const char *prefix)

While:

66f92cf9d415e96a5 (Rusty Russell           2009-03-31 13:05:36 -0600 249)  * strstarts - does @str start with @prefix?

⬢[acme@toolbox linux]$ git grep str_has_prefix| wc -l
94
⬢[acme@toolbox linux]$ git grep strstarts| wc -l
177
⬢[acme@toolbox linux]$ 

Some places use it:

kernel/printk/printk.c: len = str_has_prefix(str, "on");
kernel/printk/printk.c: len = str_has_prefix(str, "off");
kernel/printk/printk.c: len = str_has_prefix(str, "ratelimit");


static int __control_devkmsg(char *str)
{
	size_t len;

	if (!str)
		return -EINVAL;

	len = str_has_prefix(str, "on");
	if (len) {
		devkmsg_log = DEVKMSG_LOG_MASK_ON;
		return len;
	}

	len = str_has_prefix(str, "off");
	if (len) {
		devkmsg_log = DEVKMSG_LOG_MASK_OFF;
		return len;
	}

	len = str_has_prefix(str, "ratelimit");
	if (len) {
		devkmsg_log = DEVKMSG_LOG_MASK_DEFAULT;
		return len;
	}

	return -EINVAL;
}


                err = __control_devkmsg(devkmsg_log_str);
 
                /*
                 * Do not accept an unknown string OR a known string with
                 * trailing crap...
                 */
                if (err < 0 || (err + 1 != *lenp)) {

                        /* ... and restore old setting. */
                        devkmsg_log = old;
                        strncpy(devkmsg_log_str, old_str, DEVKMSG_STR_MAX_SIZE);

                        return -EINVAL;
                }


So yeah, I agree with Ian that it is more intention revealing, has this
bonus of returning the strlen for the above use cases, is in the kernel
sources, so I'm in favour of grabbing a copy of it and replacing the
strstarts() usage with it, drop strstarts(), then also introduce
str_has_suffix(), the kernel will get it when it needs, possibly from
tools/lib/ :-)

- Arnaldo

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/3] perf tools: Only treat files as map files when they have the extension .map
  2024-02-21 14:58     ` [PATCH 2/3] perf tools: Only treat files as map files when they have the extension .map Arnaldo Carvalho de Melo
@ 2024-02-23 12:10       ` Chaitanya S Prakash
  2024-03-15 20:34         ` Arnaldo Carvalho de Melo
  0 siblings, 1 reply; 6+ messages in thread
From: Chaitanya S Prakash @ 2024-02-23 12:10 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Ian Rogers
  Cc: linux-perf-users, anshuman.khandual, Linux Kernel Mailing List

I'll make the changes, thanks for the review.

On 2/21/24 20:28, Arnaldo Carvalho de Melo wrote:
> On Tue, Feb 20, 2024 at 06:40:47AM -0800, Ian Rogers wrote:
>> On Mon, Feb 19, 2024 at 8:30 PM Chaitanya S Prakash <ChaitanyaS.Prakash@arm.com> wrote:
>>> +++ b/tools/perf/util/string2.h
>>> @@ -40,5 +40,6 @@ char *strdup_esc(const char *str);
>>>
>>>   unsigned int hex(char c);
>>>   char *strreplace_chars(char needle, const char *haystack, const char *replace);
>>> +const char *ends_with(const char *str, const char *suffix);
>> nit: string2.h is an extension of linux's string.h. The tools copy of
>> that is missing functions in the kernel version:
>> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/include/linux/string.h?h=perf-tools-next
>> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/include/linux/string.h?h=perf-tools-next#n398
>> specifically str_has_prefix.
>>
>> The naming ends_with makes sense but there is also strstarts and
>> str_has_prefix, perhaps str_has_suffix would be the most consistent
>> and intention revealing name?
>>
>> Also, we have strtailcmp which behaves like a reverse strcmp that
>> doesn't compare the lengths of the strings. It seems all uses of
>> strtailcmp are just for a "str_has_suffix". It would make sense to me
>> to remove that function and switch to a common str_has_suffix function
>> which I think is a more intention revealing way of naming what the
>> code is doing.
> So far in perf we try to just reuse whatever function the kernel has for
> the purpose at hand, right now the kernel has:
>
> /**
>   * strstarts - does @str start with @prefix?
>   * @str: string to examine
>   * @prefix: prefix to look for.
>   */
> static inline bool strstarts(const char *str, const char *prefix)
> {
>          return strncmp(str, prefix, strlen(prefix)) == 0;
> }
>
> And:
>
> /**
>   * str_has_prefix - Test if a string has a given prefix
>   * @str: The string to test
>   * @prefix: The string to see if @str starts with
>   *
>   * A common way to test a prefix of a string is to do:
>   *  strncmp(str, prefix, sizeof(prefix) - 1)
>   *
>   * But this can lead to bugs due to typos, or if prefix is a pointer
>   * and not a constant. Instead use str_has_prefix().
>   *
>   * Returns:
>   * * strlen(@prefix) if @str starts with @prefix
>   * * 0 if @str does not start with @prefix
>   */
> static __always_inline size_t str_has_prefix(const char *str, const char *prefix)
> {
>          size_t len = strlen(prefix);
>          return strncmp(str, prefix, len) == 0 ? len : 0;
> }
>
> The later seems to give more bang for the buck, so to say, returning the
> prefix lenght.
>
> It is a new addition:
>
> 72921427d46bf9731 (Steven Rostedt (VMware) 2018-12-21 18:10:14 -0500 398) static __always_inline size_t str_has_prefix(const char *str, const char *prefix)
>
> While:
>
> 66f92cf9d415e96a5 (Rusty Russell           2009-03-31 13:05:36 -0600 249)  * strstarts - does @str start with @prefix?
>
> ⬢[acme@toolbox linux]$ git grep str_has_prefix| wc -l
> 94
> ⬢[acme@toolbox linux]$ git grep strstarts| wc -l
> 177
> ⬢[acme@toolbox linux]$
>
> Some places use it:
>
> kernel/printk/printk.c: len = str_has_prefix(str, "on");
> kernel/printk/printk.c: len = str_has_prefix(str, "off");
> kernel/printk/printk.c: len = str_has_prefix(str, "ratelimit");
>
>
> static int __control_devkmsg(char *str)
> {
> 	size_t len;
>
> 	if (!str)
> 		return -EINVAL;
>
> 	len = str_has_prefix(str, "on");
> 	if (len) {
> 		devkmsg_log = DEVKMSG_LOG_MASK_ON;
> 		return len;
> 	}
>
> 	len = str_has_prefix(str, "off");
> 	if (len) {
> 		devkmsg_log = DEVKMSG_LOG_MASK_OFF;
> 		return len;
> 	}
>
> 	len = str_has_prefix(str, "ratelimit");
> 	if (len) {
> 		devkmsg_log = DEVKMSG_LOG_MASK_DEFAULT;
> 		return len;
> 	}
>
> 	return -EINVAL;
> }
>
>
>                  err = __control_devkmsg(devkmsg_log_str);
>   
>                  /*
>                   * Do not accept an unknown string OR a known string with
>                   * trailing crap...
>                   */
>                  if (err < 0 || (err + 1 != *lenp)) {
>
>                          /* ... and restore old setting. */
>                          devkmsg_log = old;
>                          strncpy(devkmsg_log_str, old_str, DEVKMSG_STR_MAX_SIZE);
>
>                          return -EINVAL;
>                  }
>
>
> So yeah, I agree with Ian that it is more intention revealing, has this
> bonus of returning the strlen for the above use cases, is in the kernel
> sources, so I'm in favour of grabbing a copy of it and replacing the
> strstarts() usage with it, drop strstarts(), then also introduce
> str_has_suffix(), the kernel will get it when it needs, possibly from
> tools/lib/ :-)
>
> - Arnaldo

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/3] perf tools: Only treat files as map files when they have the extension .map
  2024-02-23 12:10       ` Chaitanya S Prakash
@ 2024-03-15 20:34         ` Arnaldo Carvalho de Melo
  2024-03-19  2:26           ` Chaitanya S Prakash
  0 siblings, 1 reply; 6+ messages in thread
From: Arnaldo Carvalho de Melo @ 2024-03-15 20:34 UTC (permalink / raw)
  To: Chaitanya S Prakash
  Cc: Ian Rogers, linux-perf-users, anshuman.khandual,
	Linux Kernel Mailing List

On Fri, Feb 23, 2024 at 05:40:02PM +0530, Chaitanya S Prakash wrote:
> I'll make the changes, thanks for the review.

Have you submitted a new series?

Thanks,

- Arnaldo
 
> On 2/21/24 20:28, Arnaldo Carvalho de Melo wrote:
> > On Tue, Feb 20, 2024 at 06:40:47AM -0800, Ian Rogers wrote:
> > > On Mon, Feb 19, 2024 at 8:30 PM Chaitanya S Prakash <ChaitanyaS.Prakash@arm.com> wrote:
> > > > +++ b/tools/perf/util/string2.h
> > > > @@ -40,5 +40,6 @@ char *strdup_esc(const char *str);
> > > > 
> > > >   unsigned int hex(char c);
> > > >   char *strreplace_chars(char needle, const char *haystack, const char *replace);
> > > > +const char *ends_with(const char *str, const char *suffix);
> > > nit: string2.h is an extension of linux's string.h. The tools copy of
> > > that is missing functions in the kernel version:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/include/linux/string.h?h=perf-tools-next
> > > https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/include/linux/string.h?h=perf-tools-next#n398
> > > specifically str_has_prefix.
> > > 
> > > The naming ends_with makes sense but there is also strstarts and
> > > str_has_prefix, perhaps str_has_suffix would be the most consistent
> > > and intention revealing name?
> > > 
> > > Also, we have strtailcmp which behaves like a reverse strcmp that
> > > doesn't compare the lengths of the strings. It seems all uses of
> > > strtailcmp are just for a "str_has_suffix". It would make sense to me
> > > to remove that function and switch to a common str_has_suffix function
> > > which I think is a more intention revealing way of naming what the
> > > code is doing.
> > So far in perf we try to just reuse whatever function the kernel has for
> > the purpose at hand, right now the kernel has:
> > 
> > /**
> >   * strstarts - does @str start with @prefix?
> >   * @str: string to examine
> >   * @prefix: prefix to look for.
> >   */
> > static inline bool strstarts(const char *str, const char *prefix)
> > {
> >          return strncmp(str, prefix, strlen(prefix)) == 0;
> > }
> > 
> > And:
> > 
> > /**
> >   * str_has_prefix - Test if a string has a given prefix
> >   * @str: The string to test
> >   * @prefix: The string to see if @str starts with
> >   *
> >   * A common way to test a prefix of a string is to do:
> >   *  strncmp(str, prefix, sizeof(prefix) - 1)
> >   *
> >   * But this can lead to bugs due to typos, or if prefix is a pointer
> >   * and not a constant. Instead use str_has_prefix().
> >   *
> >   * Returns:
> >   * * strlen(@prefix) if @str starts with @prefix
> >   * * 0 if @str does not start with @prefix
> >   */
> > static __always_inline size_t str_has_prefix(const char *str, const char *prefix)
> > {
> >          size_t len = strlen(prefix);
> >          return strncmp(str, prefix, len) == 0 ? len : 0;
> > }
> > 
> > The later seems to give more bang for the buck, so to say, returning the
> > prefix lenght.
> > 
> > It is a new addition:
> > 
> > 72921427d46bf9731 (Steven Rostedt (VMware) 2018-12-21 18:10:14 -0500 398) static __always_inline size_t str_has_prefix(const char *str, const char *prefix)
> > 
> > While:
> > 
> > 66f92cf9d415e96a5 (Rusty Russell           2009-03-31 13:05:36 -0600 249)  * strstarts - does @str start with @prefix?
> > 
> > ⬢[acme@toolbox linux]$ git grep str_has_prefix| wc -l
> > 94
> > ⬢[acme@toolbox linux]$ git grep strstarts| wc -l
> > 177
> > ⬢[acme@toolbox linux]$
> > 
> > Some places use it:
> > 
> > kernel/printk/printk.c: len = str_has_prefix(str, "on");
> > kernel/printk/printk.c: len = str_has_prefix(str, "off");
> > kernel/printk/printk.c: len = str_has_prefix(str, "ratelimit");
> > 
> > 
> > static int __control_devkmsg(char *str)
> > {
> > 	size_t len;
> > 
> > 	if (!str)
> > 		return -EINVAL;
> > 
> > 	len = str_has_prefix(str, "on");
> > 	if (len) {
> > 		devkmsg_log = DEVKMSG_LOG_MASK_ON;
> > 		return len;
> > 	}
> > 
> > 	len = str_has_prefix(str, "off");
> > 	if (len) {
> > 		devkmsg_log = DEVKMSG_LOG_MASK_OFF;
> > 		return len;
> > 	}
> > 
> > 	len = str_has_prefix(str, "ratelimit");
> > 	if (len) {
> > 		devkmsg_log = DEVKMSG_LOG_MASK_DEFAULT;
> > 		return len;
> > 	}
> > 
> > 	return -EINVAL;
> > }
> > 
> > 
> >                  err = __control_devkmsg(devkmsg_log_str);
> >                  /*
> >                   * Do not accept an unknown string OR a known string with
> >                   * trailing crap...
> >                   */
> >                  if (err < 0 || (err + 1 != *lenp)) {
> > 
> >                          /* ... and restore old setting. */
> >                          devkmsg_log = old;
> >                          strncpy(devkmsg_log_str, old_str, DEVKMSG_STR_MAX_SIZE);
> > 
> >                          return -EINVAL;
> >                  }
> > 
> > 
> > So yeah, I agree with Ian that it is more intention revealing, has this
> > bonus of returning the strlen for the above use cases, is in the kernel
> > sources, so I'm in favour of grabbing a copy of it and replacing the
> > strstarts() usage with it, drop strstarts(), then also introduce
> > str_has_suffix(), the kernel will get it when it needs, possibly from
> > tools/lib/ :-)
> > 
> > - Arnaldo

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/3] perf tools: Only treat files as map files when they have the extension .map
  2024-03-15 20:34         ` Arnaldo Carvalho de Melo
@ 2024-03-19  2:26           ` Chaitanya S Prakash
  0 siblings, 0 replies; 6+ messages in thread
From: Chaitanya S Prakash @ 2024-03-19  2:26 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Ian Rogers, linux-perf-users, anshuman.khandual,
	Linux Kernel Mailing List

On 3/16/24 02:04, Arnaldo Carvalho de Melo wrote:
> On Fri, Feb 23, 2024 at 05:40:02PM +0530, Chaitanya S Prakash wrote:
>> I'll make the changes, thanks for the review.
> Have you submitted a new series?
>
> Thanks,
>
> - Arnaldo
I will be posting it this week, extremely sorry for the delay!
>   
>> On 2/21/24 20:28, Arnaldo Carvalho de Melo wrote:
>>> On Tue, Feb 20, 2024 at 06:40:47AM -0800, Ian Rogers wrote:
>>>> On Mon, Feb 19, 2024 at 8:30 PM Chaitanya S Prakash <ChaitanyaS.Prakash@arm.com> wrote:
>>>>> +++ b/tools/perf/util/string2.h
>>>>> @@ -40,5 +40,6 @@ char *strdup_esc(const char *str);
>>>>>
>>>>>    unsigned int hex(char c);
>>>>>    char *strreplace_chars(char needle, const char *haystack, const char *replace);
>>>>> +const char *ends_with(const char *str, const char *suffix);
>>>> nit: string2.h is an extension of linux's string.h. The tools copy of
>>>> that is missing functions in the kernel version:
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/include/linux/string.h?h=perf-tools-next
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/include/linux/string.h?h=perf-tools-next#n398
>>>> specifically str_has_prefix.
>>>>
>>>> The naming ends_with makes sense but there is also strstarts and
>>>> str_has_prefix, perhaps str_has_suffix would be the most consistent
>>>> and intention revealing name?
>>>>
>>>> Also, we have strtailcmp which behaves like a reverse strcmp that
>>>> doesn't compare the lengths of the strings. It seems all uses of
>>>> strtailcmp are just for a "str_has_suffix". It would make sense to me
>>>> to remove that function and switch to a common str_has_suffix function
>>>> which I think is a more intention revealing way of naming what the
>>>> code is doing.
>>> So far in perf we try to just reuse whatever function the kernel has for
>>> the purpose at hand, right now the kernel has:
>>>
>>> /**
>>>    * strstarts - does @str start with @prefix?
>>>    * @str: string to examine
>>>    * @prefix: prefix to look for.
>>>    */
>>> static inline bool strstarts(const char *str, const char *prefix)
>>> {
>>>           return strncmp(str, prefix, strlen(prefix)) == 0;
>>> }
>>>
>>> And:
>>>
>>> /**
>>>    * str_has_prefix - Test if a string has a given prefix
>>>    * @str: The string to test
>>>    * @prefix: The string to see if @str starts with
>>>    *
>>>    * A common way to test a prefix of a string is to do:
>>>    *  strncmp(str, prefix, sizeof(prefix) - 1)
>>>    *
>>>    * But this can lead to bugs due to typos, or if prefix is a pointer
>>>    * and not a constant. Instead use str_has_prefix().
>>>    *
>>>    * Returns:
>>>    * * strlen(@prefix) if @str starts with @prefix
>>>    * * 0 if @str does not start with @prefix
>>>    */
>>> static __always_inline size_t str_has_prefix(const char *str, const char *prefix)
>>> {
>>>           size_t len = strlen(prefix);
>>>           return strncmp(str, prefix, len) == 0 ? len : 0;
>>> }
>>>
>>> The later seems to give more bang for the buck, so to say, returning the
>>> prefix lenght.
>>>
>>> It is a new addition:
>>>
>>> 72921427d46bf9731 (Steven Rostedt (VMware) 2018-12-21 18:10:14 -0500 398) static __always_inline size_t str_has_prefix(const char *str, const char *prefix)
>>>
>>> While:
>>>
>>> 66f92cf9d415e96a5 (Rusty Russell           2009-03-31 13:05:36 -0600 249)  * strstarts - does @str start with @prefix?
>>>
>>> ⬢[acme@toolbox linux]$ git grep str_has_prefix| wc -l
>>> 94
>>> ⬢[acme@toolbox linux]$ git grep strstarts| wc -l
>>> 177
>>> ⬢[acme@toolbox linux]$
>>>
>>> Some places use it:
>>>
>>> kernel/printk/printk.c: len = str_has_prefix(str, "on");
>>> kernel/printk/printk.c: len = str_has_prefix(str, "off");
>>> kernel/printk/printk.c: len = str_has_prefix(str, "ratelimit");
>>>
>>>
>>> static int __control_devkmsg(char *str)
>>> {
>>> 	size_t len;
>>>
>>> 	if (!str)
>>> 		return -EINVAL;
>>>
>>> 	len = str_has_prefix(str, "on");
>>> 	if (len) {
>>> 		devkmsg_log = DEVKMSG_LOG_MASK_ON;
>>> 		return len;
>>> 	}
>>>
>>> 	len = str_has_prefix(str, "off");
>>> 	if (len) {
>>> 		devkmsg_log = DEVKMSG_LOG_MASK_OFF;
>>> 		return len;
>>> 	}
>>>
>>> 	len = str_has_prefix(str, "ratelimit");
>>> 	if (len) {
>>> 		devkmsg_log = DEVKMSG_LOG_MASK_DEFAULT;
>>> 		return len;
>>> 	}
>>>
>>> 	return -EINVAL;
>>> }
>>>
>>>
>>>                   err = __control_devkmsg(devkmsg_log_str);
>>>                   /*
>>>                    * Do not accept an unknown string OR a known string with
>>>                    * trailing crap...
>>>                    */
>>>                   if (err < 0 || (err + 1 != *lenp)) {
>>>
>>>                           /* ... and restore old setting. */
>>>                           devkmsg_log = old;
>>>                           strncpy(devkmsg_log_str, old_str, DEVKMSG_STR_MAX_SIZE);
>>>
>>>                           return -EINVAL;
>>>                   }
>>>
>>>
>>> So yeah, I agree with Ian that it is more intention revealing, has this
>>> bonus of returning the strlen for the above use cases, is in the kernel
>>> sources, so I'm in favour of grabbing a copy of it and replacing the
>>> strstarts() usage with it, drop strstarts(), then also introduce
>>> str_has_suffix(), the kernel will get it when it needs, possibly from
>>> tools/lib/ :-)
>>>
>>> - Arnaldo

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2024-03-19  2:26 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-02-20  4:29 [PATCH 0/3] perf tools: Fix test "perf probe of function from different CU" Chaitanya S Prakash
2024-02-20 10:16 ` James Clark
     [not found] ` <20240220042957.2022391-3-ChaitanyaS.Prakash@arm.com>
     [not found]   ` <CAP-5=fUFmeoTjLuZTgcaV23iGQU1AdddG+7Rw=d6buMU007+1Q@mail.gmail.com>
2024-02-21 14:58     ` [PATCH 2/3] perf tools: Only treat files as map files when they have the extension .map Arnaldo Carvalho de Melo
2024-02-23 12:10       ` Chaitanya S Prakash
2024-03-15 20:34         ` Arnaldo Carvalho de Melo
2024-03-19  2:26           ` Chaitanya S Prakash

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox