* [PATCH net-next-2.6] inetpeer: RCU conversion
@ 2010-06-15 18:23 Eric Dumazet
2010-06-15 21:25 ` David Miller
0 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2010-06-15 18:23 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Paul E. McKenney
inetpeer currently uses an AVL tree protected by an rwlock.
It's possible to make most lookups use RCU
1) Add a struct rcu_head to struct inet_peer
2) add a lookup_rcu_bh() helper to perform lockless and opportunistic
lookup. This is a normal function, not a macro like lookup().
3) Add a limit to number of links followed by lookup_rcu_bh(). This is
needed in case we fall in a loop.
4) add an smp_wmb() in link_to_pool() right before node insert.
5) make unlink_from_pool() use atomic_cmpxchg() to make sure it can take
last reference to an inet_peer, since lockless readers could increase
refcount, even while we hold peers.lock.
6) Delay struct inet_peer freeing after rcu grace period so that
lookup_rcu_bh() cannot crash.
7) inet_getpeer() first attempts lockless lookup.
Note this lookup can fail even if target is in AVL tree, but a
concurrent writer can let tree in a non correct form.
If this attemps fails, lock is taken a regular lookup is performed
again.
8) convert peers.lock from rwlock to a spinlock
9) Remove SLAB_HWCACHE_ALIGN when peer_cachep is created, because
rcu_head adds 16 bytes on 64bit arches, doubling effective size (64 ->
128 bytes)
In a future patch, this is probably possible to revert this part, if rcu
field is put in an union to share space with rid, ip_id_count, tcp_ts &
tcp_ts_stamp. These fields being manipulated only with refcnt > 0.
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
include/net/inetpeer.h | 1
net/ipv4/inetpeer.c | 164 ++++++++++++++++++++++-----------------
2 files changed, 96 insertions(+), 69 deletions(-)
diff --git a/include/net/inetpeer.h b/include/net/inetpeer.h
index 87b1df0..6174047 100644
--- a/include/net/inetpeer.h
+++ b/include/net/inetpeer.h
@@ -26,6 +26,7 @@ struct inet_peer {
atomic_t ip_id_count; /* IP ID for the next packet */
__u32 tcp_ts;
__u32 tcp_ts_stamp;
+ struct rcu_head rcu;
};
void inet_initpeers(void) __init;
diff --git a/net/ipv4/inetpeer.c b/net/ipv4/inetpeer.c
index 035673f..5f3846c 100644
--- a/net/ipv4/inetpeer.c
+++ b/net/ipv4/inetpeer.c
@@ -51,8 +51,8 @@
* lookups performed with disabled BHs.
*
* Serialisation issues.
- * 1. Nodes may appear in the tree only with the pool write lock held.
- * 2. Nodes may disappear from the tree only with the pool write lock held
+ * 1. Nodes may appear in the tree only with the pool lock held.
+ * 2. Nodes may disappear from the tree only with the pool lock held
* AND reference count being 0.
* 3. Nodes appears and disappears from unused node list only under
* "inet_peer_unused_lock".
@@ -80,11 +80,11 @@ static const struct inet_peer peer_fake_node = {
static struct {
struct inet_peer *root;
- rwlock_t lock;
+ spinlock_t lock;
int total;
} peers = {
.root = peer_avl_empty,
- .lock = __RW_LOCK_UNLOCKED(peers.lock),
+ .lock = __SPIN_LOCK_UNLOCKED(peers.lock),
.total = 0,
};
#define PEER_MAXDEPTH 40 /* sufficient for about 2^27 nodes */
@@ -129,7 +129,7 @@ void __init inet_initpeers(void)
peer_cachep = kmem_cache_create("inet_peer_cache",
sizeof(struct inet_peer),
- 0, SLAB_HWCACHE_ALIGN|SLAB_PANIC,
+ 0, SLAB_PANIC,
NULL);
/* All the timers, started at system startup tend
@@ -153,16 +153,13 @@ static void unlink_from_unused(struct inet_peer *p)
/*
* Called with local BH disabled and the pool lock held.
- * _stack is known to be NULL or not at compile time,
- * so compiler will optimize the if (_stack) tests.
*/
#define lookup(_daddr, _stack) \
({ \
struct inet_peer *u, **v; \
- if (_stack != NULL) { \
- stackptr = _stack; \
- *stackptr++ = &peers.root; \
- } \
+ \
+ stackptr = _stack; \
+ *stackptr++ = &peers.root; \
for (u = peers.root; u != peer_avl_empty; ) { \
if (_daddr == u->v4daddr) \
break; \
@@ -170,14 +167,41 @@ static void unlink_from_unused(struct inet_peer *p)
v = &u->avl_left; \
else \
v = &u->avl_right; \
- if (_stack != NULL) \
- *stackptr++ = v; \
+ *stackptr++ = v; \
u = *v; \
} \
u; \
})
-/* Called with local BH disabled and the pool write lock held. */
+/*
+ * Called with rcu_read_lock_bh()
+ * Because we hold no lock against a writer, its quite possible we fall
+ * in an endless loop.
+ * But every pointer we follow is guaranteed to be valid thanks to RCU.
+ * We exit from this function if number of links exceeds PEER_MAXDEPTH
+ */
+static struct inet_peer *lookup_rcu_bh(__be32 daddr)
+{
+ struct inet_peer *u = rcu_dereference_bh(peers.root);
+ int count = 0;
+
+ while (u != peer_avl_empty) {
+ if (daddr == u->v4daddr) {
+ if (unlikely(!atomic_inc_not_zero(&u->refcnt)))
+ u = NULL;
+ return u;
+ }
+ if ((__force __u32)daddr < (__force __u32)u->v4daddr)
+ u = rcu_dereference_bh(u->avl_left);
+ else
+ u = rcu_dereference_bh(u->avl_right);
+ if (unlikely(++count == PEER_MAXDEPTH))
+ break;
+ }
+ return NULL;
+}
+
+/* Called with local BH disabled and the pool lock held. */
#define lookup_rightempty(start) \
({ \
struct inet_peer *u, **v; \
@@ -191,9 +215,10 @@ static void unlink_from_unused(struct inet_peer *p)
u; \
})
-/* Called with local BH disabled and the pool write lock held.
+/* Called with local BH disabled and the pool lock held.
* Variable names are the proof of operation correctness.
- * Look into mm/map_avl.c for more detail description of the ideas. */
+ * Look into mm/map_avl.c for more detail description of the ideas.
+ */
static void peer_avl_rebalance(struct inet_peer **stack[],
struct inet_peer ***stackend)
{
@@ -269,16 +294,22 @@ static void peer_avl_rebalance(struct inet_peer **stack[],
}
}
-/* Called with local BH disabled and the pool write lock held. */
+/* Called with local BH disabled and the pool lock held. */
#define link_to_pool(n) \
do { \
n->avl_height = 1; \
n->avl_left = peer_avl_empty; \
n->avl_right = peer_avl_empty; \
+ smp_wmb(); /* lockless readers can catch us now */ \
**--stackptr = n; \
peer_avl_rebalance(stack, stackptr); \
} while (0)
+static void inetpeer_free_rcu(struct rcu_head *head)
+{
+ kmem_cache_free(peer_cachep, container_of(head, struct inet_peer, rcu));
+}
+
/* May be called with local BH enabled. */
static void unlink_from_pool(struct inet_peer *p)
{
@@ -286,13 +317,13 @@ static void unlink_from_pool(struct inet_peer *p)
do_free = 0;
- write_lock_bh(&peers.lock);
+ spin_lock_bh(&peers.lock);
/* Check the reference counter. It was artificially incremented by 1
- * in cleanup() function to prevent sudden disappearing. If the
- * reference count is still 1 then the node is referenced only as `p'
- * here and from the pool. So under the exclusive pool lock it's safe
- * to remove the node and free it later. */
- if (atomic_read(&p->refcnt) == 1) {
+ * in cleanup() function to prevent sudden disappearing. If we can
+ * atomically (because of lockless readers) take this last reference,
+ * it's safe to remove the node and free it later.
+ */
+ if (atomic_cmpxchg(&p->refcnt, 1, 0) == 1) {
struct inet_peer **stack[PEER_MAXDEPTH];
struct inet_peer ***stackptr, ***delp;
if (lookup(p->v4daddr, stack) != p)
@@ -321,17 +352,18 @@ static void unlink_from_pool(struct inet_peer *p)
peers.total--;
do_free = 1;
}
- write_unlock_bh(&peers.lock);
+ spin_unlock_bh(&peers.lock);
if (do_free)
- kmem_cache_free(peer_cachep, p);
+ call_rcu_bh(&p->rcu, inetpeer_free_rcu);
else
/* The node is used again. Decrease the reference counter
* back. The loop "cleanup -> unlink_from_unused
* -> unlink_from_pool -> putpeer -> link_to_unused
* -> cleanup (for the same node)"
* doesn't really exist because the entry will have a
- * recent deletion time and will not be cleaned again soon. */
+ * recent deletion time and will not be cleaned again soon.
+ */
inet_putpeer(p);
}
@@ -375,62 +407,56 @@ static int cleanup_once(unsigned long ttl)
/* Called with or without local BH being disabled. */
struct inet_peer *inet_getpeer(__be32 daddr, int create)
{
- struct inet_peer *p, *n;
+ struct inet_peer *p;
struct inet_peer **stack[PEER_MAXDEPTH], ***stackptr;
- /* Look up for the address quickly. */
- read_lock_bh(&peers.lock);
- p = lookup(daddr, NULL);
- if (p != peer_avl_empty)
- atomic_inc(&p->refcnt);
- read_unlock_bh(&peers.lock);
+ /* Look up for the address quickly, lockless.
+ * Because of a concurrent writer, we might not find an existing entry.
+ */
+ rcu_read_lock_bh();
+ p = lookup_rcu_bh(daddr);
+ rcu_read_unlock_bh();
+
+ if (p) {
+ /* The existing node has been found.
+ * Remove the entry from unused list if it was there.
+ */
+ unlink_from_unused(p);
+ return p;
+ }
+ /* retry an exact lookup, taking the lock before.
+ * At least, nodes should be hot in our cache.
+ */
+ spin_lock_bh(&peers.lock);
+ p = lookup(daddr, stack);
if (p != peer_avl_empty) {
- /* The existing node has been found. */
+ atomic_inc(&p->refcnt);
+ spin_unlock_bh(&peers.lock);
/* Remove the entry from unused list if it was there. */
unlink_from_unused(p);
return p;
}
-
- if (!create)
- return NULL;
-
- /* Allocate the space outside the locked region. */
- n = kmem_cache_alloc(peer_cachep, GFP_ATOMIC);
- if (n == NULL)
- return NULL;
- n->v4daddr = daddr;
- atomic_set(&n->refcnt, 1);
- atomic_set(&n->rid, 0);
- atomic_set(&n->ip_id_count, secure_ip_id(daddr));
- n->tcp_ts_stamp = 0;
-
- write_lock_bh(&peers.lock);
- /* Check if an entry has suddenly appeared. */
- p = lookup(daddr, stack);
- if (p != peer_avl_empty)
- goto out_free;
-
- /* Link the node. */
- link_to_pool(n);
- INIT_LIST_HEAD(&n->unused);
- peers.total++;
- write_unlock_bh(&peers.lock);
+ p = create ? kmem_cache_alloc(peer_cachep, GFP_ATOMIC) : NULL;
+ if (p) {
+ p->v4daddr = daddr;
+ atomic_set(&p->refcnt, 1);
+ atomic_set(&p->rid, 0);
+ atomic_set(&p->ip_id_count, secure_ip_id(daddr));
+ p->tcp_ts_stamp = 0;
+ INIT_LIST_HEAD(&p->unused);
+
+
+ /* Link the node. */
+ link_to_pool(p);
+ peers.total++;
+ }
+ spin_unlock_bh(&peers.lock);
if (peers.total >= inet_peer_threshold)
/* Remove one less-recently-used entry. */
cleanup_once(0);
- return n;
-
-out_free:
- /* The appropriate node is already in the pool. */
- atomic_inc(&p->refcnt);
- write_unlock_bh(&peers.lock);
- /* Remove the entry from unused list if it was there. */
- unlink_from_unused(p);
- /* Free preallocated the preallocated node. */
- kmem_cache_free(peer_cachep, n);
return p;
}
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net-next-2.6] inetpeer: RCU conversion
2010-06-15 18:23 [PATCH net-next-2.6] inetpeer: RCU conversion Eric Dumazet
@ 2010-06-15 21:25 ` David Miller
2010-06-16 2:45 ` [PATCH net-next-2.6] inetpeer: do not use zero refcnt for freed entries Eric Dumazet
0 siblings, 1 reply; 6+ messages in thread
From: David Miller @ 2010-06-15 21:25 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev, paulmck
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 15 Jun 2010 20:23:14 +0200
> inetpeer currently uses an AVL tree protected by an rwlock.
>
> It's possible to make most lookups use RCU
...
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Applied, nice work Eric.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH net-next-2.6] inetpeer: do not use zero refcnt for freed entries
2010-06-15 21:25 ` David Miller
@ 2010-06-16 2:45 ` Eric Dumazet
2010-06-16 4:47 ` David Miller
2010-06-16 18:12 ` Paul E. McKenney
0 siblings, 2 replies; 6+ messages in thread
From: Eric Dumazet @ 2010-06-16 2:45 UTC (permalink / raw)
To: David Miller; +Cc: netdev, paulmck
Le mardi 15 juin 2010 à 14:25 -0700, David Miller a écrit :
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Tue, 15 Jun 2010 20:23:14 +0200
>
> > inetpeer currently uses an AVL tree protected by an rwlock.
> >
> > It's possible to make most lookups use RCU
> ...
> > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
>
> Applied, nice work Eric.
Thanks David !
Re-reading patch I realize refcnt is expected to be 0 for unused entries
(obviously), so we should use a different marker for 'about to be freed'
ones.
Thanks
[PATCH net-next-2.6] inetpeer: do not use zero refcnt for freed entries
Followup of commit aa1039e73cc2 (inetpeer: RCU conversion)
Unused inet_peer entries have a null refcnt.
Using atomic_inc_not_zero() in rcu lookups is not going to work for
them, and slow path is taken.
Fix this using -1 marker instead of 0 for deleted entries.
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
net/ipv4/inetpeer.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/net/ipv4/inetpeer.c b/net/ipv4/inetpeer.c
index 58fbc7e..39a14ba 100644
--- a/net/ipv4/inetpeer.c
+++ b/net/ipv4/inetpeer.c
@@ -187,7 +187,12 @@ static struct inet_peer *lookup_rcu_bh(__be32 daddr)
while (u != peer_avl_empty) {
if (daddr == u->v4daddr) {
- if (unlikely(!atomic_inc_not_zero(&u->refcnt)))
+ /* Before taking a reference, check if this entry was
+ * deleted, unlink_from_pool() sets refcnt=-1 to make
+ * distinction between an unused entry (refcnt=0) and
+ * a freed one.
+ */
+ if (unlikely(!atomic_add_unless(&u->refcnt, 1, -1)))
u = NULL;
return u;
}
@@ -322,8 +327,9 @@ static void unlink_from_pool(struct inet_peer *p)
* in cleanup() function to prevent sudden disappearing. If we can
* atomically (because of lockless readers) take this last reference,
* it's safe to remove the node and free it later.
+ * We use refcnt=-1 to alert lockless readers this entry is deleted.
*/
- if (atomic_cmpxchg(&p->refcnt, 1, 0) == 1) {
+ if (atomic_cmpxchg(&p->refcnt, 1, -1) == 1) {
struct inet_peer **stack[PEER_MAXDEPTH];
struct inet_peer ***stackptr, ***delp;
if (lookup(p->v4daddr, stack) != p)
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net-next-2.6] inetpeer: do not use zero refcnt for freed entries
2010-06-16 2:45 ` [PATCH net-next-2.6] inetpeer: do not use zero refcnt for freed entries Eric Dumazet
@ 2010-06-16 4:47 ` David Miller
2010-06-16 8:56 ` Eric Dumazet
2010-06-16 18:12 ` Paul E. McKenney
1 sibling, 1 reply; 6+ messages in thread
From: David Miller @ 2010-06-16 4:47 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev, paulmck
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 16 Jun 2010 04:45:24 +0200
> [PATCH net-next-2.6] inetpeer: do not use zero refcnt for freed entries
>
> Followup of commit aa1039e73cc2 (inetpeer: RCU conversion)
>
> Unused inet_peer entries have a null refcnt.
>
> Using atomic_inc_not_zero() in rcu lookups is not going to work for
> them, and slow path is taken.
>
> Fix this using -1 marker instead of 0 for deleted entries.
>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Applied, thanks Eric.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next-2.6] inetpeer: do not use zero refcnt for freed entries
2010-06-16 4:47 ` David Miller
@ 2010-06-16 8:56 ` Eric Dumazet
0 siblings, 0 replies; 6+ messages in thread
From: Eric Dumazet @ 2010-06-16 8:56 UTC (permalink / raw)
To: David Miller; +Cc: netdev, paulmck
Le mardi 15 juin 2010 à 21:47 -0700, David Miller a écrit :
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Wed, 16 Jun 2010 04:45:24 +0200
>
> > [PATCH net-next-2.6] inetpeer: do not use zero refcnt for freed entries
> >
> > Followup of commit aa1039e73cc2 (inetpeer: RCU conversion)
> >
> > Unused inet_peer entries have a null refcnt.
> >
> > Using atomic_inc_not_zero() in rcu lookups is not going to work for
> > them, and slow path is taken.
> >
> > Fix this using -1 marker instead of 0 for deleted entries.
> >
> > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
>
> Applied, thanks Eric.
Thanks
With 65537 peers and a DDOS frag attack, I now get following profiling
results :
-----------------------------------------------------------------------------------------
PerfTop: 1024 irqs/sec kernel:100.0% exact: 0.0% [1000Hz
cycles], (all, cpu: 0)
-----------------------------------------------------------------------------------------
samples pcnt function DSO
_______ _____ _________________________
7722.00 65.6% inet_frag_find
1355.00 11.5% ip4_frag_match
494.00 4.2% __lock_acquire
260.00 2.2% inet_getpeer
243.00 2.1% ip_route_input_common
151.00 1.3% lock_release
142.00 1.2% mark_lock
126.00 1.1% lock_acquire
104.00 0.9% __kmalloc
86.00 0.7% skb_put
Just to show what could be the next steps ;)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next-2.6] inetpeer: do not use zero refcnt for freed entries
2010-06-16 2:45 ` [PATCH net-next-2.6] inetpeer: do not use zero refcnt for freed entries Eric Dumazet
2010-06-16 4:47 ` David Miller
@ 2010-06-16 18:12 ` Paul E. McKenney
1 sibling, 0 replies; 6+ messages in thread
From: Paul E. McKenney @ 2010-06-16 18:12 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev
On Wed, Jun 16, 2010 at 04:45:24AM +0200, Eric Dumazet wrote:
> Le mardi 15 juin 2010 à 14:25 -0700, David Miller a écrit :
> > From: Eric Dumazet <eric.dumazet@gmail.com>
> > Date: Tue, 15 Jun 2010 20:23:14 +0200
> >
> > > inetpeer currently uses an AVL tree protected by an rwlock.
> > >
> > > It's possible to make most lookups use RCU
> > ...
> > > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> >
> > Applied, nice work Eric.
>
> Thanks David !
>
> Re-reading patch I realize refcnt is expected to be 0 for unused entries
> (obviously), so we should use a different marker for 'about to be freed'
> ones.
>
> Thanks
>
> [PATCH net-next-2.6] inetpeer: do not use zero refcnt for freed entries
>
> Followup of commit aa1039e73cc2 (inetpeer: RCU conversion)
>
> Unused inet_peer entries have a null refcnt.
>
> Using atomic_inc_not_zero() in rcu lookups is not going to work for
> them, and slow path is taken.
>
> Fix this using -1 marker instead of 0 for deleted entries.
Based on this patch, looks good to me! (I don't see lookup_rcu_bh() and
friends in the trees I have at hand.)
Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> ---
> net/ipv4/inetpeer.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/net/ipv4/inetpeer.c b/net/ipv4/inetpeer.c
> index 58fbc7e..39a14ba 100644
> --- a/net/ipv4/inetpeer.c
> +++ b/net/ipv4/inetpeer.c
> @@ -187,7 +187,12 @@ static struct inet_peer *lookup_rcu_bh(__be32 daddr)
>
> while (u != peer_avl_empty) {
> if (daddr == u->v4daddr) {
> - if (unlikely(!atomic_inc_not_zero(&u->refcnt)))
> + /* Before taking a reference, check if this entry was
> + * deleted, unlink_from_pool() sets refcnt=-1 to make
> + * distinction between an unused entry (refcnt=0) and
> + * a freed one.
> + */
> + if (unlikely(!atomic_add_unless(&u->refcnt, 1, -1)))
> u = NULL;
> return u;
> }
> @@ -322,8 +327,9 @@ static void unlink_from_pool(struct inet_peer *p)
> * in cleanup() function to prevent sudden disappearing. If we can
> * atomically (because of lockless readers) take this last reference,
> * it's safe to remove the node and free it later.
> + * We use refcnt=-1 to alert lockless readers this entry is deleted.
> */
> - if (atomic_cmpxchg(&p->refcnt, 1, 0) == 1) {
> + if (atomic_cmpxchg(&p->refcnt, 1, -1) == 1) {
> struct inet_peer **stack[PEER_MAXDEPTH];
> struct inet_peer ***stackptr, ***delp;
> if (lookup(p->v4daddr, stack) != p)
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-06-16 18:12 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-06-15 18:23 [PATCH net-next-2.6] inetpeer: RCU conversion Eric Dumazet
2010-06-15 21:25 ` David Miller
2010-06-16 2:45 ` [PATCH net-next-2.6] inetpeer: do not use zero refcnt for freed entries Eric Dumazet
2010-06-16 4:47 ` David Miller
2010-06-16 8:56 ` Eric Dumazet
2010-06-16 18:12 ` Paul E. McKenney
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.