[RFC PATCH] clocksource: arm_arch_timer: disable the evtstrm via the cmdline

[RFC PATCH] clocksource: arm_arch_timer: disable the evtstrm via the cmdline

[Will Deacon]

Disabling the eventstream can be useful for debugging and development
purposes and is currently controlled via a Kconfig option
(CONFIG_ARM_ARCH_TIMER_EVTSTREAM). Whilst this does the trick, it's
often desirable to toggle the feature on the command line, so this patch
adds a "noevtstrm" command line option to do just that.

Signed-off-by: Will Deacon <will.deacon@arm.com>
---

Sending as an RFC because we might want to remove the Kconfig option
altogether if we have this.

 drivers/clocksource/arm_arch_timer.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 4814446a0024..f579c0da7423 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -79,6 +79,15 @@ static enum ppi_nr arch_timer_uses_ppi = VIRT_PPI;
 static bool arch_timer_c3stop;
 static bool arch_timer_mem_use_virtual;
 
+static bool evtstrm_disable;
+
+static int __init early_evtstrm_disable(char *buf)
+{
+	evtstrm_disable = true;
+	return 0;
+}
+early_param("noevtstrm", early_evtstrm_disable);
+
 /*
  * Architected system timer support.
  */
@@ -372,7 +381,7 @@ static int arch_timer_setup(struct clock_event_device *clk)
 		enable_percpu_irq(arch_timer_ppi[PHYS_NONSECURE_PPI], 0);
 
 	arch_counter_set_user_access();
-	if (IS_ENABLED(CONFIG_ARM_ARCH_TIMER_EVTSTREAM))
+	if (IS_ENABLED(CONFIG_ARM_ARCH_TIMER_EVTSTREAM) && !evtstrm_disable)
 		arch_timer_configure_evtstream();
 
 	return 0;
-- 
2.1.4

[RFC PATCH] clocksource: arm_arch_timer: disable the evtstrm via the cmdline

[Mark Rutland]

On Fri, Jun 17, 2016 at 02:43:31PM +0100, Will Deacon wrote:
> Disabling the eventstream can be useful for debugging and development
> purposes and is currently controlled via a Kconfig option
> (CONFIG_ARM_ARCH_TIMER_EVTSTREAM). Whilst this does the trick, it's
> often desirable to toggle the feature on the command line, so this patch
> adds a "noevtstrm" command line option to do just that.

I think anything that allows for better debugging on a production
kernels is great, so FWIW:

Acked-by: Mark Rutland <mark.rutland@arm.com>

We might want to drop something in Documentation/kernel-parameters.txt

Mark.

> 
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
> 
> Sending as an RFC because we might want to remove the Kconfig option
> altogether if we have this.
> 
>  drivers/clocksource/arm_arch_timer.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index 4814446a0024..f579c0da7423 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -79,6 +79,15 @@ static enum ppi_nr arch_timer_uses_ppi = VIRT_PPI;
>  static bool arch_timer_c3stop;
>  static bool arch_timer_mem_use_virtual;
>  
> +static bool evtstrm_disable;
> +
> +static int __init early_evtstrm_disable(char *buf)
> +{
> +	evtstrm_disable = true;
> +	return 0;
> +}
> +early_param("noevtstrm", early_evtstrm_disable);
> +
>  /*
>   * Architected system timer support.
>   */
> @@ -372,7 +381,7 @@ static int arch_timer_setup(struct clock_event_device *clk)
>  		enable_percpu_irq(arch_timer_ppi[PHYS_NONSECURE_PPI], 0);
>  
>  	arch_counter_set_user_access();
> -	if (IS_ENABLED(CONFIG_ARM_ARCH_TIMER_EVTSTREAM))
> +	if (IS_ENABLED(CONFIG_ARM_ARCH_TIMER_EVTSTREAM) && !evtstrm_disable)
>  		arch_timer_configure_evtstream();
>  
>  	return 0;
> -- 
> 2.1.4
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

[RFC PATCH] clocksource: arm_arch_timer: disable the evtstrm via the cmdline

[Daniel Lezcano]

On 06/17/2016 03:43 PM, Will Deacon wrote:

[ Cc'ed tglx ]

> Disabling the eventstream can be useful for debugging and development
> purposes

If it is for debugging and development, why upstream this change ?

> and is currently controlled via a Kconfig option
> (CONFIG_ARM_ARCH_TIMER_EVTSTREAM). Whilst this does the trick, it's
> often desirable to toggle the feature on the command line, so this patch
> adds a "noevtstrm" command line option to do just that.
>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
>
> Sending as an RFC because we might want to remove the Kconfig option
> altogether if we have this.
>
>   drivers/clocksource/arm_arch_timer.c | 11 ++++++++++-
>   1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index 4814446a0024..f579c0da7423 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -79,6 +79,15 @@ static enum ppi_nr arch_timer_uses_ppi = VIRT_PPI;
>   static bool arch_timer_c3stop;
>   static bool arch_timer_mem_use_virtual;
>
> +static bool evtstrm_disable;
> +
> +static int __init early_evtstrm_disable(char *buf)
> +{
> +	evtstrm_disable = true;
> +	return 0;
> +}
> +early_param("noevtstrm", early_evtstrm_disable);
> +
>   /*
>    * Architected system timer support.
>    */
> @@ -372,7 +381,7 @@ static int arch_timer_setup(struct clock_event_device *clk)
>   		enable_percpu_irq(arch_timer_ppi[PHYS_NONSECURE_PPI], 0);
>
>   	arch_counter_set_user_access();
> -	if (IS_ENABLED(CONFIG_ARM_ARCH_TIMER_EVTSTREAM))
> +	if (IS_ENABLED(CONFIG_ARM_ARCH_TIMER_EVTSTREAM) && !evtstrm_disable)
>   		arch_timer_configure_evtstream();
>
>   	return 0;
>


-- 
  <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[RFC PATCH] clocksource: arm_arch_timer: disable the evtstrm via the cmdline

[Kefeng Wang]

On 2016/6/17 22:36, Mark Rutland wrote:
> On Fri, Jun 17, 2016 at 02:43:31PM +0100, Will Deacon wrote:
>> Disabling the eventstream can be useful for debugging and development
>> purposes and is currently controlled via a Kconfig option
>> (CONFIG_ARM_ARCH_TIMER_EVTSTREAM). Whilst this does the trick, it's
>> often desirable to toggle the feature on the command line, so this patch
>> adds a "noevtstrm" command line option to do just that.
> 
> I think anything that allows for better debugging on a production
> kernels is great, so FWIW:

Agree, I run linux in arm esl and the linux can't boot up without
ARM_ARCH_TIMER_EVTSTREAM recently.

The esl guys is looking into the issue, and it is useful to debug issue
with one Image(dynamic switching in cmdline).

BRs,
Kefeng


> 
> Acked-by: Mark Rutland <mark.rutland@arm.com>
> 
> We might want to drop something in Documentation/kernel-parameters.txt
> 
> Mark.
> 
>>
>> Signed-off-by: Will Deacon <will.deacon@arm.com>
>> ---
>>
>> Sending as an RFC because we might want to remove the Kconfig option
>> altogether if we have this.
>>
>>  drivers/clocksource/arm_arch_timer.c | 11 ++++++++++-
>>  1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
>> index 4814446a0024..f579c0da7423 100644
>> --- a/drivers/clocksource/arm_arch_timer.c
>> +++ b/drivers/clocksource/arm_arch_timer.c
>> @@ -79,6 +79,15 @@ static enum ppi_nr arch_timer_uses_ppi = VIRT_PPI;
>>  static bool arch_timer_c3stop;
>>  static bool arch_timer_mem_use_virtual;
>>  
>> +static bool evtstrm_disable;
>> +
>> +static int __init early_evtstrm_disable(char *buf)
>> +{
>> +	evtstrm_disable = true;
>> +	return 0;
>> +}
>> +early_param("noevtstrm", early_evtstrm_disable);
>> +
>>  /*
>>   * Architected system timer support.
>>   */
>> @@ -372,7 +381,7 @@ static int arch_timer_setup(struct clock_event_device *clk)
>>  		enable_percpu_irq(arch_timer_ppi[PHYS_NONSECURE_PPI], 0);
>>  
>>  	arch_counter_set_user_access();
>> -	if (IS_ENABLED(CONFIG_ARM_ARCH_TIMER_EVTSTREAM))
>> +	if (IS_ENABLED(CONFIG_ARM_ARCH_TIMER_EVTSTREAM) && !evtstrm_disable)
>>  		arch_timer_configure_evtstream();
>>  
>>  	return 0;
>> -- 
>> 2.1.4
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
> .
> 

[RFC PATCH] clocksource: arm_arch_timer: disable the evtstrm via the cmdline

[Will Deacon]

On Sun, Jun 19, 2016 at 10:08:38PM +0200, Daniel Lezcano wrote:
> On 06/17/2016 03:43 PM, Will Deacon wrote:
> 
> [ Cc'ed tglx ]
> 
> >Disabling the eventstream can be useful for debugging and development
> >purposes
> 
> If it is for debugging and development, why upstream this change ?

Mainly because it's desirable to be able to debug systems remotely, on
machines that you don't have direct access to and where recompiling the
kernel isn't necessarily an option. There are plenty of "no*" kernel
parameters already that fall into a similar category.

Will

[RFC PATCH] clocksource: arm_arch_timer: disable the evtstrm via the cmdline

[Marc Zyngier]

On Mon, 20 Jun 2016 09:21:57 +0100
Will Deacon <will.deacon@arm.com> wrote:

> On Sun, Jun 19, 2016 at 10:08:38PM +0200, Daniel Lezcano wrote:
> > On 06/17/2016 03:43 PM, Will Deacon wrote:
> > 
> > [ Cc'ed tglx ]
> > 
> > >Disabling the eventstream can be useful for debugging and development
> > >purposes
> > 
> > If it is for debugging and development, why upstream this change ?
> 
> Mainly because it's desirable to be able to debug systems remotely, on
> machines that you don't have direct access to and where recompiling the
> kernel isn't necessarily an option. There are plenty of "no*" kernel
> parameters already that fall into a similar category.

I would add that being able to toggle this without affecting the memory
layout or any of the code paths is extremely valuable. The event stream
can have an effect on the spinlock behaviour (through the WFE
instruction), so having a way to exercise it without changing anything
else is pretty cool. Also, being able to debug a production kernel at
zero cost is rare enough to make me jump on the opportunity.

So for this patch, and Mark's suggestion of adding some documentation
to it:

Acked-by: Marc Zyngier <marc.zyngier@arm.com>

	M.
-- 
Jazz is not dead. It just smells funny.

[RFC PATCH] clocksource: arm_arch_timer: disable the evtstrm via the cmdline

[Daniel Lezcano]

On 06/20/2016 10:21 AM, Will Deacon wrote:
> On Sun, Jun 19, 2016 at 10:08:38PM +0200, Daniel Lezcano wrote:
>> On 06/17/2016 03:43 PM, Will Deacon wrote:
>>
>> [ Cc'ed tglx ]
>>
>>> Disabling the eventstream can be useful for debugging and development
>>> purposes
>>
>> If it is for debugging and development, why upstream this change ?
>
> Mainly because it's desirable to be able to debug systems remotely, on
> machines that you don't have direct access to and where recompiling the
> kernel isn't necessarily an option. There are plenty of "no*" kernel
> parameters already that fall into a similar category.

Hi Will,

if the kernel is in development and debug, why this option can't be part 
of debugging code ?

If recompiling isn't an option, how this can be for "debugging and 
development" ?

I'm not a big fan of the all the specific driver options for the kernel 
parameters. If there is a real need to disable some parts of a driver, 
it would be much more interesting to write a framework for that and then 
use it from arm_arch_timer, thus giving the other drivers the 
opportunity to provide the same feature.

   -- Daniel


-- 
  <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[RFC PATCH] clocksource: arm_arch_timer: disable the evtstrm via the cmdline

[Mark Rutland]

On Mon, Jun 20, 2016 at 02:59:11PM +0200, Daniel Lezcano wrote:
> On 06/20/2016 10:21 AM, Will Deacon wrote:
> >On Sun, Jun 19, 2016 at 10:08:38PM +0200, Daniel Lezcano wrote:
> >>On 06/17/2016 03:43 PM, Will Deacon wrote:
> >>
> >>[ Cc'ed tglx ]
> >>
> >>>Disabling the eventstream can be useful for debugging and development
> >>>purposes
> >>
> >>If it is for debugging and development, why upstream this change ?
> >
> >Mainly because it's desirable to be able to debug systems remotely, on
> >machines that you don't have direct access to and where recompiling the
> >kernel isn't necessarily an option. There are plenty of "no*" kernel
> >parameters already that fall into a similar category.
> 
> Hi Will,
> 
> if the kernel is in development and debug, why this option can't be
> part of debugging code ?
> 
> If recompiling isn't an option, how this can be for "debugging and
> development" ?

We already have the config option for the cases you wish to set this at
build time, and people use it.

There are situations where you do not know at build time that you want
this, e.g. debugging in the field, for which recompilation may change
the code in other ways (e.g. layout of data and functions).

For example, if we get a bug report that a production kernel wedges in
spinlock code after running for 10 hours, being able to say "pass
noevstrm" is much better than trying to get the end-user to recompile
their kernel, which may or may not be possible, and may (for other
reasons), mask the issue we wish to debug.

The code size impact is negligible, and there is no runtime performance
impact if this option is not used.

> I'm not a big fan of the all the specific driver options for the
> kernel parameters. If there is a real need to disable some parts of
> a driver, it would be much more interesting to write a framework for
> that and then use it from arm_arch_timer, thus giving the other
> drivers the opportunity to provide the same feature.

I'm not aware of other devices which have an event stream option.

Additionally, the architected timer is at least slightly special in that
it is architected, and this HW features was designed with architectural
implications in mind (e.g. the behaviour of locking primitives). So
while this happens to live in the driver code, it's in effect an
architecture option (note that we have HWCAP_EVTSTREAM).

Thanks,
Mark.

[RFC PATCH] clocksource: arm_arch_timer: disable the evtstrm via the cmdline

[Will Deacon]

On Mon, Jun 20, 2016 at 02:59:11PM +0200, Daniel Lezcano wrote:
> On 06/20/2016 10:21 AM, Will Deacon wrote:
> >On Sun, Jun 19, 2016 at 10:08:38PM +0200, Daniel Lezcano wrote:
> >>On 06/17/2016 03:43 PM, Will Deacon wrote:
> >>
> >>[ Cc'ed tglx ]
> >>
> >>>Disabling the eventstream can be useful for debugging and development
> >>>purposes
> >>
> >>If it is for debugging and development, why upstream this change ?
> >
> >Mainly because it's desirable to be able to debug systems remotely, on
> >machines that you don't have direct access to and where recompiling the
> >kernel isn't necessarily an option. There are plenty of "no*" kernel
> >parameters already that fall into a similar category.
> 
> if the kernel is in development and debug, why this option can't be part of
> debugging code ?
> 
> If recompiling isn't an option, how this can be for "debugging and
> development" ?

Sorry, I wasn't very clear. The sort of use-case I'm envisaging is where
somebody is running a distro kernel on non-public hardware and sends me an
email about spinlock performance. Being able to disable the event stream
easily is a powerful tool in the small box of remote debugging tools and
would be useful in pinning down things like missing events.

So when I say "development and debug" I'm really thinking about *remote*
debug via a user, and then potentially the subsequent development of a
patch.

> I'm not a big fan of the all the specific driver options for the kernel
> parameters. If there is a real need to disable some parts of a driver, it
> would be much more interesting to write a framework for that and then use it
> from arm_arch_timer, thus giving the other drivers the opportunity to
> provide the same feature.

Such a framework sounds horribly ill-specified and far beyond the scope
of the simple change I'm proposing here. Are you planning to submit
patches for this?

Will

[RFC PATCH] clocksource: arm_arch_timer: disable the evtstrm via the cmdline

[Catalin Marinas]

On Mon, Jun 20, 2016 at 02:59:11PM +0200, Daniel Lezcano wrote:
> On 06/20/2016 10:21 AM, Will Deacon wrote:
> >On Sun, Jun 19, 2016 at 10:08:38PM +0200, Daniel Lezcano wrote:
> >>On 06/17/2016 03:43 PM, Will Deacon wrote:
> >>
> >>[ Cc'ed tglx ]
> >>
> >>>Disabling the eventstream can be useful for debugging and development
> >>>purposes
> >>
> >>If it is for debugging and development, why upstream this change ?
> >
> >Mainly because it's desirable to be able to debug systems remotely, on
> >machines that you don't have direct access to and where recompiling the
> >kernel isn't necessarily an option. There are plenty of "no*" kernel
> >parameters already that fall into a similar category.
> 
> if the kernel is in development and debug, why this option can't be part of
> debugging code ?

Because we may actually be debugging the hardware rather than the
software. With the event stream enabled, WFE is woken up periodically.
This can be a handy feature for user locking primitives or a simple
workaround for hardware bugs (and we've seen them before). But the side
effect is that it may be potentially hiding hardware issues.

For hardware testing/validation, you'd want to sometimes disable this
feature to check whether your event generation (usually as a result of
exclusive monitor clearing) is working as expected. It's much easier to
do with a command line option.

> I'm not a big fan of the all the specific driver options for the kernel
> parameters. If there is a real need to disable some parts of a driver, it
> would be much more interesting to write a framework for that and then use it
> from arm_arch_timer, thus giving the other drivers the opportunity to
> provide the same feature.

Well, how many non-ARM timer drivers have an exclusive monitor event
stream feature to make sense for a framework?

-- 
Catalin

[RFC PATCH] clocksource: arm_arch_timer: disable the evtstrm via the cmdline

[Mark Rutland]

On Mon, Jun 20, 2016 at 02:30:02PM +0100, Will Deacon wrote:
> On Mon, Jun 20, 2016 at 02:59:11PM +0200, Daniel Lezcano wrote:
> > On 06/20/2016 10:21 AM, Will Deacon wrote:
> > >On Sun, Jun 19, 2016 at 10:08:38PM +0200, Daniel Lezcano wrote:
> > >>On 06/17/2016 03:43 PM, Will Deacon wrote:
> > >>
> > >>[ Cc'ed tglx ]
> > >>
> > >>>Disabling the eventstream can be useful for debugging and development
> > >>>purposes
> > >>
> > >>If it is for debugging and development, why upstream this change ?
> > >
> > >Mainly because it's desirable to be able to debug systems remotely, on
> > >machines that you don't have direct access to and where recompiling the
> > >kernel isn't necessarily an option. There are plenty of "no*" kernel
> > >parameters already that fall into a similar category.
> > 
> > if the kernel is in development and debug, why this option can't be part of
> > debugging code ?
> > 
> > If recompiling isn't an option, how this can be for "debugging and
> > development" ?
> 
> Sorry, I wasn't very clear. The sort of use-case I'm envisaging is where
> somebody is running a distro kernel on non-public hardware and sends me an
> email about spinlock performance. Being able to disable the event stream
> easily is a powerful tool in the small box of remote debugging tools and
> would be useful in pinning down things like missing events.
> 
> So when I say "development and debug" I'm really thinking about *remote*
> debug via a user, and then potentially the subsequent development of a
> patch.

We should probably place this rationale in the commit message. e.g.
(fake emphasis):

Disabling the eventstream can be useful for *remotely* debugging *a
deployed production system*.

Mark.

[RFC PATCH] clocksource: arm_arch_timer: disable the evtstrm via the cmdline

[Daniel Lezcano]

On 06/20/2016 03:30 PM, Catalin Marinas wrote:
> On Mon, Jun 20, 2016 at 02:59:11PM +0200, Daniel Lezcano wrote:
>> On 06/20/2016 10:21 AM, Will Deacon wrote:
>>> On Sun, Jun 19, 2016 at 10:08:38PM +0200, Daniel Lezcano wrote:
>>>> On 06/17/2016 03:43 PM, Will Deacon wrote:
>>>>
>>>> [ Cc'ed tglx ]
>>>>
>>>>> Disabling the eventstream can be useful for debugging and development
>>>>> purposes
>>>>
>>>> If it is for debugging and development, why upstream this change ?
>>>
>>> Mainly because it's desirable to be able to debug systems remotely, on
>>> machines that you don't have direct access to and where recompiling the
>>> kernel isn't necessarily an option. There are plenty of "no*" kernel
>>> parameters already that fall into a similar category.
>>
>> if the kernel is in development and debug, why this option can't be part of
>> debugging code ?
>
> Because we may actually be debugging the hardware rather than the
> software. With the event stream enabled, WFE is woken up periodically.
> This can be a handy feature for user locking primitives or a simple
> workaround for hardware bugs (and we've seen them before). But the side
> effect is that it may be potentially hiding hardware issues.
>
> For hardware testing/validation, you'd want to sometimes disable this
> feature to check whether your event generation (usually as a result of
> exclusive monitor clearing) is working as expected. It's much easier to
> do with a command line option.
>
>> I'm not a big fan of the all the specific driver options for the kernel
>> parameters. If there is a real need to disable some parts of a driver, it
>> would be much more interesting to write a framework for that and then use it
>> from arm_arch_timer, thus giving the other drivers the opportunity to
>> provide the same feature.
>
> Well, how many non-ARM timer drivers have an exclusive monitor event
> stream feature to make sense for a framework?

Ok.

What about an option like:

clocksource.arch_arm.evtstream = 0/1




-- 
  <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog