[PATCH 13/24] [IPSEC]: Move x->outer_mode->output out of locked section
[IPSEC]: Move x->outer_mode->output out of locked section
RO mode is the only one that requires a locked output function. So it's
easier to move the lock into that function rather than requiring everyone
else to run under the lock.
In particular, this allows us to move the size check into the output
function without causing a potential dead-lock should the ICMP error
somehow hit the same SA on transmission.
Signed-off-by: Herbert Xu <[EMAIL PROTECTED]>
---
net/ipv6/xfrm6_mode_ro.c |3 +++
net/xfrm/xfrm_output.c |8
2 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/net/ipv6/xfrm6_mode_ro.c b/net/ipv6/xfrm6_mode_ro.c
index a7bc8c6..4a01cb3 100644
--- a/net/ipv6/xfrm6_mode_ro.c
+++ b/net/ipv6/xfrm6_mode_ro.c
@@ -28,6 +28,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -53,7 +54,9 @@ static int xfrm6_ro_output(struct xfrm_state *x, struct
sk_buff *skb)
__skb_pull(skb, hdr_len);
memmove(ipv6_hdr(skb), iph, hdr_len);
+ spin_lock_bh(&x->lock);
x->lastused = get_seconds();
+ spin_unlock_bh(&x->lock);
return 0;
}
diff --git a/net/xfrm/xfrm_output.c b/net/xfrm/xfrm_output.c
index 58d5a74..b1efdc8 100644
--- a/net/xfrm/xfrm_output.c
+++ b/net/xfrm/xfrm_output.c
@@ -53,6 +53,10 @@ int xfrm_output(struct sk_buff *skb)
}
do {
+ err = x->outer_mode->output(x, skb);
+ if (err)
+ goto error;
+
spin_lock_bh(&x->lock);
err = xfrm_state_check(x, skb);
if (err)
@@ -64,10 +68,6 @@ int xfrm_output(struct sk_buff *skb)
xfrm_replay_notify(x, XFRM_REPLAY_UPDATE);
}
- err = x->outer_mode->output(x, skb);
- if (err)
- goto error;
-
x->curlft.bytes += skb->len;
x->curlft.packets++;
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 13/24] [IPSEC]: Move x->outer_mode->output out of locked section
Hi Herbert, Herbert Xu schrieb: > diff --git a/net/ipv6/xfrm6_mode_ro.c b/net/ipv6/xfrm6_mode_ro.c > index a7bc8c6..4a01cb3 100644 > --- a/net/ipv6/xfrm6_mode_ro.c > +++ b/net/ipv6/xfrm6_mode_ro.c > @@ -53,7 +54,9 @@ static int xfrm6_ro_output(struct xfrm_state *x, struct > sk_buff *skb) > __skb_pull(skb, hdr_len); > memmove(ipv6_hdr(skb), iph, hdr_len); > > + spin_lock_bh(&x->lock); > x->lastused = get_seconds(); > + spin_unlock_bh(&x->lock); > > return 0; > } Can you move the retrieval of the seconds outside the spinlock? e.g. tmp = get_seconds(); spin_lock_bh(&x->lock); x->lastused = tmp; spin_unlock_bh(&x->lock); or is it not really worth it? Best Regards Ingo Oeser - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 13/24] [IPSEC]: Move x->outer_mode->output out of locked section
On Wed, Nov 07, 2007 at 05:17:42PM +0100, Ingo Oeser wrote:
> Hi Herbert,
>
> Herbert Xu schrieb:
> > diff --git a/net/ipv6/xfrm6_mode_ro.c b/net/ipv6/xfrm6_mode_ro.c
> > index a7bc8c6..4a01cb3 100644
> > --- a/net/ipv6/xfrm6_mode_ro.c
> > +++ b/net/ipv6/xfrm6_mode_ro.c
> > @@ -53,7 +54,9 @@ static int xfrm6_ro_output(struct xfrm_state *x, struct
> > sk_buff *skb)
> > __skb_pull(skb, hdr_len);
> > memmove(ipv6_hdr(skb), iph, hdr_len);
> >
> > + spin_lock_bh(&x->lock);
> > x->lastused = get_seconds();
> > + spin_unlock_bh(&x->lock);
> >
> > return 0;
> > }
>
> Can you move the retrieval of the seconds outside the spinlock?
You certainly could. Whether it's worth it I won't speculate :)
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[EMAIL PROTECTED]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 13/24] [IPSEC]: Move x->outer_mode->output out of locked section
From: Herbert Xu <[EMAIL PROTECTED]> Date: Thu, 8 Nov 2007 08:39:03 +0800 > On Wed, Nov 07, 2007 at 05:17:42PM +0100, Ingo Oeser wrote: > > Hi Herbert, > > > > Herbert Xu schrieb: > > > diff --git a/net/ipv6/xfrm6_mode_ro.c b/net/ipv6/xfrm6_mode_ro.c > > > index a7bc8c6..4a01cb3 100644 > > > --- a/net/ipv6/xfrm6_mode_ro.c > > > +++ b/net/ipv6/xfrm6_mode_ro.c > > > @@ -53,7 +54,9 @@ static int xfrm6_ro_output(struct xfrm_state *x, struct > > > sk_buff *skb) > > > __skb_pull(skb, hdr_len); > > > memmove(ipv6_hdr(skb), iph, hdr_len); > > > > > > + spin_lock_bh(&x->lock); > > > x->lastused = get_seconds(); > > > + spin_unlock_bh(&x->lock); > > > > > > return 0; > > > } > > > > Can you move the retrieval of the seconds outside the spinlock? > > You certainly could. Whether it's worth it I won't speculate :) Make 'lastused' an 'unsigned long' (that's all that get_seconds() gives to us anyways), fix up the nla_total_size(x->lastused) thing in net/xfrm/xfrm_user.c, and then you can remove this lock acquisition completely because the store into x->lastused will now be atomic and therefore locks aren't protecting anything. - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 13/24] [IPSEC]: Move x->outer_mode->output out of locked section
On Tue, Nov 13, 2007 at 03:33:48AM -0800, David Miller wrote:
>
> Make 'lastused' an 'unsigned long' (that's all that get_seconds()
> gives to us anyways), fix up the nla_total_size(x->lastused) thing in
> net/xfrm/xfrm_user.c, and then you can remove this lock acquisition
> completely because the store into x->lastused will now be atomic and
> therefore locks aren't protecting anything.
Brilliant, make that patch 25/25 :)
[IPSEC]: Make x->lastused an unsigned long
Currently x->lastused is u64 which means that it cannot be read/written
atomically on all architectures. David Miller observed that the value
stored in it is only an unsigned long which is always atomic.
So based on his suggestion this patch changes the internal representation
from u64 to unsigned long while the user-interface still refers to it as
u64.
Signed-off-by: Herbert Xu <[EMAIL PROTECTED]>
---
include/net/xfrm.h |2 +-
net/ipv6/xfrm6_mode_ro.c |2 --
net/xfrm/xfrm_user.c |4 ++--
3 files changed, 3 insertions(+), 5 deletions(-)
diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index 944fdad..e184c11 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -183,7 +183,7 @@ struct xfrm_state
struct timer_list timer;
/* Last used time */
- u64 lastused;
+ unsigned long lastused;
/* Reference to data common to all the instances of this
* transformer. */
diff --git a/net/ipv6/xfrm6_mode_ro.c b/net/ipv6/xfrm6_mode_ro.c
index 4a01cb3..63d5d49 100644
--- a/net/ipv6/xfrm6_mode_ro.c
+++ b/net/ipv6/xfrm6_mode_ro.c
@@ -54,9 +54,7 @@ static int xfrm6_ro_output(struct xfrm_state *x, struct
sk_buff *skb)
__skb_pull(skb, hdr_len);
memmove(ipv6_hdr(skb), iph, hdr_len);
- spin_lock_bh(&x->lock);
x->lastused = get_seconds();
- spin_unlock_bh(&x->lock);
return 0;
}
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index d41588d..02cf26f 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -1993,8 +1993,8 @@ static inline size_t xfrm_sa_len(struct xfrm_state *x)
if (x->coaddr)
l += nla_total_size(sizeof(*x->coaddr));
- /* Must count this as this may become non-zero behind our back. */
- l += nla_total_size(sizeof(x->lastused));
+ /* Must count x->lastused as it may become non-zero behind our back. */
+ l += nla_total_size(sizeof(u64));
return l;
}
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[EMAIL PROTECTED]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 13/24] [IPSEC]: Move x->outer_mode->output out of locked section
From: Herbert Xu <[EMAIL PROTECTED]> Date: Wed, 07 Nov 2007 22:08:29 +0800 > [IPSEC]: Move x->outer_mode->output out of locked section > > RO mode is the only one that requires a locked output function. So it's > easier to move the lock into that function rather than requiring everyone > else to run under the lock. > > In particular, this allows us to move the size check into the output > function without causing a potential dead-lock should the ICMP error > somehow hit the same SA on transmission. > > Signed-off-by: Herbert Xu <[EMAIL PROTECTED]> Applied to net-2.6.25 - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 13/24] [IPSEC]: Move x->outer_mode->output out of locked section
From: Herbert Xu <[EMAIL PROTECTED]> Date: Tue, 13 Nov 2007 19:51:19 +0800 > On Tue, Nov 13, 2007 at 03:33:48AM -0800, David Miller wrote: > > > > Make 'lastused' an 'unsigned long' (that's all that get_seconds() > > gives to us anyways), fix up the nla_total_size(x->lastused) thing in > > net/xfrm/xfrm_user.c, and then you can remove this lock acquisition > > completely because the store into x->lastused will now be atomic and > > therefore locks aren't protecting anything. > > Brilliant, make that patch 25/25 :) > > [IPSEC]: Make x->lastused an unsigned long > > Currently x->lastused is u64 which means that it cannot be read/written > atomically on all architectures. David Miller observed that the value > stored in it is only an unsigned long which is always atomic. > > So based on his suggestion this patch changes the internal representation > from u64 to unsigned long while the user-interface still refers to it as > u64. > > Signed-off-by: Herbert Xu <[EMAIL PROTECTED]> Applied to net-2.6.25 :) - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
