* [PATCH] inet_peer: Optimize inet_getid() @ 2009-09-24 19:04 Eric Dumazet 2009-09-24 19:30 ` Stephen Hemminger 0 siblings, 1 reply; 6+ messages in thread From: Eric Dumazet @ 2009-09-24 19:04 UTC (permalink / raw) To: David S. Miller; +Cc: Linux Netdev List 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 <eric.dumazet@gmail.com> --- include/net/inetpeer.h | 16 ++++++++-------- net/ipv4/inetpeer.c | 3 --- 2 files changed, 8 insertions(+), 11 deletions(-) diff --git a/include/net/inetpeer.h b/include/net/inetpeer.h index 15e1f8f..952f0ad 100644 --- a/include/net/inetpeer.h +++ b/include/net/inetpeer.h @@ -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 */ diff --git a/net/ipv4/inetpeer.c b/net/ipv4/inetpeer.c index b1fbe18..5dc29b8 100644 --- a/net/ipv4/inetpeer.c +++ b/net/ipv4/inetpeer.c @@ -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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] inet_peer: Optimize inet_getid() 2009-09-24 19:04 [PATCH] inet_peer: Optimize inet_getid() Eric Dumazet @ 2009-09-24 19:30 ` Stephen Hemminger 2009-09-24 19:57 ` Eric Dumazet 2009-09-24 20:44 ` Eric Dumazet 0 siblings, 2 replies; 6+ messages in thread From: Stephen Hemminger @ 2009-09-24 19:30 UTC (permalink / raw) To: Eric Dumazet; +Cc: David S. Miller, Linux Netdev List On Thu, 24 Sep 2009 21:04:56 +0200 Eric Dumazet <eric.dumazet@gmail.com> 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 <eric.dumazet@gmail.com> I thought cmpxchg was not available on all architectures. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] inet_peer: Optimize inet_getid() 2009-09-24 19:30 ` Stephen Hemminger @ 2009-09-24 19:57 ` Eric Dumazet 2009-09-24 20:44 ` Eric Dumazet 1 sibling, 0 replies; 6+ messages in thread From: Eric Dumazet @ 2009-09-24 19:57 UTC (permalink / raw) To: Stephen Hemminger; +Cc: David S. Miller, Linux Netdev List Stephen Hemminger a écrit : > On Thu, 24 Sep 2009 21:04:56 +0200 > Eric Dumazet <eric.dumazet@gmail.com> 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 <eric.dumazet@gmail.com> > > 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] inet_peer: Optimize inet_getid() 2009-09-24 19:30 ` Stephen Hemminger 2009-09-24 19:57 ` Eric Dumazet @ 2009-09-24 20:44 ` Eric Dumazet 2009-10-05 7:08 ` David Miller 1 sibling, 1 reply; 6+ messages in thread From: Eric Dumazet @ 2009-09-24 20:44 UTC (permalink / raw) To: Stephen Hemminger; +Cc: David S. Miller, Linux Netdev List Stephen Hemminger a écrit : > On Thu, 24 Sep 2009 21:04:56 +0200 > Eric Dumazet <eric.dumazet@gmail.com> 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 <eric.dumazet@gmail.com> > > 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 <eric.dumazet@gmail.com> --- include/net/inetpeer.h | 43 ++++++++++++++++++++++++++++++++------- net/ipv4/inetpeer.c | 5 ---- net/ipv4/route.c | 2 - 3 files changed, 38 insertions(+), 12 deletions(-) diff --git a/include/net/inetpeer.h b/include/net/inetpeer.h index 15e1f8f..f1cde51 100644 --- a/include/net/inetpeer.h +++ b/include/net/inetpeer.h @@ -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 */ diff --git a/net/ipv4/inetpeer.c b/net/ipv4/inetpeer.c index b1fbe18..3641831 100644 --- a/net/ipv4/inetpeer.c +++ b/net/ipv4/inetpeer.c @@ -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); diff --git a/net/ipv4/route.c b/net/ipv4/route.c index df93473..a090fcf 100644 --- a/net/ipv4/route.c +++ b/net/ipv4/route.c @@ -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; ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] inet_peer: Optimize inet_getid() 2009-09-24 20:44 ` Eric Dumazet @ 2009-10-05 7:08 ` David Miller 2009-10-05 7:38 ` Eric Dumazet 0 siblings, 1 reply; 6+ messages in thread From: David Miller @ 2009-10-05 7:08 UTC (permalink / raw) To: eric.dumazet; +Cc: shemminger, netdev From: Eric Dumazet <eric.dumazet@gmail.com> Date: Thu, 24 Sep 2009 22:44:53 +0200 > [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 <eric.dumazet@gmail.com> 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] inet_peer: Optimize inet_getid() 2009-10-05 7:08 ` David Miller @ 2009-10-05 7:38 ` Eric Dumazet 0 siblings, 0 replies; 6+ messages in thread From: Eric Dumazet @ 2009-10-05 7:38 UTC (permalink / raw) To: David Miller; +Cc: shemminger, netdev David Miller a écrit : > > 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 ! ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2009-10-05 7:39 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-09-24 19:04 [PATCH] inet_peer: Optimize inet_getid() Eric Dumazet 2009-09-24 19:30 ` Stephen Hemminger 2009-09-24 19:57 ` Eric Dumazet 2009-09-24 20:44 ` Eric Dumazet 2009-10-05 7:08 ` David Miller 2009-10-05 7:38 ` Eric Dumazet
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).