drm: Reduce object size of DRM_ERROR and DRM_DEBUG uses
diff mbox

Message ID 1b50f5d8-97a6-2442-34bb-2782c35505fd@linux.intel.com
State New
Headers show

Commit Message

Maarten Lankhorst March 15, 2018, 2:04 p.m. UTC
Op 15-03-18 om 14:30 schreef Ville Syrjälä:
> On Tue, Mar 13, 2018 at 03:02:15PM -0700, Joe Perches wrote:
>> drm_printk is used for both DRM_ERROR and DRM_DEBUG with unnecessary
>> arguments that can be removed by creating separate functins.
>>
>> Create specific functions for these calls to reduce x86/64 defconfig
>> size by ~20k.
>>
>> Modify the existing macros to use the specific calls.
>>
>> new:
>> $ size -t drivers/gpu/drm/built-in.a | tail -1
>> 1876562	  44542	    995	1922099	 1d5433	(TOTALS)
>>
>> old:
>> $ size -t drivers/gpu/drm/built-in.a | tail -1
>> 1897565	  44542	    995	1943102	 1da63e	(TOTALS)
>>
>> Miscellanea:
>>
>> o intel_display requires a change to use the specific calls.
> How much would we lose if we move the (drm_debug&FOO) outside the
> functions again? I'm somewhat concerned about all the function call
> overhead when debugs aren't even enabled.

Upstream:
   text    data     bss     dec     hex filename
 377143    5689    4352  387184   5e870 drivers/gpu/drm/drm.ko

With this patch:
 373831    5689    4352  383872   5db80 drivers/gpu/drm/drm.ko

Moving the if outside (below):
 377629    5689    4352  387670   5ea56 drivers/gpu/drm/drm.ko

Bye savings..

I don't think there are any places in which the debug output is performance sensitive,
so I'm ok with not inlining.
---

Comments

Ville Syrjala March 15, 2018, 3:05 p.m. UTC | #1
On Thu, Mar 15, 2018 at 03:04:52PM +0100, Maarten Lankhorst wrote:
> Op 15-03-18 om 14:30 schreef Ville Syrjälä:
> > On Tue, Mar 13, 2018 at 03:02:15PM -0700, Joe Perches wrote:
> >> drm_printk is used for both DRM_ERROR and DRM_DEBUG with unnecessary
> >> arguments that can be removed by creating separate functins.
> >>
> >> Create specific functions for these calls to reduce x86/64 defconfig
> >> size by ~20k.
> >>
> >> Modify the existing macros to use the specific calls.
> >>
> >> new:
> >> $ size -t drivers/gpu/drm/built-in.a | tail -1
> >> 1876562	  44542	    995	1922099	 1d5433	(TOTALS)
> >>
> >> old:
> >> $ size -t drivers/gpu/drm/built-in.a | tail -1
> >> 1897565	  44542	    995	1943102	 1da63e	(TOTALS)
> >>
> >> Miscellanea:
> >>
> >> o intel_display requires a change to use the specific calls.
> > How much would we lose if we move the (drm_debug&FOO) outside the
> > functions again? I'm somewhat concerned about all the function call
> > overhead when debugs aren't even enabled.
> 
> Upstream:
>    text    data     bss     dec     hex filename
>  377143    5689    4352  387184   5e870 drivers/gpu/drm/drm.ko
> 
> With this patch:
>  373831    5689    4352  383872   5db80 drivers/gpu/drm/drm.ko
> 
> Moving the if outside (below):
>  377629    5689    4352  387670   5ea56 drivers/gpu/drm/drm.ko
> 
> Bye savings..
> 
> I don't think there are any places in which the debug output is performance sensitive,
> so I'm ok with not inlining.

Not performance sensitive as such perhaps. But pointlessly wasting cpu
cycles for nop function calls isn't particularly great. Would be nice
to actually measure how much overhead there is on some weaker systems.
IIRC older Atoms were particularly bad at this stuff.

> ---
> diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
> index 79abf6d5b4db..928822403a59 100644
> --- a/drivers/gpu/drm/drm_print.c
> +++ b/drivers/gpu/drm/drm_print.c
> @@ -89,14 +89,11 @@ void drm_dev_printk(const struct device *dev, const char *level,
>  }
>  EXPORT_SYMBOL(drm_dev_printk);
>  
> -void drm_dbg(unsigned int category, const char *format, ...)
> +void __drm_dbg(const char *format, ...)
>  {
>  	struct va_format vaf;
>  	va_list args;
>  
> -	if (!(drm_debug & category))
> -		return;
> -
>  	va_start(args, format);
>  	vaf.fmt = format;
>  	vaf.va = &args;
> @@ -106,7 +103,7 @@ void drm_dbg(unsigned int category, const char *format, ...)
>  
>  	va_end(args);
>  }
> -EXPORT_SYMBOL(drm_dbg);
> +EXPORT_SYMBOL(__drm_dbg);
>  
>  void drm_err(const char *format, ...)
>  {
> diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
> index 3a40c5a3a5fa..2a145b97bdfc 100644
> --- a/include/drm/drm_print.h
> +++ b/include/drm/drm_print.h
> @@ -200,8 +200,17 @@ __printf(6, 7)
>  void drm_dev_printk(const struct device *dev, const char *level,
>  		    unsigned int category, const char *function_name,
>  		    const char *prefix, const char *format, ...);
> -__printf(2, 3)
> -void drm_dbg(unsigned int category, const char *format, ...);
> +
> +__printf(1, 2)
> +void __drm_dbg(const char *format, ...);
> +
> +
> +#define drm_dbg(category, format, ...) \
> +	do {	\
> +		if (drm_debug & category)	\
> +			__drm_dbg(format, ## __VA_ARGS__);	\
> +	} while (0)
> +
>  __printf(1, 2)
>  void drm_err(const char *format, ...);
>
Joe Perches March 15, 2018, 3:17 p.m. UTC | #2
On Thu, 2018-03-15 at 17:05 +0200, Ville Syrjälä wrote:
> On Thu, Mar 15, 2018 at 03:04:52PM +0100, Maarten Lankhorst wrote:
> > Op 15-03-18 om 14:30 schreef Ville Syrjälä:
> > > On Tue, Mar 13, 2018 at 03:02:15PM -0700, Joe Perches wrote:
> > > > drm_printk is used for both DRM_ERROR and DRM_DEBUG with unnecessary
> > > > arguments that can be removed by creating separate functins.
> > > > 
> > > > Create specific functions for these calls to reduce x86/64 defconfig
> > > > size by ~20k.
> > > > 
> > > > Modify the existing macros to use the specific calls.
> > > > 
> > > > new:
> > > > $ size -t drivers/gpu/drm/built-in.a | tail -1
> > > > 1876562	  44542	    995	1922099	 1d5433	(TOTALS)
> > > > 
> > > > old:
> > > > $ size -t drivers/gpu/drm/built-in.a | tail -1
> > > > 1897565	  44542	    995	1943102	 1da63e	(TOTALS)
> > > > 
> > > > Miscellanea:
> > > > 
> > > > o intel_display requires a change to use the specific calls.
> > > 
> > > How much would we lose if we move the (drm_debug&FOO) outside the
> > > functions again?

again?

> > >  I'm somewhat concerned about all the function call
> > > overhead when debugs aren't even enabled.

Perhaps better to have compilation elimination
of the entire debug output instead.

I think you are discussing a different issue and
this discussion should not block this patch as
this patch has no impact other than code size
reduction.
Ville Syrjala March 15, 2018, 3:37 p.m. UTC | #3
On Thu, Mar 15, 2018 at 08:17:53AM -0700, Joe Perches wrote:
> On Thu, 2018-03-15 at 17:05 +0200, Ville Syrjälä wrote:
> > On Thu, Mar 15, 2018 at 03:04:52PM +0100, Maarten Lankhorst wrote:
> > > Op 15-03-18 om 14:30 schreef Ville Syrjälä:
> > > > On Tue, Mar 13, 2018 at 03:02:15PM -0700, Joe Perches wrote:
> > > > > drm_printk is used for both DRM_ERROR and DRM_DEBUG with unnecessary
> > > > > arguments that can be removed by creating separate functins.
> > > > > 
> > > > > Create specific functions for these calls to reduce x86/64 defconfig
> > > > > size by ~20k.
> > > > > 
> > > > > Modify the existing macros to use the specific calls.
> > > > > 
> > > > > new:
> > > > > $ size -t drivers/gpu/drm/built-in.a | tail -1
> > > > > 1876562	  44542	    995	1922099	 1d5433	(TOTALS)
> > > > > 
> > > > > old:
> > > > > $ size -t drivers/gpu/drm/built-in.a | tail -1
> > > > > 1897565	  44542	    995	1943102	 1da63e	(TOTALS)
> > > > > 
> > > > > Miscellanea:
> > > > > 
> > > > > o intel_display requires a change to use the specific calls.
> > > > 
> > > > How much would we lose if we move the (drm_debug&FOO) outside the
> > > > functions again?
> 
> again?

We used to do that. Someone changed it a while back, unintentially
I believe.

> 
> > > >  I'm somewhat concerned about all the function call
> > > > overhead when debugs aren't even enabled.
> 
> Perhaps better to have compilation elimination
> of the entire debug output instead.

That would require every bug reporter to recompile the kernel first.
So this is not a solution we would ever seriously consider.

Not sure if it would be possible to use the alternatives thing to
eliminate the function calls unless the user boots wih drm.debug!=0?

> 
> I think you are discussing a different issue and
> this discussion should not block this patch as
> this patch has no impact other than code size
> reduction.

But what is the goal of the code size reduction? I assume the main
goal is to make better use of the instruction cache to make the
code faster. If there's a tradeoff between smaller and slightly
faster vs. larger and a singificantly faster I tend to think we
should go for the latter option.
Joe Perches March 15, 2018, 3:44 p.m. UTC | #4
On Thu, 2018-03-15 at 17:37 +0200, Ville Syrjälä wrote:
> On Thu, Mar 15, 2018 at 08:17:53AM -0700, Joe Perches wrote:
> > On Thu, 2018-03-15 at 17:05 +0200, Ville Syrjälä wrote:
> > > On Thu, Mar 15, 2018 at 03:04:52PM +0100, Maarten Lankhorst wrote:
> > > > Op 15-03-18 om 14:30 schreef Ville Syrjälä:
> > > > > On Tue, Mar 13, 2018 at 03:02:15PM -0700, Joe Perches wrote:
> > > > > > drm_printk is used for both DRM_ERROR and DRM_DEBUG with unnecessary
> > > > > > arguments that can be removed by creating separate functins.
> > > > > > 
> > > > > > Create specific functions for these calls to reduce x86/64 defconfig
> > > > > > size by ~20k.
> > > > > > 
> > > > > > Modify the existing macros to use the specific calls.
> > > > > > 
> > > > > > new:
> > > > > > $ size -t drivers/gpu/drm/built-in.a | tail -1
> > > > > > 1876562	  44542	    995	1922099	 1d5433	(TOTALS)
> > > > > > 
> > > > > > old:
> > > > > > $ size -t drivers/gpu/drm/built-in.a | tail -1
> > > > > > 1897565	  44542	    995	1943102	 1da63e	(TOTALS)
> > > > > > 
> > > > > > Miscellanea:
> > > > > > 
> > > > > > o intel_display requires a change to use the specific calls.
> > > > > 
> > > > > How much would we lose if we move the (drm_debug&FOO) outside the
> > > > > functions again?
> > 
> > again?
> 
> We used to do that. Someone changed it a while back, unintentially
> I believe.
> 
> > 
> > > > >  I'm somewhat concerned about all the function call
> > > > > overhead when debugs aren't even enabled.
> > 
> > Perhaps better to have compilation elimination
> > of the entire debug output instead.
> 
> That would require every bug reporter to recompile the kernel first.
> So this is not a solution we would ever seriously consider.
> 
> Not sure if it would be possible to use the alternatives thing to
> eliminate the function calls unless the user boots wih drm.debug!=0?
> 
> > 
> > I think you are discussing a different issue and
> > this discussion should not block this patch as
> > this patch has no impact other than code size
> > reduction.
> 
> But what is the goal of the code size reduction?

Smaller code.

> I assume the main
> goal is to make better use of the instruction cache to make the
> code faster. If there's a tradeoff between smaller and slightly
> faster vs. larger and a singificantly faster I tend to think we
> should go for the latter option.

There's no trade-off in this patch for faster/larger.
This patch is simply smaller.  Smaller is better.

Your faster/larger should be a different patch proposal.
Ville Syrjala March 15, 2018, 4:14 p.m. UTC | #5
On Thu, Mar 15, 2018 at 08:44:05AM -0700, Joe Perches wrote:
> On Thu, 2018-03-15 at 17:37 +0200, Ville Syrjälä wrote:
> > On Thu, Mar 15, 2018 at 08:17:53AM -0700, Joe Perches wrote:
> > > On Thu, 2018-03-15 at 17:05 +0200, Ville Syrjälä wrote:
> > > > On Thu, Mar 15, 2018 at 03:04:52PM +0100, Maarten Lankhorst wrote:
> > > > > Op 15-03-18 om 14:30 schreef Ville Syrjälä:
> > > > > > On Tue, Mar 13, 2018 at 03:02:15PM -0700, Joe Perches wrote:
> > > > > > > drm_printk is used for both DRM_ERROR and DRM_DEBUG with unnecessary
> > > > > > > arguments that can be removed by creating separate functins.
> > > > > > > 
> > > > > > > Create specific functions for these calls to reduce x86/64 defconfig
> > > > > > > size by ~20k.
> > > > > > > 
> > > > > > > Modify the existing macros to use the specific calls.
> > > > > > > 
> > > > > > > new:
> > > > > > > $ size -t drivers/gpu/drm/built-in.a | tail -1
> > > > > > > 1876562	  44542	    995	1922099	 1d5433	(TOTALS)
> > > > > > > 
> > > > > > > old:
> > > > > > > $ size -t drivers/gpu/drm/built-in.a | tail -1
> > > > > > > 1897565	  44542	    995	1943102	 1da63e	(TOTALS)
> > > > > > > 
> > > > > > > Miscellanea:
> > > > > > > 
> > > > > > > o intel_display requires a change to use the specific calls.
> > > > > > 
> > > > > > How much would we lose if we move the (drm_debug&FOO) outside the
> > > > > > functions again?
> > > 
> > > again?
> > 
> > We used to do that. Someone changed it a while back, unintentially
> > I believe.
> > 
> > > 
> > > > > >  I'm somewhat concerned about all the function call
> > > > > > overhead when debugs aren't even enabled.
> > > 
> > > Perhaps better to have compilation elimination
> > > of the entire debug output instead.
> > 
> > That would require every bug reporter to recompile the kernel first.
> > So this is not a solution we would ever seriously consider.
> > 
> > Not sure if it would be possible to use the alternatives thing to
> > eliminate the function calls unless the user boots wih drm.debug!=0?
> > 
> > > 
> > > I think you are discussing a different issue and
> > > this discussion should not block this patch as
> > > this patch has no impact other than code size
> > > reduction.
> > 
> > But what is the goal of the code size reduction?
> 
> Smaller code.
> 
> > I assume the main
> > goal is to make better use of the instruction cache to make the
> > code faster. If there's a tradeoff between smaller and slightly
> > faster vs. larger and a singificantly faster I tend to think we
> > should go for the latter option.
> 
> There's no trade-off in this patch for faster/larger.
> This patch is simply smaller.  Smaller is better.

This feels a bit like saying pink is better than red because it's
more pink.

That said, I'm not arguing against this patch as such. Making things
smaller "just because" usually doesn't cause problems. But I was
hoping that we might be after some more tangible gains here, and
thus pointed out that there may be a better way to achieve even
bigger gains.
Joe Perches March 15, 2018, 4:29 p.m. UTC | #6
On Thu, 2018-03-15 at 18:14 +0200, Ville Syrjälä wrote:
> > There's no trade-off in this patch for faster/larger.
> > This patch is simply smaller.  Smaller is better.
> 
> This feels a bit like saying pink is better than red because it's
> more pink.

Silly.  If you can't say smaller total object code that
performs the same task identically is better, I think
we can't discuss much of anything about code together.

Any printk related mechanism is not fast-path so any
icache dilution isn't an issue.

> That said, I'm not arguing against this patch as such. Making things
> smaller "just because" usually doesn't cause problems.

It seems more like you haven't read the patch.

>  But I was
> hoping that we might be after some more tangible gains here, and
> thus pointed out that there may be a better way to achieve even
> bigger gains.

Sure, it's just any such a discussion should not affect
this patch being applied.

This patch reduces the argument count of the drm_printk
(now drm_dbg) call and so is faster to execute even if
the emit test is internal to the drm_dbg function.

Patch
diff mbox

diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
index 79abf6d5b4db..928822403a59 100644
--- a/drivers/gpu/drm/drm_print.c
+++ b/drivers/gpu/drm/drm_print.c
@@ -89,14 +89,11 @@  void drm_dev_printk(const struct device *dev, const char *level,
 }
 EXPORT_SYMBOL(drm_dev_printk);
 
-void drm_dbg(unsigned int category, const char *format, ...)
+void __drm_dbg(const char *format, ...)
 {
 	struct va_format vaf;
 	va_list args;
 
-	if (!(drm_debug & category))
-		return;
-
 	va_start(args, format);
 	vaf.fmt = format;
 	vaf.va = &args;
@@ -106,7 +103,7 @@  void drm_dbg(unsigned int category, const char *format, ...)
 
 	va_end(args);
 }
-EXPORT_SYMBOL(drm_dbg);
+EXPORT_SYMBOL(__drm_dbg);
 
 void drm_err(const char *format, ...)
 {
diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
index 3a40c5a3a5fa..2a145b97bdfc 100644
--- a/include/drm/drm_print.h
+++ b/include/drm/drm_print.h
@@ -200,8 +200,17 @@  __printf(6, 7)
 void drm_dev_printk(const struct device *dev, const char *level,
 		    unsigned int category, const char *function_name,
 		    const char *prefix, const char *format, ...);
-__printf(2, 3)
-void drm_dbg(unsigned int category, const char *format, ...);
+
+__printf(1, 2)
+void __drm_dbg(const char *format, ...);
+
+
+#define drm_dbg(category, format, ...) \
+	do {	\
+		if (drm_debug & category)	\
+			__drm_dbg(format, ## __VA_ARGS__);	\
+	} while (0)
+
 __printf(1, 2)
 void drm_err(const char *format, ...);