[PATCH v1] Print a log when the sched clock is marked unstable

[PATCH v1] Print a log when the sched clock is marked unstable

[Ani Sinha]

In other parts of the kernel when the sched clock transitions from
stable to unstable and vice versa, a kernel info log is printed.
When the sched clock is marked explicitly as unstable, we should
print an information log as well. This patch addresses this. It is
useful in cases where for example, we want to understand why a
certain feature like NOHZ, which depends on a stable sched clock, is
not available.

Signed-off-by: Ani Sinha <ani@anisinha.ca>
---
 kernel/sched/clock.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

See also thread
https://lkml.org/lkml/2021/6/20/32

diff --git a/kernel/sched/clock.c b/kernel/sched/clock.c
index c2b2859ddd82..5a403b2c9b3f 100644
--- a/kernel/sched/clock.c
+++ b/kernel/sched/clock.c
@@ -192,8 +192,10 @@ void clear_sched_clock_stable(void)
 
 	smp_mb(); /* matches sched_clock_init_late() */
 
-	if (static_key_count(&sched_clock_running.key) == 2)
+	if (static_key_count(&sched_clock_running.key) == 2) {
+		pr_info("sched_clock: Marking unstable.\n");
 		__clear_sched_clock_stable();
+	}
 }
 
 static void __sched_clock_gtod_offset(void)
-- 
2.25.1

Re: [PATCH v1] Print a log when the sched clock is marked unstable

[Ani Sinha]

ping ...

On Sun, Jun 27, 2021 at 12:19 PM Ani Sinha <ani@anisinha.ca> wrote:
>
> In other parts of the kernel when the sched clock transitions from
> stable to unstable and vice versa, a kernel info log is printed.
> When the sched clock is marked explicitly as unstable, we should
> print an information log as well. This patch addresses this. It is
> useful in cases where for example, we want to understand why a
> certain feature like NOHZ, which depends on a stable sched clock, is
> not available.
>
> Signed-off-by: Ani Sinha <ani@anisinha.ca>
> ---
>  kernel/sched/clock.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> See also thread
> https://lkml.org/lkml/2021/6/20/32
>
> diff --git a/kernel/sched/clock.c b/kernel/sched/clock.c
> index c2b2859ddd82..5a403b2c9b3f 100644
> --- a/kernel/sched/clock.c
> +++ b/kernel/sched/clock.c
> @@ -192,8 +192,10 @@ void clear_sched_clock_stable(void)
>
>         smp_mb(); /* matches sched_clock_init_late() */
>
> -       if (static_key_count(&sched_clock_running.key) == 2)
> +       if (static_key_count(&sched_clock_running.key) == 2) {
> +               pr_info("sched_clock: Marking unstable.\n");
>                 __clear_sched_clock_stable();
> +       }
>  }
>
>  static void __sched_clock_gtod_offset(void)
> --
> 2.25.1

Re: [PATCH v1] Print a log when the sched clock is marked unstable

[Thomas Gleixner]

Ani,

On Sun, Jun 27 2021 at 12:19, Ani Sinha wrote:

I asked you to read Documentation/process carefully and I told you:

        Also your subject line want's a proper prefix.

Your patch is missing a prefix again. Hint:

  git log kernel/sched/clock.c

> In other parts of the kernel when the sched clock transitions from
> stable to unstable and vice versa, a kernel info log is printed.  When
> the sched clock is marked explicitly as unstable, we should print an
> information log as well. This patch addresses this. It is useful in

git grep 'This patch' Documentation/process/

Also please avoid the 'we' wording. 'We should print' does not make
sense if you really think about it.

> cases where for example, we want to understand why a certain feature
> like NOHZ, which depends on a stable sched clock, is not available.
>
> Signed-off-by: Ani Sinha <ani@anisinha.ca>
> ---
>  kernel/sched/clock.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> See also thread
> https://lkml.org/lkml/2021/6/20/32
>
> diff --git a/kernel/sched/clock.c b/kernel/sched/clock.c
> index c2b2859ddd82..5a403b2c9b3f 100644
> --- a/kernel/sched/clock.c
> +++ b/kernel/sched/clock.c
> @@ -192,8 +192,10 @@ void clear_sched_clock_stable(void)
>  
>  	smp_mb(); /* matches sched_clock_init_late() */
>  
> -	if (static_key_count(&sched_clock_running.key) == 2)
> +	if (static_key_count(&sched_clock_running.key) == 2) {
> +		pr_info("sched_clock: Marking unstable.\n");
>  		__clear_sched_clock_stable();

Why isn't that in __clear_sched_clock_stable() ?

Thanks,

        tglx

Re: [PATCH v1] Print a log when the sched clock is marked unstable

[Ani Sinha]

Tglx,

On Wed, 7 Jul 2021, Thomas Gleixner wrote:

> Ani,
>
> On Sun, Jun 27 2021 at 12:19, Ani Sinha wrote:
>
> I asked you to read Documentation/process carefully and I told you:
>
>         Also your subject line want's a proper prefix.
>
> Your patch is missing a prefix again. Hint:
>
>   git log kernel/sched/clock.c
>

OK thanks for the pointer. Its my bad that staring at the computer for so
many hours stuck at home makes me read "prefix" as "fix". So I read

Also your subject line want's a proper fix


> > In other parts of the kernel when the sched clock transitions from
> > stable to unstable and vice versa, a kernel info log is printed.  When
> > the sched clock is marked explicitly as unstable, we should print an
> > information log as well. This patch addresses this. It is useful in
>
> git grep 'This patch' Documentation/process/
>
> Also please avoid the 'we' wording. 'We should print' does not make
> sense if you really think about it.

Please understand that Linux kernel is not the only upstream software
where I am contrinuting. Each community has their own rules. Maybe add a
check in checkpatch.pl in the kernel so that these things maybe caught
automatically?

>
> > cases where for example, we want to understand why a certain feature
> > like NOHZ, which depends on a stable sched clock, is not available.
> >
> > Signed-off-by: Ani Sinha <ani@anisinha.ca>
> > ---
> >  kernel/sched/clock.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > See also thread
> > https://lkml.org/lkml/2021/6/20/32
> >
> > diff --git a/kernel/sched/clock.c b/kernel/sched/clock.c
> > index c2b2859ddd82..5a403b2c9b3f 100644
> > --- a/kernel/sched/clock.c
> > +++ b/kernel/sched/clock.c
> > @@ -192,8 +192,10 @@ void clear_sched_clock_stable(void)
> >
> >  	smp_mb(); /* matches sched_clock_init_late() */
> >
> > -	if (static_key_count(&sched_clock_running.key) == 2)
> > +	if (static_key_count(&sched_clock_running.key) == 2) {
> > +		pr_info("sched_clock: Marking unstable.\n");
> >  		__clear_sched_clock_stable();
>
> Why isn't that in __clear_sched_clock_stable() ?
>

updated in v2. Makes sense do emit the log at the lowest level of
abstraction so that all code paths are potentially caught.

Ani