[PATCH net-next] ipv4: use a 64bit load/store in output path

[PATCH net-next] ipv4: use a 64bit load/store in output path

[Eric Dumazet]

gcc compiler is smart enough to use a single load/store if we
memcpy(dptr, sptr, 8) on x86_64, regardless of
CONFIG_CC_OPTIMIZE_FOR_SIZE
    
In IP header, daddr immediately follows saddr, this wont change in the
future. We only need to make sure our flowi4 (saddr,daddr) fields wont
break the rule.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
Next step is to speedup route lookup in input path

 include/net/flow.h   |    5 ++++-
 net/ipv4/ip_output.c |   21 +++++++++++++++++----
 2 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/include/net/flow.h b/include/net/flow.h
index a094477..9192d69 100644
--- a/include/net/flow.h
+++ b/include/net/flow.h
@@ -59,8 +59,11 @@ struct flowi4 {
 #define flowi4_proto		__fl_common.flowic_proto
 #define flowi4_flags		__fl_common.flowic_flags
 #define flowi4_secid		__fl_common.flowic_secid
-	__be32			daddr;
+
+	/* (saddr,daddr) must be grouped, same order as in IP header */
 	__be32			saddr;
+	__be32			daddr;
+
 	union flowi_uli		uli;
 #define fl4_sport		uli.ports.sport
 #define fl4_dport		uli.ports.dport
diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index 0bc95f3..0d5e567 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -319,6 +319,20 @@ int ip_output(struct sk_buff *skb)
 			    !(IPCB(skb)->flags & IPSKB_REROUTED));
 }
 
+/*
+ * copy saddr and daddr, possibly using 64bit load/stores
+ * Equivalent to :
+ *   iph->saddr = fl4->saddr;
+ *   iph->daddr = fl4->daddr;
+ */
+static void ip_copy_addrs(struct iphdr *iph, const struct flowi4 *fl4)
+{
+	BUILD_BUG_ON(offsetof(typeof(*fl4), daddr) !=
+		     offsetof(typeof(*fl4), saddr) + sizeof(fl4->saddr));
+	memcpy(&iph->saddr, &fl4->saddr,
+	       sizeof(fl4->saddr) + sizeof(fl4->daddr));
+}
+
 int ip_queue_xmit(struct sk_buff *skb, struct flowi *fl)
 {
 	struct sock *sk = skb->sk;
@@ -381,8 +395,8 @@ packet_routed:
 		iph->frag_off = 0;
 	iph->ttl      = ip_select_ttl(inet, &rt->dst);
 	iph->protocol = sk->sk_protocol;
-	iph->saddr    = fl4->saddr;
-	iph->daddr    = fl4->daddr;
+	ip_copy_addrs(iph, fl4);
+
 	/* Transport layer set skb->h.foo itself. */
 
 	if (inet_opt && inet_opt->opt.optlen) {
@@ -1337,8 +1351,7 @@ struct sk_buff *__ip_make_skb(struct sock *sk,
 	ip_select_ident(iph, &rt->dst, sk);
 	iph->ttl = ttl;
 	iph->protocol = sk->sk_protocol;
-	iph->saddr = fl4->saddr;
-	iph->daddr = fl4->daddr;
+	ip_copy_addrs(iph, fl4);
 
 	if (opt) {
 		iph->ihl += opt->optlen>>2;

Re: [PATCH net-next] ipv4: use a 64bit load/store in output path

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 01 Dec 2011 06:00:53 +0100

> gcc compiler is smart enough to use a single load/store if we
> memcpy(dptr, sptr, 8) on x86_64, regardless of
> CONFIG_CC_OPTIMIZE_FOR_SIZE
>     
> In IP header, daddr immediately follows saddr, this wont change in the
> future. We only need to make sure our flowi4 (saddr,daddr) fields wont
> break the rule.
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>

Hmmm, this triggers a strange new build warning:

net/dccp/ipv4.c: In function ‘dccp_v4_route_skb’:
net/dccp/ipv4.c:481:3: warning: initialized field with side-effects overwritten [enabled by default]
net/dccp/ipv4.c:481:3: warning: (near initialization for ‘fl4’) [enabled by default]

Re: [PATCH net-next] ipv4: use a 64bit load/store in output path

[Eric Dumazet]

Le jeudi 01 décembre 2011 à 01:48 -0500, David Miller a écrit :
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Thu, 01 Dec 2011 06:00:53 +0100
> 
> > gcc compiler is smart enough to use a single load/store if we
> > memcpy(dptr, sptr, 8) on x86_64, regardless of
> > CONFIG_CC_OPTIMIZE_FOR_SIZE
> >     
> > In IP header, daddr immediately follows saddr, this wont change in the
> > future. We only need to make sure our flowi4 (saddr,daddr) fields wont
> > break the rule.
> > 
> > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> 
> Hmmm, this triggers a strange new build warning:
> 
> net/dccp/ipv4.c: In function ‘dccp_v4_route_skb’:
> net/dccp/ipv4.c:481:3: warning: initialized field with side-effects overwritten [enabled by default]
> net/dccp/ipv4.c:481:3: warning: (near initialization for ‘fl4’) [enabled by default]

Hmm, seems a compiler bug, since following fixes the warning ?

[PATCH net-next] dccp: fix a compiler warning

net/dccp/ipv4.c: In function ‘dccp_v4_route_skb’:
net/dccp/ipv4.c:481:3: warning: initialized field with
	side-effects overwritten [enabled by default]
net/dccp/ipv4.c:481:3: warning: (near initialization
	for ‘fl4’) [enabled by default]


Reported-by: David Miller <davem@davemloft.net>
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
 net/dccp/ipv4.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/dccp/ipv4.c b/net/dccp/ipv4.c
index 3f4e541..1c67fe8 100644
--- a/net/dccp/ipv4.c
+++ b/net/dccp/ipv4.c
@@ -474,10 +474,11 @@ static struct dst_entry* dccp_v4_route_skb(struct net *net, struct sock *sk,
 					   struct sk_buff *skb)
 {
 	struct rtable *rt;
+	const struct iphdr *iph = ip_hdr(skb);
 	struct flowi4 fl4 = {
 		.flowi4_oif = skb_rtable(skb)->rt_iif,
-		.daddr = ip_hdr(skb)->saddr,
-		.saddr = ip_hdr(skb)->daddr,
+		.daddr = iph->saddr,
+		.saddr = iph->daddr,
 		.flowi4_tos = RT_CONN_FLAGS(sk),
 		.flowi4_proto = sk->sk_protocol,
 		.fl4_sport = dccp_hdr(skb)->dccph_dport,

Re: [PATCH net-next] ipv4: use a 64bit load/store in output path

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 01 Dec 2011 08:28:41 +0100

> Le jeudi 01 décembre 2011 à 01:48 -0500, David Miller a écrit :
>> From: Eric Dumazet <eric.dumazet@gmail.com>
>> Date: Thu, 01 Dec 2011 06:00:53 +0100
>> 
>> > gcc compiler is smart enough to use a single load/store if we
>> > memcpy(dptr, sptr, 8) on x86_64, regardless of
>> > CONFIG_CC_OPTIMIZE_FOR_SIZE
>> >     
>> > In IP header, daddr immediately follows saddr, this wont change in the
>> > future. We only need to make sure our flowi4 (saddr,daddr) fields wont
>> > break the rule.
>> > 
>> > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
>> 
>> Hmmm, this triggers a strange new build warning:
>> 
>> net/dccp/ipv4.c: In function ‘dccp_v4_route_skb’:
>> net/dccp/ipv4.c:481:3: warning: initialized field with side-effects overwritten [enabled by default]
>> net/dccp/ipv4.c:481:3: warning: (near initialization for ‘fl4’) [enabled by default]
> 
> Hmm, seems a compiler bug, since following fixes the warning ?

I stared at this for some time and I couldn't figure out even what GCC might
be thinking.  I wondered if, for example, the WARN_ON() in the skb_dst()
implementation was part of the issue.  Since that ends up calling functions.

Anyways, I applied the "iph" thing first, then your 64bit load/store change
afterwards.

Thanks.