Re: [PATCH] net/8021q/vlan_dev.c - Use print_mac
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. -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] net/8021q/vlan_dev.c - Use print_mac
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? -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] net/8021q/vlan_dev.c - Use print_mac
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. -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] net/8021q/vlan_dev.c - Use print_mac
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. -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] net/8021q/vlan_dev.c - Use print_mac
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. :-) -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] net/8021q/vlan_dev.c - Use print_mac
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. 0x60244313 vlan_dev_hard_start_xmit+433: callq 0x60161dbd print_mac 0x60244318 vlan_dev_hard_start_xmit+438: lea -0x50(%rbp),%rdi 0x6024431c vlan_dev_hard_start_xmit+442: mov%r15,%rsi 0x6024431f vlan_dev_hard_start_xmit+445: callq 0x60161dbd print_mac -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] net/8021q/vlan_dev.c - Use print_mac
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 -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] net/8021q/vlan_dev.c - Use print_mac
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
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] net/8021q/vlan_dev.c - Use print_mac
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. -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] net/8021q/vlan_dev.c - Use print_mac
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.
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] net/8021q/vlan_dev.c - Use print_mac
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.
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] net/8021q/vlan_dev.c - Use print_mac
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. -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] net/8021q/vlan_dev.c - Use 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);
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
