diff mbox

[v2,09/22] brcm80211: Allow trace support to be enabled separately from debug

Message ID 1352988492-21340-10-git-send-email-seth.forshee@canonical.com (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Seth Forshee Nov. 15, 2012, 2:07 p.m. UTC
Since the runtime overhead of trace support is small when tracing is
disabled, users may be interested in turning on trace support while
leaving other debug features off. Add a new config option named
CONFIG_BRCM_TRACING for this purpose.

Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
---
 drivers/net/wireless/brcm80211/Kconfig             |   11 +++++++++++
 .../brcm80211/brcmsmac/brcms_trace_events.h        |    6 +++---
 2 files changed, 14 insertions(+), 3 deletions(-)

Comments

Arend van Spriel Nov. 19, 2012, 8:33 p.m. UTC | #1
On 11/15/2012 03:07 PM, Seth Forshee wrote:
> Since the runtime overhead of trace support is small when tracing is
> disabled, users may be interested in turning on trace support while
> leaving other debug features off. Add a new config option named
> CONFIG_BRCM_TRACING for this purpose.

Reviewed-by: Pieter-Paul Giesberts <pieterpg@broadcom.com>
Reviewed-by: Arend van Spriel
> Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> ---
>   drivers/net/wireless/brcm80211/Kconfig             |   11 +++++++++++
>   .../brcm80211/brcmsmac/brcms_trace_events.h        |    6 +++---
>   2 files changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/wireless/brcm80211/Kconfig b/drivers/net/wireless/brcm80211/Kconfig
> index c9d811e..3735c27 100644
> --- a/drivers/net/wireless/brcm80211/Kconfig
> +++ b/drivers/net/wireless/brcm80211/Kconfig
> @@ -63,6 +63,17 @@ config BRCMISCAN
>   	  new E-Scan method which uses less memory in firmware and gives no
>   	  limitation on the number of scan results.
>
> +config BRCM_TRACING
> +	bool "Broadcom device tracing"
> +	depends on BRCMSMAC || BRCMFMAC
> +	---help---
> +	  If you say Y here, the Broadcom wireless drivers will register
> +	  with ftrace to dump event information into the trace ringbuffer.
> +	  Tracing can be enabled at runtime to aid in debugging wireless
> +	  issues. This option adds a small amount of overhead when tracing
> +	  is disabled. If unsure, say Y to allow developers to better help
> +	  you when wireless problems occur.
> +

I regard this as a debugging feature. Did you consider making it depend 
on BRCMDBG instead? Or do you think that BRCMDBG code would affect 
run-time behavior during tracing.

>   config BRCMDBG
>   	bool "Broadcom driver debug functions"
>   	depends on BRCMSMAC || BRCMFMAC


--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Seth Forshee Nov. 19, 2012, 9:15 p.m. UTC | #2
On Mon, Nov 19, 2012 at 09:33:13PM +0100, Arend van Spriel wrote:
> On 11/15/2012 03:07 PM, Seth Forshee wrote:
> >Since the runtime overhead of trace support is small when tracing is
> >disabled, users may be interested in turning on trace support while
> >leaving other debug features off. Add a new config option named
> >CONFIG_BRCM_TRACING for this purpose.
> 
> Reviewed-by: Pieter-Paul Giesberts <pieterpg@broadcom.com>
> Reviewed-by: Arend van Spriel
> >Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> >---
> >  drivers/net/wireless/brcm80211/Kconfig             |   11 +++++++++++
> >  .../brcm80211/brcmsmac/brcms_trace_events.h        |    6 +++---
> >  2 files changed, 14 insertions(+), 3 deletions(-)
> >
> >diff --git a/drivers/net/wireless/brcm80211/Kconfig b/drivers/net/wireless/brcm80211/Kconfig
> >index c9d811e..3735c27 100644
> >--- a/drivers/net/wireless/brcm80211/Kconfig
> >+++ b/drivers/net/wireless/brcm80211/Kconfig
> >@@ -63,6 +63,17 @@ config BRCMISCAN
> >  	  new E-Scan method which uses less memory in firmware and gives no
> >  	  limitation on the number of scan results.
> >
> >+config BRCM_TRACING
> >+	bool "Broadcom device tracing"
> >+	depends on BRCMSMAC || BRCMFMAC
> >+	---help---
> >+	  If you say Y here, the Broadcom wireless drivers will register
> >+	  with ftrace to dump event information into the trace ringbuffer.
> >+	  Tracing can be enabled at runtime to aid in debugging wireless
> >+	  issues. This option adds a small amount of overhead when tracing
> >+	  is disabled. If unsure, say Y to allow developers to better help
> >+	  you when wireless problems occur.
> >+
> 
> I regard this as a debugging feature. Did you consider making it
> depend on BRCMDBG instead? Or do you think that BRCMDBG code would
> affect run-time behavior during tracing.

It is a debugging feature, but making it depend on BRCMDBG prevents my
intended use case. I'm planning to enable BRCM_TRACING in Ubuntu and to
leave BRCMDBG disabled. This will make it easy for us to ask users for
detailed debug information when needed with minimal overhead during
normal use.

Seth

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arend van Spriel Nov. 19, 2012, 9:19 p.m. UTC | #3
On 11/19/2012 10:15 PM, Seth Forshee wrote:
> On Mon, Nov 19, 2012 at 09:33:13PM +0100, Arend van Spriel wrote:
>> On 11/15/2012 03:07 PM, Seth Forshee wrote:
>>> Since the runtime overhead of trace support is small when tracing is
>>> disabled, users may be interested in turning on trace support while
>>> leaving other debug features off. Add a new config option named
>>> CONFIG_BRCM_TRACING for this purpose.
>>
>> Reviewed-by: Pieter-Paul Giesberts <pieterpg@broadcom.com>
>> Reviewed-by: Arend van Spriel
>>> Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
>>> ---
>>>   drivers/net/wireless/brcm80211/Kconfig             |   11 +++++++++++
>>>   .../brcm80211/brcmsmac/brcms_trace_events.h        |    6 +++---
>>>   2 files changed, 14 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/net/wireless/brcm80211/Kconfig b/drivers/net/wireless/brcm80211/Kconfig
>>> index c9d811e..3735c27 100644
>>> --- a/drivers/net/wireless/brcm80211/Kconfig
>>> +++ b/drivers/net/wireless/brcm80211/Kconfig
>>> @@ -63,6 +63,17 @@ config BRCMISCAN
>>>   	  new E-Scan method which uses less memory in firmware and gives no
>>>   	  limitation on the number of scan results.
>>>
>>> +config BRCM_TRACING
>>> +	bool "Broadcom device tracing"
>>> +	depends on BRCMSMAC || BRCMFMAC
>>> +	---help---
>>> +	  If you say Y here, the Broadcom wireless drivers will register
>>> +	  with ftrace to dump event information into the trace ringbuffer.
>>> +	  Tracing can be enabled at runtime to aid in debugging wireless
>>> +	  issues. This option adds a small amount of overhead when tracing
>>> +	  is disabled. If unsure, say Y to allow developers to better help
>>> +	  you when wireless problems occur.
>>> +
>>
>> I regard this as a debugging feature. Did you consider making it
>> depend on BRCMDBG instead? Or do you think that BRCMDBG code would
>> affect run-time behavior during tracing.
>
> It is a debugging feature, but making it depend on BRCMDBG prevents my
> intended use case. I'm planning to enable BRCM_TRACING in Ubuntu and to
> leave BRCMDBG disabled. This will make it easy for us to ask users for
> detailed debug information when needed with minimal overhead during
> normal use.

That seems reasonable. Have you any significant impact in throughput 
with tracing enabled?

Gr. AvS


--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Seth Forshee Nov. 19, 2012, 9:33 p.m. UTC | #4
On Mon, Nov 19, 2012 at 10:19:49PM +0100, Arend van Spriel wrote:
> On 11/19/2012 10:15 PM, Seth Forshee wrote:
> >On Mon, Nov 19, 2012 at 09:33:13PM +0100, Arend van Spriel wrote:
> >>On 11/15/2012 03:07 PM, Seth Forshee wrote:
> >>>Since the runtime overhead of trace support is small when tracing is
> >>>disabled, users may be interested in turning on trace support while
> >>>leaving other debug features off. Add a new config option named
> >>>CONFIG_BRCM_TRACING for this purpose.
> >>
> >>Reviewed-by: Pieter-Paul Giesberts <pieterpg@broadcom.com>
> >>Reviewed-by: Arend van Spriel
> >>>Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> >>>---
> >>>  drivers/net/wireless/brcm80211/Kconfig             |   11 +++++++++++
> >>>  .../brcm80211/brcmsmac/brcms_trace_events.h        |    6 +++---
> >>>  2 files changed, 14 insertions(+), 3 deletions(-)
> >>>
> >>>diff --git a/drivers/net/wireless/brcm80211/Kconfig b/drivers/net/wireless/brcm80211/Kconfig
> >>>index c9d811e..3735c27 100644
> >>>--- a/drivers/net/wireless/brcm80211/Kconfig
> >>>+++ b/drivers/net/wireless/brcm80211/Kconfig
> >>>@@ -63,6 +63,17 @@ config BRCMISCAN
> >>>  	  new E-Scan method which uses less memory in firmware and gives no
> >>>  	  limitation on the number of scan results.
> >>>
> >>>+config BRCM_TRACING
> >>>+	bool "Broadcom device tracing"
> >>>+	depends on BRCMSMAC || BRCMFMAC
> >>>+	---help---
> >>>+	  If you say Y here, the Broadcom wireless drivers will register
> >>>+	  with ftrace to dump event information into the trace ringbuffer.
> >>>+	  Tracing can be enabled at runtime to aid in debugging wireless
> >>>+	  issues. This option adds a small amount of overhead when tracing
> >>>+	  is disabled. If unsure, say Y to allow developers to better help
> >>>+	  you when wireless problems occur.
> >>>+
> >>
> >>I regard this as a debugging feature. Did you consider making it
> >>depend on BRCMDBG instead? Or do you think that BRCMDBG code would
> >>affect run-time behavior during tracing.
> >
> >It is a debugging feature, but making it depend on BRCMDBG prevents my
> >intended use case. I'm planning to enable BRCM_TRACING in Ubuntu and to
> >leave BRCMDBG disabled. This will make it easy for us to ask users for
> >detailed debug information when needed with minimal overhead during
> >normal use.
> 
> That seems reasonable. Have you any significant impact in throughput
> with tracing enabled?

I've really only tested with tracing enabled. Compared to before this
patch series the peak performance (as measured with iperf using a TCP
connection) is close enough that any differences would be lost in the
noise, and average performance is better due to the transfer rate being
more stable. I'll run some tests with and without tracing enabled to
compare the results. I may not get to it before next week though due to
the Thanksgiving holiday here in the US.

Seth

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Seth Forshee Nov. 27, 2012, 4:22 p.m. UTC | #5
On Mon, Nov 19, 2012 at 03:33:00PM -0600, Seth Forshee wrote:
> > That seems reasonable. Have you any significant impact in throughput
> > with tracing enabled?
> 
> I've really only tested with tracing enabled. Compared to before this
> patch series the peak performance (as measured with iperf using a TCP
> connection) is close enough that any differences would be lost in the
> noise, and average performance is better due to the transfer rate being
> more stable. I'll run some tests with and without tracing enabled to
> compare the results. I may not get to it before next week though due to
> the Thanksgiving holiday here in the US.

I've done some unscientific comparison between builds with and without
BRCM_TRACING enabled. UDP iperf performance was identical, both send and
receive. With TCP the difference in throughput was small, definitely
within the range of noise for my small sample size. So as far as I can
tell enabling tracing has a negligible impact on throughput.

Seth

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/net/wireless/brcm80211/Kconfig b/drivers/net/wireless/brcm80211/Kconfig
index c9d811e..3735c27 100644
--- a/drivers/net/wireless/brcm80211/Kconfig
+++ b/drivers/net/wireless/brcm80211/Kconfig
@@ -63,6 +63,17 @@  config BRCMISCAN
 	  new E-Scan method which uses less memory in firmware and gives no
 	  limitation on the number of scan results.
 
+config BRCM_TRACING
+	bool "Broadcom device tracing"
+	depends on BRCMSMAC || BRCMFMAC
+	---help---
+	  If you say Y here, the Broadcom wireless drivers will register
+	  with ftrace to dump event information into the trace ringbuffer.
+	  Tracing can be enabled at runtime to aid in debugging wireless
+	  issues. This option adds a small amount of overhead when tracing
+	  is disabled. If unsure, say Y to allow developers to better help
+	  you when wireless problems occur.
+
 config BRCMDBG
 	bool "Broadcom driver debug functions"
 	depends on BRCMSMAC || BRCMFMAC
diff --git a/drivers/net/wireless/brcm80211/brcmsmac/brcms_trace_events.h b/drivers/net/wireless/brcm80211/brcmsmac/brcms_trace_events.h
index 27dd73e..bcf6969 100644
--- a/drivers/net/wireless/brcm80211/brcmsmac/brcms_trace_events.h
+++ b/drivers/net/wireless/brcm80211/brcmsmac/brcms_trace_events.h
@@ -24,7 +24,7 @@ 
 #include <linux/tracepoint.h>
 #include "mac80211_if.h"
 
-#ifndef CONFIG_BRCMDBG
+#ifndef CONFIG_BRCM_TRACING
 #undef TRACE_EVENT
 #define TRACE_EVENT(name, proto, ...) \
 static inline void trace_ ## name(proto) {}
@@ -80,7 +80,7 @@  TRACE_EVENT(brcms_dpc,
 
 #endif /* __TRACE_BRCMSMAC_H */
 
-#ifdef CONFIG_BRCMDBG
+#ifdef CONFIG_BRCM_TRACING
 
 #undef TRACE_INCLUDE_PATH
 #define TRACE_INCLUDE_PATH .
@@ -89,4 +89,4 @@  TRACE_EVENT(brcms_dpc,
 
 #include <trace/define_trace.h>
 
-#endif /* CONFIG_BRCMDBG */
+#endif /* CONFIG_BRCM_TRACING */