[PATCH] add macro for printing mac addresses

[PATCH] add macro for printing mac addresses

[Bruno Randolf]

is there any chance to include a macro like this for printing mac addresses?

its advantage is that it can be used without the need to declare buffers for
print_mac(), for example:

printk("mac address: " MAC_FMT, MAC_ADDR(addr));

Signed-off-by: Bruno Randolf <bruno-L9ZBdB2wSWtl57MIdRCFDg@public.gmane.org>
---

 include/linux/if_ether.h |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)


diff --git a/include/linux/if_ether.h b/include/linux/if_ether.h
index 5f92977..b9a6fb2 100644
--- a/include/linux/if_ether.h
+++ b/include/linux/if_ether.h
@@ -127,6 +127,15 @@ extern struct ctl_table ether_table[];
  *	Display a 6 byte device address (MAC) in a readable format.
  */
 #define MAC_FMT "%02x:%02x:%02x:%02x:%02x:%02x"
+
+#define MAC_ADDR(addr) \
+    ((unsigned char *)(addr))[0], \
+    ((unsigned char *)(addr))[1], \
+    ((unsigned char *)(addr))[2], \
+    ((unsigned char *)(addr))[3], \
+    ((unsigned char *)(addr))[4], \
+    ((unsigned char *)(addr))[5]
+
 extern char *print_mac(char *buf, const u8 *addr);
 #define DECLARE_MAC_BUF(var) char var[18] __maybe_unused
 

Re: [PATCH] add macro for printing mac addresses

[David Miller]

From: Bruno Randolf <bruno@thinktube.com>
Date: Fri, 15 Feb 2008 19:48:05 +0900

> is there any chance to include a macro like this for printing mac addresses?
> 
> its advantage is that it can be used without the need to declare buffers for
> print_mac(), for example:
> 
> printk("mac address: " MAC_FMT, MAC_ADDR(addr));
> 
> Signed-off-by: Bruno Randolf <bruno@thinktube.com>

We specifically removed this sort of thing, please don't
add it back.

Thank you.

Re: [PATCH] add macro for printing mac addresses

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 344 bytes --]


On Fri, 2008-02-15 at 19:48 +0900, Bruno Randolf wrote:
> is there any chance to include a macro like this for printing mac addresses?
> 
> its advantage is that it can be used without the need to declare buffers for
> print_mac(), for example:
> 
> printk("mac address: " MAC_FMT, MAC_ADDR(addr));

you want print_mac()

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

[PATCH] net/8021q/vlan_dev.c - Use print_mac

[Joe Perches]

On Fri, 2008-02-15 at 02:58 -0800, David Miller wrote:
> From: Bruno Randolf <bruno-L9ZBdB2wSWtl57MIdRCFDg@public.gmane.org>
> Date: Fri, 15 Feb 2008 19:48:05 +0900
> > is there any chance to include a macro like this for printing mac
> addresses?
> > its advantage is that it can be used without the need to declare
> buffers for
> > print_mac(), for example:
> We specifically removed this sort of thing, please don't
> add it back.

Remove direct use of MAC_FMT

Signed-off-by: Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org>

diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index 77f04e4..839c70e 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -366,7 +366,8 @@ static int vlan_dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct net_device_stats *stats = &dev->stats;
 	struct vlan_ethhdr *veth = (struct vlan_ethhdr *)(skb->data);
-
+	DECLARE_MAC_BUF(mac);
+	DECLARE_MAC_BUF(mac2);
 	/* Handle non-VLAN frames if they are sent to us, for example by DHCP.
 	 *
 	 * NOTE: THIS ASSUMES DIX ETHERNET, SPECIFICALLY NOT SUPPORTING
@@ -404,11 +405,8 @@ static int vlan_dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	pr_debug("%s: about to send skb: %p to dev: %s\n",
 		__FUNCTION__, skb, skb->dev->name);
-	pr_debug("  " MAC_FMT " " MAC_FMT " %4hx %4hx %4hx\n",
-		 veth->h_dest[0], veth->h_dest[1], veth->h_dest[2],
-		 veth->h_dest[3], veth->h_dest[4], veth->h_dest[5],
-		 veth->h_source[0], veth->h_source[1], veth->h_source[2],
-		 veth->h_source[3], veth->h_source[4], veth->h_source[5],
+	pr_debug("  %s %s %4hx %4hx %4hx\n",
+		 print_mac(mac, veth->h_dest), print_mac(mac2, veth->h_source),
 		 veth->h_vlan_proto, veth->h_vlan_TCI,
 		 veth->h_vlan_encapsulated_proto);
 

[PATCH 2/2] Remove MAC_FMT

[Joe Perches]

MAC_FMT is no longer used

Signed-off-by: Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org>

diff --git a/include/linux/if_ether.h b/include/linux/if_ether.h
index e157c13..7a1e011 100644
--- a/include/linux/if_ether.h
+++ b/include/linux/if_ether.h
@@ -130,7 +130,6 @@ extern ssize_t sysfs_format_mac(char *buf, const unsigned char *addr, int len);
  *	Display a 6 byte device address (MAC) in a readable format.
  */
 extern char *print_mac(char *buf, const unsigned char *addr);
-#define MAC_FMT "%02x:%02x:%02x:%02x:%02x:%02x"
 #define MAC_BUF_SIZE	18
 #define DECLARE_MAC_BUF(var) char var[MAC_BUF_SIZE] __maybe_unused
 

Re: [PATCH] net/8021q/vlan_dev.c - Use print_mac

[David Miller]

From: Joe Perches <joe@perches.com>
Date: Fri, 15 Feb 2008 09:42:50 -0800

> On Fri, 2008-02-15 at 02:58 -0800, David Miller wrote:
> > From: Bruno Randolf <bruno@thinktube.com>
> > Date: Fri, 15 Feb 2008 19:48:05 +0900
> > > is there any chance to include a macro like this for printing mac
> > addresses?
> > > its advantage is that it can be used without the need to declare
> > buffers for
> > > print_mac(), for example:
> > We specifically removed this sort of thing, please don't
> > add it back.
> 
> Remove direct use of MAC_FMT
> 
> Signed-off-by: Joe Perches <joe@perches.com>

Applied, thanks.

Re: [PATCH 2/2] Remove MAC_FMT

[David Miller]

From: Joe Perches <joe@perches.com>
Date: Fri, 15 Feb 2008 09:42:55 -0800

> MAC_FMT is no longer used
> 
> Signed-off-by: Joe Perches <joe@perches.com>

Applied, thanks.

Re: [PATCH] net/8021q/vlan_dev.c - Use print_mac

[Patrick McHardy]

Joe Perches wrote:
> On Fri, 2008-02-15 at 02:58 -0800, David Miller wrote:
>> From: Bruno Randolf <bruno@thinktube.com>
>> Date: Fri, 15 Feb 2008 19:48:05 +0900
>>> is there any chance to include a macro like this for printing mac
>> addresses?
>>> its advantage is that it can be used without the need to declare
>> buffers for
>>> print_mac(), for example:
>> We specifically removed this sort of thing, please don't
>> add it back.

Why?

> @@ -404,11 +405,8 @@ static int vlan_dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  
>  	pr_debug("%s: about to send skb: %p to dev: %s\n",
>  		__FUNCTION__, skb, skb->dev->name);
> -	pr_debug("  " MAC_FMT " " MAC_FMT " %4hx %4hx %4hx\n",
> -		 veth->h_dest[0], veth->h_dest[1], veth->h_dest[2],
> -		 veth->h_dest[3], veth->h_dest[4], veth->h_dest[5],
> -		 veth->h_source[0], veth->h_source[1], veth->h_source[2],
> -		 veth->h_source[3], veth->h_source[4], veth->h_source[5],
> +	pr_debug("  %s %s %4hx %4hx %4hx\n",
> +		 print_mac(mac, veth->h_dest), print_mac(mac2, veth->h_source),


This results in print_mac getting called twice per packet even without
debugging. Whats the problem with MAC_FMT?

Re: [PATCH] net/8021q/vlan_dev.c - Use print_mac

[Joe Perches]

On Mon, 2008-02-18 at 16:19 +0100, Patrick McHardy wrote:
> > @@ -404,11 +405,8 @@ static int vlan_dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev)
> >  
> >  	pr_debug("%s: about to send skb: %p to dev: %s\n",
> >  		__FUNCTION__, skb, skb->dev->name);
> > -	pr_debug("  " MAC_FMT " " MAC_FMT " %4hx %4hx %4hx\n",
> > -		 veth->h_dest[0], veth->h_dest[1], veth->h_dest[2],
> > -		 veth->h_dest[3], veth->h_dest[4], veth->h_dest[5],
> > -		 veth->h_source[0], veth->h_source[1], veth->h_source[2],
> > -		 veth->h_source[3], veth->h_source[4], veth->h_source[5],
> > +	pr_debug("  %s %s %4hx %4hx %4hx\n",
> > +		 print_mac(mac, veth->h_dest), print_mac(mac2, veth->h_source),
> This results in print_mac getting called twice per packet even without
> debugging. Whats the problem with MAC_FMT?

It's just a consistency thing.
It identifies code where MAC addresses are used.
an allyesconfig is a bit smaller (~.1%).
pr_debug is a noop when not debugging, print_mac is optimized away.

Re: [PATCH] net/8021q/vlan_dev.c - Use print_mac

[David Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Mon, 18 Feb 2008 16:19:40 +0100

> Joe Perches wrote:
> > On Fri, 2008-02-15 at 02:58 -0800, David Miller wrote:
> >> From: Bruno Randolf <bruno@thinktube.com>
> >> Date: Fri, 15 Feb 2008 19:48:05 +0900
> >>> is there any chance to include a macro like this for printing mac
> >> addresses?
> >>> its advantage is that it can be used without the need to declare
> >> buffers for
> >>> print_mac(), for example:
> >> We specifically removed this sort of thing, please don't
> >> add it back.
> 
> Why?

We converted the entire tree over the print_mac(), and since
the MAC_FMT stuff was therefore no longer used we could
remove it.

Some references slipped back in somehow, and thus MAC_FMT
did too.

There is no reason to keep around a global interface for
_one_ user when that user can use the recommended interface
just as equally as the rest of the tree which we converted.

This is a pr_debug() statement we're talking about here.
:-)

Re: [PATCH] net/8021q/vlan_dev.c - Use print_mac

[Patrick McHardy]

David Miller wrote:
> From: Patrick McHardy <kaber-dcUjhNyLwpNeoWH0uzbU5w@public.gmane.org>
> Date: Mon, 18 Feb 2008 16:19:40 +0100
>
>   
>> Joe Perches wrote:
>>     
>>>
>>>> We specifically removed this sort of thing, please don't
>>>> add it back.
>>>>         
>> Why?
>>     
>
> We converted the entire tree over the print_mac(), and since
> the MAC_FMT stuff was therefore no longer used we could
> remove it.
>
> Some references slipped back in somehow, and thus MAC_FMT
> did too.
>
> There is no reason to keep around a global interface for
> _one_ user when that user can use the recommended interface
> just as equally as the rest of the tree which we converted.
>
> This is a pr_debug() statement we're talking about here.
> :-)
>   

The way pr_debug is implemented it still results in two function
calls per packet since the compiler doesn't know that it doesn't
have visible side-effects besides modifying the (unused) buffer.
I confirmed this using codiff.

Re: [PATCH] net/8021q/vlan_dev.c - Use print_mac

[Patrick McHardy]

Joe Perches wrote:
> On Mon, 2008-02-18 at 16:19 +0100, Patrick McHardy wrote:
>   
>>> @@ -404,11 +405,8 @@ static int vlan_dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev)
>>>  
>>>  	pr_debug("%s: about to send skb: %p to dev: %s\n",
>>>  		__FUNCTION__, skb, skb->dev->name);
>>> -	pr_debug("  " MAC_FMT " " MAC_FMT " %4hx %4hx %4hx\n",
>>> -		 veth->h_dest[0], veth->h_dest[1], veth->h_dest[2],
>>> -		 veth->h_dest[3], veth->h_dest[4], veth->h_dest[5],
>>> -		 veth->h_source[0], veth->h_source[1], veth->h_source[2],
>>> -		 veth->h_source[3], veth->h_source[4], veth->h_source[5],
>>> +	pr_debug("  %s %s %4hx %4hx %4hx\n",
>>> +		 print_mac(mac, veth->h_dest), print_mac(mac2, veth->h_source),
>>>       
>> This results in print_mac getting called twice per packet even without
>> debugging. Whats the problem with MAC_FMT?
>>     
>
> It's just a consistency thing.
> It identifies code where MAC addresses are used.
>   
> an allyesconfig is a bit smaller (~.1%).
> pr_debug is a noop when not debugging, print_mac is optimized away.
>   

No its not, which I also stated in the commit message that restored
it.

0x0000000060244313 <vlan_dev_hard_start_xmit+433>:      callq  
0x60161dbd <print_mac>
0x0000000060244318 <vlan_dev_hard_start_xmit+438>:      lea    
-0x50(%rbp),%rdi
0x000000006024431c <vlan_dev_hard_start_xmit+442>:      mov    %r15,%rsi
0x000000006024431f <vlan_dev_hard_start_xmit+445>:      callq  
0x60161dbd <print_mac>

Re: [PATCH] net/8021q/vlan_dev.c - Use print_mac

[David Miller]

From: Patrick McHardy <kaber-dcUjhNyLwpNeoWH0uzbU5w@public.gmane.org>
Date: Mon, 18 Feb 2008 22:17:27 +0100

> The way pr_debug is implemented it still results in two function
> calls per packet since the compiler doesn't know that it doesn't
> have visible side-effects besides modifying the (unused) buffer.
> I confirmed this using codiff.

That's a bug.

I think we can fix this easily by using __attribute_const_
on the print_mac() declaration.  Let me play with that.

Re: [PATCH] net/8021q/vlan_dev.c - Use print_mac

[David Miller]

From: David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
Date: Mon, 18 Feb 2008 16:43:05 -0800 (PST)

> I think we can fix this easily by using __attribute_const_
> on the print_mac() declaration.  Let me play with that.

Actually it seems the 'pure' attribute is more important
here.  Although it's not semantically a perfect match,
what we need to tell the compiler is basically that:

1) the return value depends upon the inputs
2) if the input is not used, it's safe to avoid the call

and 'pure' accomplishes that without any unwanted side-effects.

I think this will not result in any unwanted over-optimization.
Because if the inputs change in any way GCC has to emit the
call.

Any objections?

commit 8f789c48448aed74fe1c07af76de8f04adacec7d
Author: David S. Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
Date:   Mon Feb 18 16:50:22 2008 -0800

    [NET]: Elminate spurious print_mac() calls.
    
    Patrick McHardy notes that print_mac() can get invoked
    even if the result it unused (f.e. as an argument to
    pr_debug() when DEBUG is not defined).
    
    Mark this function as "__pure" to eliminate this problem.
    
    Signed-off-by: David S. Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>

diff --git a/include/linux/if_ether.h b/include/linux/if_ether.h
index 7a1e011..42dc6a3 100644
--- a/include/linux/if_ether.h
+++ b/include/linux/if_ether.h
@@ -129,7 +129,7 @@ extern ssize_t sysfs_format_mac(char *buf, const unsigned char *addr, int len);
 /*
  *	Display a 6 byte device address (MAC) in a readable format.
  */
-extern char *print_mac(char *buf, const unsigned char *addr);
+extern __pure char *print_mac(char *buf, const unsigned char *addr);
 #define MAC_BUF_SIZE	18
 #define DECLARE_MAC_BUF(var) char var[MAC_BUF_SIZE] __maybe_unused
 

Re: [PATCH] net/8021q/vlan_dev.c - Use print_mac

[Joe Perches]

On Mon, 2008-02-18 at 16:50 -0800, David Miller wrote:
> Actually it seems the 'pure' attribute is more important
> here.  Although it's not semantically a perfect match,
> what we need to tell the compiler is basically that:
> 
> 1) the return value depends upon the inputs
> 2) if the input is not used, it's safe to avoid the call
> 
> and 'pure' accomplishes that without any unwanted side-effects.
> 
> I think this will not result in any unwanted over-optimization.
> Because if the inputs change in any way GCC has to emit the
> call.
> 
> Any objections?

Does this need to be done for all function calls
declared with __attribute__((format(printf, x, y)))
{
return 0;
}

ie: pr_debug, dev_dbg, dev_vdbg?

Perhaps it's more sensible to go back to

#ifdef DEBUG
#define pr_debug(fmt, arg...) do {} while (0)
#endif

and give up the printf argument verification

Re: [PATCH] net/8021q/vlan_dev.c - Use print_mac

[Philip Craig]

Joe Perches wrote:
> Perhaps it's more sensible to go back to
> 
> #ifdef DEBUG
> #define pr_debug(fmt, arg...) do {} while (0)
> #endif
> 
> and give up the printf argument verification

I think argument verification is important.   Can you keep it
like this:

#ifdef DEBUG
#define pr_debug(fmt, arg...) \
	do { \
		if (0) \
			printk(KERN_DEBUG fmt, ##arg); \
	} while (0)
#endif

We still lose the return value though, I'm not sure how to keep that
safely in macros.

But does anything rely on the side effects already?  This would
introduce bugs if so.

Re: [PATCH] net/8021q/vlan_dev.c - Use print_mac

[David Miller]

From: Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org>
Date: Mon, 18 Feb 2008 17:03:32 -0800

> Does this need to be done for all function calls
> declared with __attribute__((format(printf, x, y)))
> {
> return 0;
> }
> 
> ie: pr_debug, dev_dbg, dev_vdbg?

No, I don't think so.

We're adding the tag to teach the compiler that if the
return value isn't used, it is OK not to emit the call
altogether.

Re: [PATCH] net/8021q/vlan_dev.c - Use print_mac

[Patrick McHardy]

David Miller wrote:
> From: David Miller <davem@davemloft.net>
> Date: Mon, 18 Feb 2008 16:43:05 -0800 (PST)
> 
>> I think we can fix this easily by using __attribute_const_
>> on the print_mac() declaration.  Let me play with that.
> 
> Actually it seems the 'pure' attribute is more important
> here.  Although it's not semantically a perfect match,
> what we need to tell the compiler is basically that:
> 
> 1) the return value depends upon the inputs
> 2) if the input is not used, it's safe to avoid the call
> 
> and 'pure' accomplishes that without any unwanted side-effects.
> 
> I think this will not result in any unwanted over-optimization.
> Because if the inputs change in any way GCC has to emit the
> call.
> 
> Any objections?


This seems fine to me, thanks Dave.