* [PATCH net-next-2.6] net: build_ehash_secret() and rt_bind_peer() cleanups
@ 2010-08-19 15:46 Eric Dumazet
2010-08-19 15:58 ` Changli Gao
2010-08-19 16:28 ` Mathieu Desnoyers
0 siblings, 2 replies; 8+ messages in thread
From: Eric Dumazet @ 2010-08-19 15:46 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Mathieu Desnoyers
Now cmpxchg() is available on all arches, we can use it in
build_ehash_secret() and rt_bind_peer() instead of using spinlocks.
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
CC: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>
---
net/ipv4/af_inet.c | 8 +++-----
net/ipv4/route.c | 11 +++--------
2 files changed, 6 insertions(+), 13 deletions(-)
diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index 6a1100c..f581f77 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c
@@ -227,18 +227,16 @@ EXPORT_SYMBOL(inet_ehash_secret);
/*
* inet_ehash_secret must be set exactly once
- * Instead of using a dedicated spinlock, we (ab)use inetsw_lock
*/
void build_ehash_secret(void)
{
u32 rnd;
+
do {
get_random_bytes(&rnd, sizeof(rnd));
} while (rnd == 0);
- spin_lock_bh(&inetsw_lock);
- if (!inet_ehash_secret)
- inet_ehash_secret = rnd;
- spin_unlock_bh(&inetsw_lock);
+
+ cmpxchg(&inet_ehash_secret, 0, rnd);
}
EXPORT_SYMBOL(build_ehash_secret);
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 3f56b6e..ae3dba7 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -1268,18 +1268,13 @@ skip_hashing:
void rt_bind_peer(struct rtable *rt, int create)
{
- static DEFINE_SPINLOCK(rt_peer_lock);
struct inet_peer *peer;
peer = inet_getpeer(rt->rt_dst, create);
- spin_lock_bh(&rt_peer_lock);
- if (rt->peer == NULL) {
- rt->peer = peer;
- peer = NULL;
- }
- spin_unlock_bh(&rt_peer_lock);
- if (peer)
+ peer = cmpxchg(&rt->peer, NULL, peer);
+
+ if (unlikely(peer))
inet_putpeer(peer);
}
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH net-next-2.6] net: build_ehash_secret() and rt_bind_peer() cleanups
2010-08-19 15:46 [PATCH net-next-2.6] net: build_ehash_secret() and rt_bind_peer() cleanups Eric Dumazet
@ 2010-08-19 15:58 ` Changli Gao
2010-08-19 16:10 ` [PATCH net-next-2.6 v2] " Eric Dumazet
` (2 more replies)
2010-08-19 16:28 ` Mathieu Desnoyers
1 sibling, 3 replies; 8+ messages in thread
From: Changli Gao @ 2010-08-19 15:58 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev, Mathieu Desnoyers
On Thu, Aug 19, 2010 at 11:46 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Now cmpxchg() is available on all arches, we can use it in
> build_ehash_secret() and rt_bind_peer() instead of using spinlocks.
>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> CC: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>
> ---
> net/ipv4/af_inet.c | 8 +++-----
> net/ipv4/route.c | 11 +++--------
> 2 files changed, 6 insertions(+), 13 deletions(-)
>
> diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
> index 6a1100c..f581f77 100644
> --- a/net/ipv4/af_inet.c
> +++ b/net/ipv4/af_inet.c
> @@ -227,18 +227,16 @@ EXPORT_SYMBOL(inet_ehash_secret);
>
> /*
> * inet_ehash_secret must be set exactly once
> - * Instead of using a dedicated spinlock, we (ab)use inetsw_lock
> */
> void build_ehash_secret(void)
> {
> u32 rnd;
> +
> do {
> get_random_bytes(&rnd, sizeof(rnd));
> } while (rnd == 0);
> - spin_lock_bh(&inetsw_lock);
> - if (!inet_ehash_secret)
> - inet_ehash_secret = rnd;
> - spin_unlock_bh(&inetsw_lock);
> +
> + cmpxchg(&inet_ehash_secret, 0, rnd);
> }
> EXPORT_SYMBOL(build_ehash_secret);
>
> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
> index 3f56b6e..ae3dba7 100644
> --- a/net/ipv4/route.c
> +++ b/net/ipv4/route.c
> @@ -1268,18 +1268,13 @@ skip_hashing:
>
> void rt_bind_peer(struct rtable *rt, int create)
> {
> - static DEFINE_SPINLOCK(rt_peer_lock);
> struct inet_peer *peer;
>
> peer = inet_getpeer(rt->rt_dst, create);
>
> - spin_lock_bh(&rt_peer_lock);
> - if (rt->peer == NULL) {
> - rt->peer = peer;
> - peer = NULL;
> - }
> - spin_unlock_bh(&rt_peer_lock);
> - if (peer)
> + peer = cmpxchg(&rt->peer, NULL, peer);
> +
> + if (unlikely(peer))
It isn't correct, and should be
if (unlikely(cmpxchg(&rt->peer, NULL, peer) != NULL))
> inet_putpeer(peer);
> }
--
Regards,
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH net-next-2.6 v2] net: build_ehash_secret() and rt_bind_peer() cleanups
2010-08-19 15:58 ` Changli Gao
@ 2010-08-19 16:10 ` Eric Dumazet
2010-08-20 7:51 ` David Miller
2010-08-19 16:10 ` [PATCH net-next-2.6] " Changli Gao
2010-08-19 16:13 ` Mathieu Desnoyers
2 siblings, 1 reply; 8+ messages in thread
From: Eric Dumazet @ 2010-08-19 16:10 UTC (permalink / raw)
To: Changli Gao; +Cc: David Miller, netdev, Mathieu Desnoyers
Le jeudi 19 août 2010 à 23:58 +0800, Changli Gao a écrit :
> > - if (peer)
> > + peer = cmpxchg(&rt->peer, NULL, peer);
> > +
> > + if (unlikely(peer))
>
> It isn't correct, and should be
> if (unlikely(cmpxchg(&rt->peer, NULL, peer) != NULL))
>
> > inet_putpeer(peer);
> > }
>
>
Good catch, thanks !
I was trying to take into account peer being NULL...
[PATCH net-next-2.6] net: build_ehash_secret() and rt_bind_peer() cleanups
Now cmpxchg() is available on all arches, we can use it in
build_ehash_secret() and rt_bind_peer() instead of using spinlocks.
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
CC: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>
---
net/ipv4/af_inet.c | 8 +++-----
net/ipv4/route.c | 9 +--------
2 files changed, 4 insertions(+), 13 deletions(-)
diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index 6a1100c..f581f77 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c
@@ -227,18 +227,16 @@ EXPORT_SYMBOL(inet_ehash_secret);
/*
* inet_ehash_secret must be set exactly once
- * Instead of using a dedicated spinlock, we (ab)use inetsw_lock
*/
void build_ehash_secret(void)
{
u32 rnd;
+
do {
get_random_bytes(&rnd, sizeof(rnd));
} while (rnd == 0);
- spin_lock_bh(&inetsw_lock);
- if (!inet_ehash_secret)
- inet_ehash_secret = rnd;
- spin_unlock_bh(&inetsw_lock);
+
+ cmpxchg(&inet_ehash_secret, 0, rnd);
}
EXPORT_SYMBOL(build_ehash_secret);
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 3f56b6e..85a67c9 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -1268,18 +1268,11 @@ skip_hashing:
void rt_bind_peer(struct rtable *rt, int create)
{
- static DEFINE_SPINLOCK(rt_peer_lock);
struct inet_peer *peer;
peer = inet_getpeer(rt->rt_dst, create);
- spin_lock_bh(&rt_peer_lock);
- if (rt->peer == NULL) {
- rt->peer = peer;
- peer = NULL;
- }
- spin_unlock_bh(&rt_peer_lock);
- if (peer)
+ if (peer && cmpxchg(&rt->peer, NULL, peer) != NULL)
inet_putpeer(peer);
}
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH net-next-2.6] net: build_ehash_secret() and rt_bind_peer() cleanups
2010-08-19 15:58 ` Changli Gao
2010-08-19 16:10 ` [PATCH net-next-2.6 v2] " Eric Dumazet
@ 2010-08-19 16:10 ` Changli Gao
2010-08-19 16:13 ` Mathieu Desnoyers
2 siblings, 0 replies; 8+ messages in thread
From: Changli Gao @ 2010-08-19 16:10 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev, Mathieu Desnoyers
On Thu, Aug 19, 2010 at 11:58 PM, Changli Gao <xiaosuo@gmail.com> wrote:
> On Thu, Aug 19, 2010 at 11:46 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> Now cmpxchg() is available on all arches, we can use it in
>> build_ehash_secret() and rt_bind_peer() instead of using spinlocks.
>>
>> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
>> CC: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>
>> ---
>> net/ipv4/af_inet.c | 8 +++-----
>> net/ipv4/route.c | 11 +++--------
>> 2 files changed, 6 insertions(+), 13 deletions(-)
>>
>> diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
>> index 6a1100c..f581f77 100644
>> --- a/net/ipv4/af_inet.c
>> +++ b/net/ipv4/af_inet.c
>> @@ -227,18 +227,16 @@ EXPORT_SYMBOL(inet_ehash_secret);
>>
>> /*
>> * inet_ehash_secret must be set exactly once
>> - * Instead of using a dedicated spinlock, we (ab)use inetsw_lock
>> */
>> void build_ehash_secret(void)
>> {
>> u32 rnd;
>> +
>> do {
>> get_random_bytes(&rnd, sizeof(rnd));
>> } while (rnd == 0);
>> - spin_lock_bh(&inetsw_lock);
>> - if (!inet_ehash_secret)
>> - inet_ehash_secret = rnd;
>> - spin_unlock_bh(&inetsw_lock);
>> +
>> + cmpxchg(&inet_ehash_secret, 0, rnd);
>> }
>> EXPORT_SYMBOL(build_ehash_secret);
>>
>> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
>> index 3f56b6e..ae3dba7 100644
>> --- a/net/ipv4/route.c
>> +++ b/net/ipv4/route.c
>> @@ -1268,18 +1268,13 @@ skip_hashing:
>>
>> void rt_bind_peer(struct rtable *rt, int create)
>> {
>> - static DEFINE_SPINLOCK(rt_peer_lock);
>> struct inet_peer *peer;
>>
>> peer = inet_getpeer(rt->rt_dst, create);
>>
>> - spin_lock_bh(&rt_peer_lock);
>> - if (rt->peer == NULL) {
>> - rt->peer = peer;
>> - peer = NULL;
>> - }
>> - spin_unlock_bh(&rt_peer_lock);
>> - if (peer)
>> + peer = cmpxchg(&rt->peer, NULL, peer);
>> +
>> + if (unlikely(peer))
>
> It isn't correct, and should be
> if (unlikely(cmpxchg(&rt->peer, NULL, peer) != NULL))
>
we should also test peer != NULL.
--
Regards,
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next-2.6] net: build_ehash_secret() and rt_bind_peer() cleanups
2010-08-19 15:58 ` Changli Gao
2010-08-19 16:10 ` [PATCH net-next-2.6 v2] " Eric Dumazet
2010-08-19 16:10 ` [PATCH net-next-2.6] " Changli Gao
@ 2010-08-19 16:13 ` Mathieu Desnoyers
2010-08-19 16:30 ` Mathieu Desnoyers
2 siblings, 1 reply; 8+ messages in thread
From: Mathieu Desnoyers @ 2010-08-19 16:13 UTC (permalink / raw)
To: Changli Gao; +Cc: Eric Dumazet, David Miller, netdev
* Changli Gao (xiaosuo@gmail.com) wrote:
> On Thu, Aug 19, 2010 at 11:46 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > Now cmpxchg() is available on all arches, we can use it in
> > build_ehash_secret() and rt_bind_peer() instead of using spinlocks.
> >
> > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> > CC: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>
> > ---
[...]
> > void rt_bind_peer(struct rtable *rt, int create)
> > {
> > - static DEFINE_SPINLOCK(rt_peer_lock);
> > struct inet_peer *peer;
> >
> > peer = inet_getpeer(rt->rt_dst, create);
> >
> > - spin_lock_bh(&rt_peer_lock);
> > - if (rt->peer == NULL) {
> > - rt->peer = peer;
> > - peer = NULL;
> > - }
> > - spin_unlock_bh(&rt_peer_lock);
> > - if (peer)
> > + peer = cmpxchg(&rt->peer, NULL, peer);
> > +
> > + if (unlikely(peer))
>
> It isn't correct, and should be
> if (unlikely(cmpxchg(&rt->peer, NULL, peer) != NULL))
>
> > inet_putpeer(peer);
> > }
What do you mean by "It isn't correct" ? Eric code and yours perform the
exact same thing. So I assume this is just a code style issue here.
Thanks,
Mathieu
--
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next-2.6] net: build_ehash_secret() and rt_bind_peer() cleanups
2010-08-19 15:46 [PATCH net-next-2.6] net: build_ehash_secret() and rt_bind_peer() cleanups Eric Dumazet
2010-08-19 15:58 ` Changli Gao
@ 2010-08-19 16:28 ` Mathieu Desnoyers
1 sibling, 0 replies; 8+ messages in thread
From: Mathieu Desnoyers @ 2010-08-19 16:28 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev
* Eric Dumazet (eric.dumazet@gmail.com) wrote:
> Now cmpxchg() is available on all arches, we can use it in
> build_ehash_secret() and rt_bind_peer() instead of using spinlocks.
>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> CC: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>
> ---
> net/ipv4/af_inet.c | 8 +++-----
> net/ipv4/route.c | 11 +++--------
> 2 files changed, 6 insertions(+), 13 deletions(-)
>
> diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
> index 6a1100c..f581f77 100644
> --- a/net/ipv4/af_inet.c
> +++ b/net/ipv4/af_inet.c
> @@ -227,18 +227,16 @@ EXPORT_SYMBOL(inet_ehash_secret);
>
> /*
> * inet_ehash_secret must be set exactly once
> - * Instead of using a dedicated spinlock, we (ab)use inetsw_lock
> */
> void build_ehash_secret(void)
> {
> u32 rnd;
> +
> do {
> get_random_bytes(&rnd, sizeof(rnd));
> } while (rnd == 0);
> - spin_lock_bh(&inetsw_lock);
> - if (!inet_ehash_secret)
> - inet_ehash_secret = rnd;
> - spin_unlock_bh(&inetsw_lock);
> +
> + cmpxchg(&inet_ehash_secret, 0, rnd);
I'd be more comfortable if you add a comment saying why you can get away
with not testing the cmpxchg failure case. (it's because the failure is
caused by a concurrent CPU setting the ehash secret to a random value,
which actually does the job for us, so no need to retry).
> }
> EXPORT_SYMBOL(build_ehash_secret);
>
> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
> index 3f56b6e..ae3dba7 100644
> --- a/net/ipv4/route.c
> +++ b/net/ipv4/route.c
> @@ -1268,18 +1268,13 @@ skip_hashing:
>
> void rt_bind_peer(struct rtable *rt, int create)
> {
> - static DEFINE_SPINLOCK(rt_peer_lock);
> struct inet_peer *peer;
>
> peer = inet_getpeer(rt->rt_dst, create);
Hrm, peer can be NULL on OOM condition.
>
> - spin_lock_bh(&rt_peer_lock);
> - if (rt->peer == NULL) {
> - rt->peer = peer;
> - peer = NULL;
> - }
> - spin_unlock_bh(&rt_peer_lock);
> - if (peer)
> + peer = cmpxchg(&rt->peer, NULL, peer);
> +
> + if (unlikely(peer))
> inet_putpeer(peer);
And you don't want to put the peer returned by cmpxchg, you want to put
the peer you just allocated.
I'd go for:
if (!peer)
return;
if (unlikely(cmpxchg(&rt->peer, NULL, peer) != NULL))
inet_putpeer(peer);
Thanks,
Mathieu
> }
>
>
>
--
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next-2.6] net: build_ehash_secret() and rt_bind_peer() cleanups
2010-08-19 16:13 ` Mathieu Desnoyers
@ 2010-08-19 16:30 ` Mathieu Desnoyers
0 siblings, 0 replies; 8+ messages in thread
From: Mathieu Desnoyers @ 2010-08-19 16:30 UTC (permalink / raw)
To: Changli Gao; +Cc: Eric Dumazet, David Miller, netdev
* Mathieu Desnoyers (mathieu.desnoyers@polymtl.ca) wrote:
> * Changli Gao (xiaosuo@gmail.com) wrote:
> > On Thu, Aug 19, 2010 at 11:46 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
[...]
> > > + peer = cmpxchg(&rt->peer, NULL, peer);
> > > +
> > > + if (unlikely(peer))
> >
> > It isn't correct, and should be
> > if (unlikely(cmpxchg(&rt->peer, NULL, peer) != NULL))
> >
> > > inet_putpeer(peer);
> > > }
>
> What do you mean by "It isn't correct" ? Eric code and yours perform the
> exact same thing. So I assume this is just a code style issue here.
Nevermind my comment, it's not accurate.
Thanks,
Mathieu
--
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next-2.6 v2] net: build_ehash_secret() and rt_bind_peer() cleanups
2010-08-19 16:10 ` [PATCH net-next-2.6 v2] " Eric Dumazet
@ 2010-08-20 7:51 ` David Miller
0 siblings, 0 replies; 8+ messages in thread
From: David Miller @ 2010-08-20 7:51 UTC (permalink / raw)
To: eric.dumazet; +Cc: xiaosuo, netdev, mathieu.desnoyers
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 19 Aug 2010 18:10:45 +0200
> [PATCH net-next-2.6] net: build_ehash_secret() and rt_bind_peer() cleanups
>
> Now cmpxchg() is available on all arches, we can use it in
> build_ehash_secret() and rt_bind_peer() instead of using spinlocks.
>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Applied, thanks Eric.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2010-08-20 7:50 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-08-19 15:46 [PATCH net-next-2.6] net: build_ehash_secret() and rt_bind_peer() cleanups Eric Dumazet
2010-08-19 15:58 ` Changli Gao
2010-08-19 16:10 ` [PATCH net-next-2.6 v2] " Eric Dumazet
2010-08-20 7:51 ` David Miller
2010-08-19 16:10 ` [PATCH net-next-2.6] " Changli Gao
2010-08-19 16:13 ` Mathieu Desnoyers
2010-08-19 16:30 ` Mathieu Desnoyers
2010-08-19 16:28 ` Mathieu Desnoyers
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).