| Message ID | 1466171011-30468-1-git-send-email-will.deacon@arm.com |
|---|---|
| State | New |
| Headers | show |
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@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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; >
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@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >> > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > . > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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
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. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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
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;
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(-) -- 2.1.4 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel