Skip to contentSkip to conversation

inet_peer: Optimize inet_getid()

PATCH In review Eric Dumazet 3 participants 6 messages Sep 24, 2009 – Oct 5, 2009

Subsystems (3)
Networking Network drivers IPv4/IPv6
lore
Share & tools
lore

Eric Dumazet

author
#0 lore raw

PATCH 1/1 inet_peer: Optimize inet_getid()

While investigating for network latencies, I found inet_getid() was a contention point for some workloads.

Fix is straightforward, using cmpxchg() instead of a spin_lock_bh()/spin_unlock_bh() pair on a central lock.

Another possibility was to use an atomic_t and atomic_add_return() but the size of struct inet_peer object would had doubled on x86_64 because of SLAB_HWCACHE_ALIGN constraint.

Signed-off-by Eric Dumazet <[email protected]>
---
 include/net/inetpeer.h |   16 ++++++++--------
 net/ipv4/inetpeer.c    |    3 ---
 2 files changed, 8 insertions(+), 11 deletions(-)
include/net/inetpeer.h +8 -8
index 15e1f8f..952f0ad 100644
@@ -37,17 +37,17 @@ struct inet_peer	*inet_getpeer(__be32 daddr, int create);
 /* can be called from BH context or outside */
 extern void inet_putpeer(struct inet_peer *p);
 
-extern spinlock_t inet_peer_idlock;
 /* can be called with or without local BH being disabled */
 static inline __u16	inet_getid(struct inet_peer *p, int more)
 {
-	__u16 id;
-
-	spin_lock_bh(&inet_peer_idlock);
-	id = p->ip_id_count;
-	p->ip_id_count += 1 + more;
-	spin_unlock_bh(&inet_peer_idlock);
-	return id;
+	__u16 old;
+
+	while (1) {
+		old = p->ip_id_count;
+		if (cmpxchg(&p->ip_id_count, old, old + 1 + more) == old)
+			break;
+	}
+	return old;
 }
 
 #endif /* _NET_INETPEER_H */
net/ipv4/inetpeer.c +0 -3
index b1fbe18..5dc29b8 100644
@@ -67,9 +67,6 @@
  *		ip_id_count: idlock
  */
 
-/* Exported for inet_getid inline function.  */
-DEFINE_SPINLOCK(inet_peer_idlock);
-
 static struct kmem_cache *peer_cachep __read_mostly;
 
 #define node_height(x) x->avl_height
#1 lore raw
Quoted 11 lines · On Thu, 24 Sep 2009 21:04:56 +0200 Eric Dumazet <[email protected]> wrote:

While investigating for network latencies, I found inet_getid() was a contention point for some workloads.

Fix is straightforward, using cmpxchg() instead of a spin_lock_bh()/spin_unlock_bh() pair on a central lock.

Another possibility was to use an atomic_t and atomic_add_return() but the size of struct inet_peer object would had doubled on x86_64 because of SLAB_HWCACHE_ALIGN constraint.

Signed-off-by Eric Dumazet <[email protected]>

I thought cmpxchg was not available on all architectures.

author
#2 lore raw
Quoted 9 lines · Stephen Hemminger
Quoted 6 lines · On Thu, 24 Sep 2009 21:04:56 +0200 Eric Dumazet <[email protected]> wrote:

While investigating for network latencies, I found inet_getid() was a contention point for some workloads.

Fix is straightforward, using cmpxchg() instead of a spin_lock_bh()/spin_unlock_bh() pair on a central lock.

Quoted 7 lines
Quoted 5 lines

Another possibility was to use an atomic_t and atomic_add_return() but the size of struct inet_peer object would had doubled on x86_64 because of SLAB_HWCACHE_ALIGN constraint.

Signed-off-by Eric Dumazet <[email protected]>

I thought cmpxchg was not available on all architectures.

Good point Stephen, I forgot about non x86 arches ;)

I'll send an update with cmpxchg() if available, and atomic_t as fallback.

author
#3 lore raw
In reply to Stephen Hemminger
Quoted 9 lines · Stephen Hemminger
Quoted 6 lines · On Thu, 24 Sep 2009 21:04:56 +0200 Eric Dumazet <[email protected]> wrote:

While investigating for network latencies, I found inet_getid() was a contention point for some workloads.

Fix is straightforward, using cmpxchg() instead of a spin_lock_bh()/spin_unlock_bh() pair on a central lock.

Quoted 7 lines
Quoted 5 lines

Another possibility was to use an atomic_t and atomic_add_return() but the size of struct inet_peer object would had doubled on x86_64 because of SLAB_HWCACHE_ALIGN constraint.

Signed-off-by Eric Dumazet <[email protected]>

I thought cmpxchg was not available on all architectures.

Here is the second version of the patch, thanks Stephen

[PATCH] inet_peer: Optimize inet_getid()

While investigating for network latencies, I found inet_getid() was a contention point for some workloads.

If __HAVE_ARCH_CMPXCHG is defined, we can use cmpxchg() instead of a spin_lock_bh()/spin_unlock_bh() pair on a central lock.

On other arches, we can use an atomic_t instead of a u16, and atomic_add_return(). This might grow memory usage a bit, unless someone invents atomic16_t :)

Signed-off-by Eric Dumazet <[email protected]>
---
 include/net/inetpeer.h |   43 ++++++++++++++++++++++++++++++++-------
 net/ipv4/inetpeer.c    |    5 ----
 net/ipv4/route.c       |    2 -
 3 files changed, 38 insertions(+), 12 deletions(-)
include/net/inetpeer.h +36 -7
index 15e1f8f..f1cde51 100644
@@ -19,7 +19,11 @@ struct inet_peer
 	struct inet_peer	*avl_left, *avl_right;
 	__be32			v4daddr;	/* peer's address */
 	__u16			avl_height;
+#if defined(__HAVE_ARCH_CMPXCHG)
 	__u16			ip_id_count;	/* IP ID for the next packet */
+#else
+	atomic_t		ip_id_count;
+#endif
 	struct list_head	unused;
 	__u32			dtime;		/* the time of last use of not
 						 * referenced entries */

@@ -37,17 +41,42 @@ struct inet_peer	*inet_getpeer(__be32 daddr, int create);
 /* can be called from BH context or outside */
 extern void inet_putpeer(struct inet_peer *p);
 
-extern spinlock_t inet_peer_idlock;
 /* can be called with or without local BH being disabled */
 static inline __u16	inet_getid(struct inet_peer *p, int more)
 {
-	__u16 id;
+	__u16 old;
 
-	spin_lock_bh(&inet_peer_idlock);
-	id = p->ip_id_count;
-	p->ip_id_count += 1 + more;
-	spin_unlock_bh(&inet_peer_idlock);
-	return id;
+	more++;
+#if defined(__HAVE_ARCH_CMPXCHG)
+	while (1) {
+		old = p->ip_id_count;
+		if (cmpxchg(&p->ip_id_count, old, old + more) == old)
+			break;
+	}
+#else
+	old = atomic_add_return(more, &p->ip_id_count) - more;
+#endif
+	return old;
 }
 
+static inline void inet_id_set(struct inet_peer *p, int val)
+{
+#if defined(__HAVE_ARCH_CMPXCHG)
+	p->ip_id_count = val;
+#else
+	atomic_set(&p->ip_id_count, val);
+#endif
+}
+
+static inline __u16 inet_id_read(const struct inet_peer *p)
+{
+#if defined(__HAVE_ARCH_CMPXCHG)
+	return p->ip_id_count;
+#else
+	return atomic_read(&p->ip_id_count);
+#endif
+}
+
+
+
 #endif /* _NET_INETPEER_H */
net/ipv4/inetpeer.c +1 -4
index b1fbe18..3641831 100644
@@ -67,9 +67,6 @@
  *		ip_id_count: idlock
  */
 
-/* Exported for inet_getid inline function.  */
-DEFINE_SPINLOCK(inet_peer_idlock);
-
 static struct kmem_cache *peer_cachep __read_mostly;
 
 #define node_height(x) x->avl_height

@@ -390,7 +387,7 @@ struct inet_peer *inet_getpeer(__be32 daddr, int create)
 	n->v4daddr = daddr;
 	atomic_set(&n->refcnt, 1);
 	atomic_set(&n->rid, 0);
-	n->ip_id_count = secure_ip_id(daddr);
+	inet_id_set(n, secure_ip_id(daddr));
 	n->tcp_ts_stamp = 0;
 
 	write_lock_bh(&peer_pool_lock);
net/ipv4/route.c +1 -1
index df93473..a090fcf 100644
@@ -2855,7 +2855,7 @@ static int rt_fill_info(struct net *net,
 	error = rt->u.dst.error;
 	expires = rt->u.dst.expires ? rt->u.dst.expires - jiffies : 0;
 	if (rt->peer) {
-		id = rt->peer->ip_id_count;
+		id = inet_id_read(rt->peer);
 		if (rt->peer->tcp_ts_stamp) {
 			ts = rt->peer->tcp_ts;
 			tsage = get_seconds() - rt->peer->tcp_ts_stamp;
maintainer
#4 lore raw

From: Eric Dumazet <[email protected]>
Date: Thu, 24 Sep 2009 22:44:53 +0200

Quoted 5 lines

[PATCH] inet_peer: Optimize inet_getid()

While investigating for network latencies, I found inet_getid() was a contention point for some workloads.

Quoted 8 lines

If __HAVE_ARCH_CMPXCHG is defined, we can use cmpxchg() instead of a spin_lock_bh()/spin_unlock_bh() pair on a central lock.

On other arches, we can use an atomic_t instead of a u16, and atomic_add_return(). This might grow memory usage a bit, unless someone invents atomic16_t :)

Signed-off-by Eric Dumazet <[email protected]>

I can't apply this, it's going to break the build on some architectures.

For example, sparc64 only supports cmpxchg on u32 and u64 objects, but you're trying to use it on a u16 here.

author
#5 lore raw
Quoted 6 lines · David Miller

I can't apply this, it's going to break the build on some architectures.

For example, sparc64 only supports cmpxchg on u32 and u64 objects, but you're trying to use it on a u16 here.

Oops, thanks for the info David !