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 <[email protected]>
---
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
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
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.
MAC_FMT is no longer used
Signed-off-by: Joe Perches <[email protected]>
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
From: Joe Perches <[email protected]>
Date: Fri, 15 Feb 2008 09:42:50 -0800
> On Fri, 2008-02-15 at 02:58 -0800, David Miller wrote:
> > From: Bruno Randolf <[email protected]>
> > 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 <[email protected]>
Applied, thanks.
From: Bruno Randolf <[email protected]>
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 <[email protected]>
We specifically removed this sort of thing, please don't
add it back.
Thank you.
From: Joe Perches <[email protected]>
Date: Fri, 15 Feb 2008 09:42:55 -0800
> MAC_FMT is no longer used
>
> Signed-off-by: Joe Perches <[email protected]>
Applied, thanks.
From: David Miller <[email protected]>
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 <[email protected]>
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 <[email protected]>
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
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>
On Fri, 2008-02-15 at 02:58 -0800, David Miller wrote:
> From: Bruno Randolf <[email protected]>
> 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 <[email protected]>
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);
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
David Miller wrote:
> From: David Miller <[email protected]>
> 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.
From: Patrick McHardy <[email protected]>
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.
From: Patrick McHardy <[email protected]>
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 <[email protected]>
> >> 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.
:-)
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.
Joe Perches wrote:
> On Fri, 2008-02-15 at 02:58 -0800, David Miller wrote:
>> From: Bruno Randolf <[email protected]>
>> 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?
From: Joe Perches <[email protected]>
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.
David Miller wrote:
> From: Patrick McHardy <[email protected]>
> 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.