[PATCH] perf: Fix "Track with sched_switch" test by not printing warnings in quiet mode

[PATCH] perf: Fix "Track with sched_switch" test by not printing warnings in quiet mode

[James Clark]

Especially when CONFIG_LOCKDEP and other debug configs are enabled,
Perf can print the following warning when running the "Track with
sched_switch" test:

  Warning:
  Processed 1378918 events and lost 4 chunks!

  Check IO/CPU overload!

  Warning:
  Processed 4593325 samples and lost 70.00%!

The test already supplies -q to run in quiet mode, so extend quiet mode
to perf_stdio__warning() and also ui__warning() for consistency.

This fixes the following failure due to the extra lines counted:

  perf test "lock cont" -vvv

  82: kernel lock contention analysis test                            :
  --- start ---
  test child forked, pid 3125
  Testing perf lock record and perf lock contention
  [Fail] Recorded result count is not 1: 9
  test child finished with -1
  ---- end ----
  kernel lock contention analysis test: FAILED!

Fixes: ec685de25b67 ("perf test: Add kernel lock contention test")
Cc: Namhyung Kim <namhyung@kernel.org>
Signed-off-by: James Clark <james.clark@arm.com>
---
 tools/perf/ui/util.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tools/perf/ui/util.c b/tools/perf/ui/util.c
index 689b27c34246..1d38ddf01b60 100644
--- a/tools/perf/ui/util.c
+++ b/tools/perf/ui/util.c
@@ -15,6 +15,9 @@ static int perf_stdio__error(const char *format, va_list args)
 
 static int perf_stdio__warning(const char *format, va_list args)
 {
+	if (quiet)
+		return 0;
+
 	fprintf(stderr, "Warning:\n");
 	vfprintf(stderr, format, args);
 	return 0;
@@ -45,6 +48,8 @@ int ui__warning(const char *format, ...)
 {
 	int ret;
 	va_list args;
+	if (quiet)
+		return 0;
 
 	va_start(args, format);
 	ret = perf_eops->warning(format, args);
-- 
2.28.0

Re: [PATCH] perf: Fix "Track with sched_switch" test by not printing warnings in quiet mode

[James Clark]

On 12/10/2022 12:10, James Clark wrote:
> Especially when CONFIG_LOCKDEP and other debug configs are enabled,
> Perf can print the following warning when running the "Track with
> sched_switch" test:

Oops got the wrong test name here and in the title. Should be "kernel
lock contention analysis test"

> 
>   Warning:
>   Processed 1378918 events and lost 4 chunks!
> 
>   Check IO/CPU overload!
> 
>   Warning:
>   Processed 4593325 samples and lost 70.00%!
> 
> The test already supplies -q to run in quiet mode, so extend quiet mode
> to perf_stdio__warning() and also ui__warning() for consistency.
> 
> This fixes the following failure due to the extra lines counted:
> 
>   perf test "lock cont" -vvv
> 
>   82: kernel lock contention analysis test                            :
>   --- start ---
>   test child forked, pid 3125
>   Testing perf lock record and perf lock contention
>   [Fail] Recorded result count is not 1: 9
>   test child finished with -1
>   ---- end ----
>   kernel lock contention analysis test: FAILED!
> 
> Fixes: ec685de25b67 ("perf test: Add kernel lock contention test")
> Cc: Namhyung Kim <namhyung@kernel.org>
> Signed-off-by: James Clark <james.clark@arm.com>
> ---
>  tools/perf/ui/util.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/tools/perf/ui/util.c b/tools/perf/ui/util.c
> index 689b27c34246..1d38ddf01b60 100644
> --- a/tools/perf/ui/util.c
> +++ b/tools/perf/ui/util.c
> @@ -15,6 +15,9 @@ static int perf_stdio__error(const char *format, va_list args)
>  
>  static int perf_stdio__warning(const char *format, va_list args)
>  {
> +	if (quiet)
> +		return 0;
> +
>  	fprintf(stderr, "Warning:\n");
>  	vfprintf(stderr, format, args);
>  	return 0;
> @@ -45,6 +48,8 @@ int ui__warning(const char *format, ...)
>  {
>  	int ret;
>  	va_list args;
> +	if (quiet)
> +		return 0;
>  
>  	va_start(args, format);
>  	ret = perf_eops->warning(format, args);

Re: [PATCH] perf: Fix "Track with sched_switch" test by not printing warnings in quiet mode

[Namhyung Kim]

On Wed, Oct 12, 2022 at 4:13 AM James Clark <james.clark@arm.com> wrote:
>
>
>
> On 12/10/2022 12:10, James Clark wrote:
> > Especially when CONFIG_LOCKDEP and other debug configs are enabled,
> > Perf can print the following warning when running the "Track with
> > sched_switch" test:
>
> Oops got the wrong test name here and in the title. Should be "kernel
> lock contention analysis test"

Could you please resend?

>
> >
> >   Warning:
> >   Processed 1378918 events and lost 4 chunks!
> >
> >   Check IO/CPU overload!
> >
> >   Warning:
> >   Processed 4593325 samples and lost 70.00%!
> >
> > The test already supplies -q to run in quiet mode, so extend quiet mode
> > to perf_stdio__warning() and also ui__warning() for consistency.

I'm not sure if suppressing the warnings with -q is a good thing.
Maybe we need to separate warning/debug messages from the output.

Thanks,
Namhyung


> >
> > This fixes the following failure due to the extra lines counted:
> >
> >   perf test "lock cont" -vvv
> >
> >   82: kernel lock contention analysis test                            :
> >   --- start ---
> >   test child forked, pid 3125
> >   Testing perf lock record and perf lock contention
> >   [Fail] Recorded result count is not 1: 9
> >   test child finished with -1
> >   ---- end ----
> >   kernel lock contention analysis test: FAILED!
> >
> > Fixes: ec685de25b67 ("perf test: Add kernel lock contention test")
> > Cc: Namhyung Kim <namhyung@kernel.org>
> > Signed-off-by: James Clark <james.clark@arm.com>
> > ---
> >  tools/perf/ui/util.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/tools/perf/ui/util.c b/tools/perf/ui/util.c
> > index 689b27c34246..1d38ddf01b60 100644
> > --- a/tools/perf/ui/util.c
> > +++ b/tools/perf/ui/util.c
> > @@ -15,6 +15,9 @@ static int perf_stdio__error(const char *format, va_list args)
> >
> >  static int perf_stdio__warning(const char *format, va_list args)
> >  {
> > +     if (quiet)
> > +             return 0;
> > +
> >       fprintf(stderr, "Warning:\n");
> >       vfprintf(stderr, format, args);
> >       return 0;
> > @@ -45,6 +48,8 @@ int ui__warning(const char *format, ...)
> >  {
> >       int ret;
> >       va_list args;
> > +     if (quiet)
> > +             return 0;
> >
> >       va_start(args, format);
> >       ret = perf_eops->warning(format, args);

Re: [PATCH] perf: Fix "Track with sched_switch" test by not printing warnings in quiet mode

[James Clark]

On 12/10/2022 17:50, Namhyung Kim wrote:
> On Wed, Oct 12, 2022 at 4:13 AM James Clark <james.clark@arm.com> wrote:
>>
>>
>>
>> On 12/10/2022 12:10, James Clark wrote:
>>> Especially when CONFIG_LOCKDEP and other debug configs are enabled,
>>> Perf can print the following warning when running the "Track with
>>> sched_switch" test:
>>
>> Oops got the wrong test name here and in the title. Should be "kernel
>> lock contention analysis test"
> 
> Could you please resend?
> 
>>
>>>
>>>   Warning:
>>>   Processed 1378918 events and lost 4 chunks!
>>>
>>>   Check IO/CPU overload!
>>>
>>>   Warning:
>>>   Processed 4593325 samples and lost 70.00%!
>>>
>>> The test already supplies -q to run in quiet mode, so extend quiet mode
>>> to perf_stdio__warning() and also ui__warning() for consistency.
> 
> I'm not sure if suppressing the warnings with -q is a good thing.
> Maybe we need to separate warning/debug messages from the output.

I don't see the issue with warnings being suppressed in quiet mode as
long as errors are still printed. In other cases warnings have already
been suppressed by quiet mode and this site is the odd one out.

What use case are you thinking of where someone explicitly adds -q but
wants to see non fatal warnings?

Thanks
James

> 
> Thanks,
> Namhyung
> 
> 
>>>
>>> This fixes the following failure due to the extra lines counted:
>>>
>>>   perf test "lock cont" -vvv
>>>
>>>   82: kernel lock contention analysis test                            :
>>>   --- start ---
>>>   test child forked, pid 3125
>>>   Testing perf lock record and perf lock contention
>>>   [Fail] Recorded result count is not 1: 9
>>>   test child finished with -1
>>>   ---- end ----
>>>   kernel lock contention analysis test: FAILED!
>>>
>>> Fixes: ec685de25b67 ("perf test: Add kernel lock contention test")
>>> Cc: Namhyung Kim <namhyung@kernel.org>
>>> Signed-off-by: James Clark <james.clark@arm.com>
>>> ---
>>>  tools/perf/ui/util.c | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/tools/perf/ui/util.c b/tools/perf/ui/util.c
>>> index 689b27c34246..1d38ddf01b60 100644
>>> --- a/tools/perf/ui/util.c
>>> +++ b/tools/perf/ui/util.c
>>> @@ -15,6 +15,9 @@ static int perf_stdio__error(const char *format, va_list args)
>>>
>>>  static int perf_stdio__warning(const char *format, va_list args)
>>>  {
>>> +     if (quiet)
>>> +             return 0;
>>> +
>>>       fprintf(stderr, "Warning:\n");
>>>       vfprintf(stderr, format, args);
>>>       return 0;
>>> @@ -45,6 +48,8 @@ int ui__warning(const char *format, ...)
>>>  {
>>>       int ret;
>>>       va_list args;
>>> +     if (quiet)
>>> +             return 0;
>>>
>>>       va_start(args, format);
>>>       ret = perf_eops->warning(format, args);

Re: [PATCH] perf: Fix "Track with sched_switch" test by not printing warnings in quiet mode

[Namhyung Kim]

On Wed, Oct 12, 2022 at 10:12 AM James Clark <james.clark@arm.com> wrote:
>
>
>
> On 12/10/2022 17:50, Namhyung Kim wrote:
> > On Wed, Oct 12, 2022 at 4:13 AM James Clark <james.clark@arm.com> wrote:
> >>> The test already supplies -q to run in quiet mode, so extend quiet mode
> >>> to perf_stdio__warning() and also ui__warning() for consistency.
> >
> > I'm not sure if suppressing the warnings with -q is a good thing.
> > Maybe we need to separate warning/debug messages from the output.
>
> I don't see the issue with warnings being suppressed in quiet mode as
> long as errors are still printed. In other cases warnings have already
> been suppressed by quiet mode and this site is the odd one out.
>
> What use case are you thinking of where someone explicitly adds -q but
> wants to see non fatal warnings?

I don't have any specific use case.  If it's already suppressed in other
cases, I'm fine with it.

Thanks,
Namhyung

Re: [PATCH] perf: Fix "Track with sched_switch" test by not printing warnings in quiet mode

[James Clark]

On 13/10/2022 17:57, Namhyung Kim wrote:
> On Wed, Oct 12, 2022 at 10:12 AM James Clark <james.clark@arm.com> wrote:
>>
>>
>>
>> On 12/10/2022 17:50, Namhyung Kim wrote:
>>> On Wed, Oct 12, 2022 at 4:13 AM James Clark <james.clark@arm.com> wrote:
>>>>> The test already supplies -q to run in quiet mode, so extend quiet mode
>>>>> to perf_stdio__warning() and also ui__warning() for consistency.
>>>
>>> I'm not sure if suppressing the warnings with -q is a good thing.
>>> Maybe we need to separate warning/debug messages from the output.
>>
>> I don't see the issue with warnings being suppressed in quiet mode as
>> long as errors are still printed. In other cases warnings have already
>> been suppressed by quiet mode and this site is the odd one out.
>>
>> What use case are you thinking of where someone explicitly adds -q but
>> wants to see non fatal warnings?
> 
> I don't have any specific use case.  If it's already suppressed in other
> cases, I'm fine with it.
> 

Actually I may have been mistaken. Seems like quiet is only used for
"extra info" type messages rather than warnings. Although the commit
message does say:

  The -q/--quiet option is to suppress any message. Sometimes users just
  want to see the numbers and it can be used for that case.

With 'any' that I would take to include warnings as well. I could move
warnings to stderr, but this has a much greater chance of breaking
anyone's workflows that might be looking for warnings on stdout than
removing warnings when -q is provided.

Also if warnings are moved to stderr and quiet isn't used, there would
be no way to suppress warnings in the TUI which might actually be a
useful feature.

So I'm still leaning towards the original change, if you are ok with
that even though it's not done elsewhere?

> Thanks,
> Namhyung

Re: [PATCH] perf: Fix "Track with sched_switch" test by not printing warnings in quiet mode

[Arnaldo Carvalho de Melo]

Em Fri, Oct 14, 2022 at 10:47:34AM +0100, James Clark escreveu:
> On 13/10/2022 17:57, Namhyung Kim wrote:
> > On Wed, Oct 12, 2022 at 10:12 AM James Clark <james.clark@arm.com> wrote:
> >> On 12/10/2022 17:50, Namhyung Kim wrote:
> >>> On Wed, Oct 12, 2022 at 4:13 AM James Clark <james.clark@arm.com> wrote:
> >>>>> The test already supplies -q to run in quiet mode, so extend quiet mode
> >>>>> to perf_stdio__warning() and also ui__warning() for consistency.
> >>>
> >>> I'm not sure if suppressing the warnings with -q is a good thing.
> >>> Maybe we need to separate warning/debug messages from the output.
> >>
> >> I don't see the issue with warnings being suppressed in quiet mode as
> >> long as errors are still printed. In other cases warnings have already
> >> been suppressed by quiet mode and this site is the odd one out.
> >>
> >> What use case are you thinking of where someone explicitly adds -q but
> >> wants to see non fatal warnings?
> > 
> > I don't have any specific use case.  If it's already suppressed in other
> > cases, I'm fine with it.
> > 
> 
> Actually I may have been mistaken. Seems like quiet is only used for
> "extra info" type messages rather than warnings. Although the commit
> message does say:
> 
>   The -q/--quiet option is to suppress any message. Sometimes users just
>   want to see the numbers and it can be used for that case.
> 
> With 'any' that I would take to include warnings as well. I could move
> warnings to stderr, but this has a much greater chance of breaking
> anyone's workflows that might be looking for warnings on stdout than
> removing warnings when -q is provided.
> 
> Also if warnings are moved to stderr and quiet isn't used, there would
> be no way to suppress warnings in the TUI which might actually be a
> useful feature.
> 
> So I'm still leaning towards the original change, if you are ok with
> that even though it's not done elsewhere?

Namhyung? I tend to agree with James.

- Arnaldo

Re: [PATCH] perf: Fix "Track with sched_switch" test by not printing warnings in quiet mode

[Namhyung Kim]

On Mon, Oct 17, 2022 at 5:33 AM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> Em Fri, Oct 14, 2022 at 10:47:34AM +0100, James Clark escreveu:
> > On 13/10/2022 17:57, Namhyung Kim wrote:
> > > On Wed, Oct 12, 2022 at 10:12 AM James Clark <james.clark@arm.com> wrote:
> > >> On 12/10/2022 17:50, Namhyung Kim wrote:
> > >>> On Wed, Oct 12, 2022 at 4:13 AM James Clark <james.clark@arm.com> wrote:
> > >>>>> The test already supplies -q to run in quiet mode, so extend quiet mode
> > >>>>> to perf_stdio__warning() and also ui__warning() for consistency.
> > >>>
> > >>> I'm not sure if suppressing the warnings with -q is a good thing.
> > >>> Maybe we need to separate warning/debug messages from the output.
> > >>
> > >> I don't see the issue with warnings being suppressed in quiet mode as
> > >> long as errors are still printed. In other cases warnings have already
> > >> been suppressed by quiet mode and this site is the odd one out.
> > >>
> > >> What use case are you thinking of where someone explicitly adds -q but
> > >> wants to see non fatal warnings?
> > >
> > > I don't have any specific use case.  If it's already suppressed in other
> > > cases, I'm fine with it.
> > >
> >
> > Actually I may have been mistaken. Seems like quiet is only used for
> > "extra info" type messages rather than warnings. Although the commit
> > message does say:
> >
> >   The -q/--quiet option is to suppress any message. Sometimes users just
> >   want to see the numbers and it can be used for that case.
> >
> > With 'any' that I would take to include warnings as well. I could move
> > warnings to stderr, but this has a much greater chance of breaking
> > anyone's workflows that might be looking for warnings on stdout than
> > removing warnings when -q is provided.
> >
> > Also if warnings are moved to stderr and quiet isn't used, there would
> > be no way to suppress warnings in the TUI which might actually be a
> > useful feature.
> >
> > So I'm still leaning towards the original change, if you are ok with
> > that even though it's not done elsewhere?
>
> Namhyung? I tend to agree with James.

I think it's a matter of choice.  With this change, users won't see
warnings with -q anymore.  This MIGHT affect some users as they
can see low quality data silently due to missing data or something.

If you're ok with the behavior, I'm fine.  But then we need to update
the document to clarify the behavior.

Thanks,
Namhyung