diff mbox

[net-next,10/16] Don't lookup the socket if there's a socket attached to the skb

Message ID 1222962200.14079.19.camel@este
State Not Applicable, archived
Delegated to: David Miller
Headers show

Commit Message

KOVACS Krisztian Oct. 2, 2008, 3:43 p.m. UTC 
Hi,

On Wed, 2008-10-01 at 08:51 -0700, David Miller wrote:
> From: KOVACS Krisztian <hidden@sch.bme.hu>
> Date: Wed, 01 Oct 2008 17:38:20 +0200
> 
> > The problem is that if you include the if() test then you have to
> > include the lookup call as well and that's different for TCP/UDP.
> 
> No, I only mean to make a helper for this construct:
> 
> 	if (unlikely(skb->sk)) {
> 		...
> 	}
> 
> so, something like:
> 
> static inline struct sock *sock_skb_steal(struct sk_buff *skb)
> {
> 	if (unlikely(skb->sk)) {
> 		struct sock *sk = skb->sk;
> 
> 		skb->destructor = NULL;
> 		skb->sk = NULL;
> 		return sk;
> 	}
> 	return NULL;
> }
> 
> and then also get rid of the ifdefs at the place where
> these calls are made (TCP and UDP).

Something like this?

- 8< -

Use the socket reference cached in the skb if present. The iptables
TPROXY rule for example does a socket lookup early and attaches the
socket reference to the skb. So in the respective TCP/UDP/DCCP rcv()
routine we don't do a lookup but simply steal the socket reference from
the skb. 

Signed-off-by: KOVACS Krisztian <hidden@sch.bme.hu>
---

 include/net/sock.h  |   12 ++++++++++++
 net/dccp/ipv4.c     |    7 ++++---
 net/dccp/ipv6.c     |    9 +++++----
 net/ipv4/tcp_ipv4.c |    6 ++++--
 net/ipv4/udp.c      |    5 +++--
 net/ipv6/tcp_ipv6.c |    9 +++++----
 net/ipv6/udp.c      |    5 +++--
 7 files changed, 36 insertions(+), 17 deletions(-)



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Arnaldo Carvalho de Melo Oct. 2, 2008, 5:09 p.m. UTC | #1 
Em Thu, Oct 02, 2008 at 05:43:20PM +0200, KOVACS Krisztian escreveu:
> Hi,
> 
> On Wed, 2008-10-01 at 08:51 -0700, David Miller wrote:
> > From: KOVACS Krisztian <hidden@sch.bme.hu>
> > Date: Wed, 01 Oct 2008 17:38:20 +0200
> > 
> > > The problem is that if you include the if() test then you have to
> > > include the lookup call as well and that's different for TCP/UDP.
> > 
> > No, I only mean to make a helper for this construct:
> > 
> > 	if (unlikely(skb->sk)) {
> > 		...
> > 	}
> > 
> > so, something like:
> > 
> > static inline struct sock *sock_skb_steal(struct sk_buff *skb)
> > {
> > 	if (unlikely(skb->sk)) {
> > 		struct sock *sk = skb->sk;
> > 
> > 		skb->destructor = NULL;
> > 		skb->sk = NULL;
> > 		return sk;
> > 	}
> > 	return NULL;
> > }
> > 
> > and then also get rid of the ifdefs at the place where
> > these calls are made (TCP and UDP).
> 
> Something like this?
> 
> - 8< -

Why don't you add it to __inet6_lookup, __inet6_lookup and the udp_lib
lookup routines? And please rename it to skb_steal_sock, as it acts on a
skb, not on a sock.

- Arnaldo
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller Oct. 2, 2008, 7:58 p.m. UTC | #2 
From: Arnaldo Carvalho de Melo <acme@redhat.com>
Date: Thu, 2 Oct 2008 14:09:35 -0300

> Why don't you add it to __inet6_lookup, __inet6_lookup and the udp_lib
> lookup routines? And please rename it to skb_steal_sock, as it acts on a
> skb, not on a sock.

That seems to make sense to me.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/include/net/sock.h b/include/net/sock.h
index 75a312d..b60fdc1 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -1324,6 +1324,18 @@  static inline void sk_change_net(struct sock *sk, struct net *net)
 	sock_net_set(sk, hold_net(net));
 }
 
+static inline struct sock *sock_skb_steal(struct sk_buff *skb)
+{
+	if (unlikely(skb->sk)) {
+		struct sock *sk = skb->sk;
+
+		skb->destructor = NULL;
+		skb->sk = NULL;
+		return sk;
+	}
+	return NULL;
+}
+
 extern void sock_enable_timestamp(struct sock *sk);
 extern int sock_get_timestamp(struct sock *, struct timeval __user *);
 extern int sock_get_timestampns(struct sock *, struct timespec __user *);
diff --git a/net/dccp/ipv4.c b/net/dccp/ipv4.c
index 882c5c4..6968ac8 100644
--- a/net/dccp/ipv4.c
+++ b/net/dccp/ipv4.c
@@ -811,9 +811,10 @@  static int dccp_v4_rcv(struct sk_buff *skb)
 
 	/* Step 2:
 	 *	Look up flow ID in table and get corresponding socket */
-	sk = __inet_lookup(dev_net(skb->dst->dev), &dccp_hashinfo,
-			   iph->saddr, dh->dccph_sport,
-			   iph->daddr, dh->dccph_dport, inet_iif(skb));
+	if (likely((sk = sock_skb_steal(skb)) == NULL))
+		sk = __inet_lookup(dev_net(skb->dst->dev), &dccp_hashinfo,
+				   iph->saddr, dh->dccph_sport,
+				   iph->daddr, dh->dccph_dport, inet_iif(skb));
 	/*
 	 * Step 2:
 	 *	If no socket ...
diff --git a/net/dccp/ipv6.c b/net/dccp/ipv6.c
index 5e1ee0d..97d1405 100644
--- a/net/dccp/ipv6.c
+++ b/net/dccp/ipv6.c
@@ -805,10 +805,11 @@  static int dccp_v6_rcv(struct sk_buff *skb)
 
 	/* Step 2:
 	 *	Look up flow ID in table and get corresponding socket */
-	sk = __inet6_lookup(dev_net(skb->dst->dev), &dccp_hashinfo,
-			    &ipv6_hdr(skb)->saddr, dh->dccph_sport,
-			    &ipv6_hdr(skb)->daddr, ntohs(dh->dccph_dport),
-			    inet6_iif(skb));
+	if (likely((sk = sock_skb_steal(skb)) == NULL))
+		sk = __inet6_lookup(dev_net(skb->dst->dev), &dccp_hashinfo,
+				    &ipv6_hdr(skb)->saddr, dh->dccph_sport,
+				    &ipv6_hdr(skb)->daddr, ntohs(dh->dccph_dport),
+				    inet6_iif(skb));
 	/*
 	 * Step 2:
 	 *	If no socket ...
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 1ac4d05..e879037 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1577,8 +1577,10 @@  int tcp_v4_rcv(struct sk_buff *skb)
 	TCP_SKB_CB(skb)->flags	 = iph->tos;
 	TCP_SKB_CB(skb)->sacked	 = 0;
 
-	sk = __inet_lookup(net, &tcp_hashinfo, iph->saddr,
-			th->source, iph->daddr, th->dest, inet_iif(skb));
+	if (likely((sk = sock_skb_steal(skb)) == NULL))
+		sk = __inet_lookup(net, &tcp_hashinfo, iph->saddr,
+				th->source, iph->daddr, th->dest, inet_iif(skb));
+
 	if (!sk)
 		goto no_tcp_socket;
 
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 28c3c31..874bceb 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -1198,8 +1198,9 @@  int __udp4_lib_rcv(struct sk_buff *skb, struct hlist_head udptable[],
 		return __udp4_lib_mcast_deliver(net, skb, uh,
 				saddr, daddr, udptable);
 
-	sk = __udp4_lib_lookup(net, saddr, uh->source, daddr,
-			uh->dest, inet_iif(skb), udptable);
+	if (likely((sk = sock_skb_steal(skb)) == NULL))
+		sk = __udp4_lib_lookup(net, saddr, uh->source, daddr,
+				uh->dest, inet_iif(skb), udptable);
 
 	if (sk != NULL) {
 		int ret = 0;
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index e85f377..c44b6d7 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -1681,10 +1681,11 @@  static int tcp_v6_rcv(struct sk_buff *skb)
 	TCP_SKB_CB(skb)->flags = ipv6_get_dsfield(ipv6_hdr(skb));
 	TCP_SKB_CB(skb)->sacked = 0;
 
-	sk = __inet6_lookup(net, &tcp_hashinfo,
-			&ipv6_hdr(skb)->saddr, th->source,
-			&ipv6_hdr(skb)->daddr, ntohs(th->dest),
-			inet6_iif(skb));
+	if (likely((sk = sock_skb_steal(skb)) == NULL))
+		sk = __inet6_lookup(net, &tcp_hashinfo,
+				&ipv6_hdr(skb)->saddr, th->source,
+				&ipv6_hdr(skb)->daddr, ntohs(th->dest),
+				inet6_iif(skb));
 
 	if (!sk)
 		goto no_tcp_socket;
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index a6aecf7..e2741aa 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -488,8 +488,9 @@  int __udp6_lib_rcv(struct sk_buff *skb, struct hlist_head udptable[],
 	 * check socket cache ... must talk to Alan about his plans
 	 * for sock caches... i'll skip this for now.
 	 */
-	sk = __udp6_lib_lookup(net, saddr, uh->source,
-			       daddr, uh->dest, inet6_iif(skb), udptable);
+	if (likely((sk = sock_skb_steal(skb)) == NULL))
+		sk = __udp6_lib_lookup(net, saddr, uh->source,
+				       daddr, uh->dest, inet6_iif(skb), udptable);
 
 	if (sk == NULL) {
 		if (!xfrm6_policy_check(NULL, XFRM_POLICY_IN, skb))