[PATCH][XFRM] Replace rwlock on xfrm_policy_afinfo with rcu

Subsystems: networking [general], networking [ipsec], the rest

STALE5104d

9 messages, 5 authors, 2012-08-10 · open the first message on its own page

[PATCH][XFRM] Replace rwlock on xfrm_policy_afinfo with rcu

From: Priyanka Jain <hidden>
Date: 2012-08-07 05:22:17

xfrm_policy_afinfo is read mosly data structure.
Write on xfrm_policy_afinfo is done only at the
time of configuration.
So rwlocks can be safely replaced with RCU.

RCUs usage optimizes the performance.

Signed-off-by: Priyanka Jain <redacted>
---
 For IPSEC fwd test 
 -On p4080ds (8-core, SMP system)
	 Around 110% throughput increase in case of PREEMPT_RT enabled
	 Around 5-6% throughput increase in case of PREEMPT_RT disabled

 -On p2020 (2-core, SMP system)
	 Around 4-5% throughput increase in case of PREEMPT_RT disabled

 net/xfrm/xfrm_policy.c |   37 +++++++++++++++++++++----------------
 1 files changed, 21 insertions(+), 16 deletions(-)

diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 5082bcf..0ca3258 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c

@@ -41,13 +41,14 @@ static DEFINE_SPINLOCK(xfrm_policy_sk_bundle_lock);
 static struct dst_entry *xfrm_policy_sk_bundles;
 static DEFINE_RWLOCK(xfrm_policy_lock);
 
-static DEFINE_RWLOCK(xfrm_policy_afinfo_lock);
-static struct xfrm_policy_afinfo *xfrm_policy_afinfo[NPROTO];
+static DEFINE_SPINLOCK(xfrm_policy_afinfo_lock);
+static struct xfrm_policy_afinfo __rcu * xfrm_policy_afinfo[NPROTO]
+						__read_mostly;
 
 static struct kmem_cache *xfrm_dst_cache __read_mostly;
 
 static struct xfrm_policy_afinfo *xfrm_policy_get_afinfo(unsigned short family);
-static void xfrm_policy_put_afinfo(struct xfrm_policy_afinfo *afinfo);
+static inline void xfrm_policy_put_afinfo(struct xfrm_policy_afinfo *afinfo);
 static void xfrm_init_pmtu(struct dst_entry *dst);
 static int stale_bundle(struct dst_entry *dst);
 static int xfrm_bundle_ok(struct xfrm_dst *xdst);

@@ -2436,7 +2437,7 @@ int xfrm_policy_register_afinfo(struct xfrm_policy_afinfo *afinfo)
 		return -EINVAL;
 	if (unlikely(afinfo->family >= NPROTO))
 		return -EAFNOSUPPORT;
-	write_lock_bh(&xfrm_policy_afinfo_lock);
+	spin_lock_bh(&xfrm_policy_afinfo_lock);
 	if (unlikely(xfrm_policy_afinfo[afinfo->family] != NULL))
 		err = -ENOBUFS;
 	else {

@@ -2455,9 +2456,9 @@ int xfrm_policy_register_afinfo(struct xfrm_policy_afinfo *afinfo)
 			dst_ops->link_failure = xfrm_link_failure;
 		if (likely(afinfo->garbage_collect == NULL))
 			afinfo->garbage_collect = __xfrm_garbage_collect;
-		xfrm_policy_afinfo[afinfo->family] = afinfo;
+		rcu_assign_pointer(xfrm_policy_afinfo[afinfo->family], afinfo);
 	}
-	write_unlock_bh(&xfrm_policy_afinfo_lock);
+	spin_unlock_bh(&xfrm_policy_afinfo_lock);
 
 	rtnl_lock();
 	for_each_net(net) {

@@ -2490,13 +2491,14 @@ int xfrm_policy_unregister_afinfo(struct xfrm_policy_afinfo *afinfo)
 		return -EINVAL;
 	if (unlikely(afinfo->family >= NPROTO))
 		return -EAFNOSUPPORT;
-	write_lock_bh(&xfrm_policy_afinfo_lock);
+	spin_lock_bh(&xfrm_policy_afinfo_lock);
 	if (likely(xfrm_policy_afinfo[afinfo->family] != NULL)) {
 		if (unlikely(xfrm_policy_afinfo[afinfo->family] != afinfo))
 			err = -EINVAL;
 		else {
 			struct dst_ops *dst_ops = afinfo->dst_ops;
-			xfrm_policy_afinfo[afinfo->family] = NULL;
+			rcu_assign_pointer(xfrm_policy_afinfo[afinfo->family],
+				NULL);
 			dst_ops->kmem_cachep = NULL;
 			dst_ops->check = NULL;
 			dst_ops->negative_advice = NULL;

@@ -2504,7 +2506,8 @@ int xfrm_policy_unregister_afinfo(struct xfrm_policy_afinfo *afinfo)
 			afinfo->garbage_collect = NULL;
 		}
 	}
-	write_unlock_bh(&xfrm_policy_afinfo_lock);
+	spin_unlock_bh(&xfrm_policy_afinfo_lock);
+	synchronize_rcu();
 	return err;
 }
 EXPORT_SYMBOL(xfrm_policy_unregister_afinfo);

@@ -2513,8 +2516,9 @@ static void __net_init xfrm_dst_ops_init(struct net *net)
 {
 	struct xfrm_policy_afinfo *afinfo;
 
-	read_lock_bh(&xfrm_policy_afinfo_lock);
-	afinfo = xfrm_policy_afinfo[AF_INET];
+	local_bh_disable();
+	rcu_read_lock();
+	afinfo = rcu_dereference(xfrm_policy_afinfo[AF_INET]);
 	if (afinfo)
 		net->xfrm.xfrm4_dst_ops = *afinfo->dst_ops;
 #if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)

@@ -2522,7 +2526,8 @@ static void __net_init xfrm_dst_ops_init(struct net *net)
 	if (afinfo)
 		net->xfrm.xfrm6_dst_ops = *afinfo->dst_ops;
 #endif
-	read_unlock_bh(&xfrm_policy_afinfo_lock);
+	rcu_read_unlock();
+	local_bh_enable();
 }
 
 static struct xfrm_policy_afinfo *xfrm_policy_get_afinfo(unsigned short family)

@@ -2530,16 +2535,16 @@ static struct xfrm_policy_afinfo *xfrm_policy_get_afinfo(unsigned short family)
 	struct xfrm_policy_afinfo *afinfo;
 	if (unlikely(family >= NPROTO))
 		return NULL;
-	read_lock(&xfrm_policy_afinfo_lock);
-	afinfo = xfrm_policy_afinfo[family];
+	rcu_read_lock();
+	afinfo = rcu_dereference(xfrm_policy_afinfo[family]);
 	if (unlikely(!afinfo))
-		read_unlock(&xfrm_policy_afinfo_lock);
+		rcu_read_unlock();
 	return afinfo;
 }
 
 static void xfrm_policy_put_afinfo(struct xfrm_policy_afinfo *afinfo)
 {
-	read_unlock(&xfrm_policy_afinfo_lock);
+	rcu_read_unlock();
 }
 
 static int xfrm_dev_event(struct notifier_block *this, unsigned long event, void *ptr)

-- 
1.7.4.1

Re: [PATCH][XFRM] Replace rwlock on xfrm_policy_afinfo with rcu

From: David Miller <davem@davemloft.net>
Date: 2012-08-07 23:21:39

From: Priyanka Jain <redacted>
Date: Tue, 7 Aug 2012 10:51:44 +0530

xfrm_policy_afinfo is read mosly data structure.
Write on xfrm_policy_afinfo is done only at the
time of configuration.
So rwlocks can be safely replaced with RCU.

RCUs usage optimizes the performance.

Signed-off-by: Priyanka Jain <redacted>

This patch doesn't apply to the net-next tree, please respin.

Also:

-			xfrm_policy_afinfo[afinfo->family] = NULL;
+			rcu_assign_pointer(xfrm_policy_afinfo[afinfo->family],
+				NULL);

Indent that NULL argument properly, it must line up with the first
column after the openning '(' on the previous line.

RE: [PATCH][XFRM] Replace rwlock on xfrm_policy_afinfo with rcu

From: Jain Priyanka-B32167 <hidden>
Date: 2012-08-08 04:54:12


-----Original Message-----
From: David Miller [mailto:davem@davemloft.net] 
Sent: Wednesday, August 08, 2012 4:52 AM
To: Jain Priyanka-B32167
Cc: netdev@vger.kernel.org
Subject: Re: [PATCH][XFRM] Replace rwlock on xfrm_policy_afinfo with rcu

From: Priyanka Jain <redacted>
Date: Tue, 7 Aug 2012 10:51:44 +0530

xfrm_policy_afinfo is read mosly data structure.
Write on xfrm_policy_afinfo is done only at the time of configuration.
So rwlocks can be safely replaced with RCU.

RCUs usage optimizes the performance.

Signed-off-by: Priyanka Jain <redacted>

This patch doesn't apply to the net-next tree, please respin.
[Priyanka]: I will send v2 after re-spinning against git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git.

Also:

-			xfrm_policy_afinfo[afinfo->family] = NULL;
+			rcu_assign_pointer(xfrm_policy_afinfo[afinfo->family],
+				NULL);

Indent that NULL argument properly, it must line up with the first column after the openning '(' on the previous line.
[Priyanka]: NULL has been pushed to next line to confirm to 80 characters per line rule. If I indent NULL to previous line, it will break 80 characters per line rule.
Please let me know your final say on this. I will make changes accordingly if required.

Thanks
Priyanka

Re: [PATCH][XFRM] Replace rwlock on xfrm_policy_afinfo with rcu

From: David Miller <davem@davemloft.net>
Date: 2012-08-08 05:55:34

From: Jain Priyanka-B32167 <redacted>
Date: Wed, 8 Aug 2012 04:53:42 +0000


-----Original Message-----
From: David Miller [mailto:davem@davemloft.net] 
Sent: Wednesday, August 08, 2012 4:52 AM
To: Jain Priyanka-B32167
Cc: netdev@vger.kernel.org
Subject: Re: [PATCH][XFRM] Replace rwlock on xfrm_policy_afinfo with rcu

From: Priyanka Jain <redacted>
Date: Tue, 7 Aug 2012 10:51:44 +0530

quoted

xfrm_policy_afinfo is read mosly data structure.
Write on xfrm_policy_afinfo is done only at the time of configuration.
So rwlocks can be safely replaced with RCU.

RCUs usage optimizes the performance.

Signed-off-by: Priyanka Jain <redacted>

This patch doesn't apply to the net-next tree, please respin.
[Priyanka]: I will send v2 after re-spinning against git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git.

Also:

quoted

-			xfrm_policy_afinfo[afinfo->family] = NULL;
+			rcu_assign_pointer(xfrm_policy_afinfo[afinfo->family],
+				NULL);

Indent that NULL argument properly, it must line up with the first column after the openning '(' on the previous line.
[Priyanka]: NULL has been pushed to next line to confirm to 80 characters per line rule. If I indent NULL to previous line, it will break 80 characters per line rule.
Please let me know your final say on this. I will make changes accordingly if required.

What in the world are you talking about?

The openning parenthesis of the rcu_assign_pointer() statement is not anywhere
close to the 80th column.

You're doing this:

		x(A,
	B);

and I want you to do this:

		x(A,
		  B);

RE: [PATCH][XFRM] Replace rwlock on xfrm_policy_afinfo with rcu

From: Jain Priyanka-B32167 <hidden>
Date: 2012-08-08 09:41:46


-----Original Message-----
From: David Miller [mailto:davem@davemloft.net] 
Sent: Wednesday, August 08, 2012 11:26 AM
To: Jain Priyanka-B32167
Cc: netdev@vger.kernel.org
Subject: Re: [PATCH][XFRM] Replace rwlock on xfrm_policy_afinfo with rcu

From: Jain Priyanka-B32167 <redacted>
Date: Wed, 8 Aug 2012 04:53:42 +0000


-----Original Message-----
From: David Miller [mailto:davem@davemloft.net]
Sent: Wednesday, August 08, 2012 4:52 AM
To: Jain Priyanka-B32167
Cc: netdev@vger.kernel.org
Subject: Re: [PATCH][XFRM] Replace rwlock on xfrm_policy_afinfo with 
rcu

From: Priyanka Jain <redacted>
Date: Tue, 7 Aug 2012 10:51:44 +0530

quoted

xfrm_policy_afinfo is read mosly data structure.
Write on xfrm_policy_afinfo is done only at the time of configuration.
So rwlocks can be safely replaced with RCU.

RCUs usage optimizes the performance.

Signed-off-by: Priyanka Jain <redacted>

This patch doesn't apply to the net-next tree, please respin.
[Priyanka]: I will send v2 after re-spinning against git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git.

Also:

quoted

-			xfrm_policy_afinfo[afinfo->family] = NULL;
+			rcu_assign_pointer(xfrm_policy_afinfo[afinfo->family],
+				NULL);

Indent that NULL argument properly, it must line up with the first column after the openning '(' on the previous line.
[Priyanka]: NULL has been pushed to next line to confirm to 80 characters per line rule. If I indent NULL to previous line, it will break 80 characters per line rule.
Please let me know your final say on this. I will make changes accordingly if required.

What in the world are you talking about?

The openning parenthesis of the rcu_assign_pointer() statement is not anywhere close to the 80th column.

You're doing this:

		x(A,
	B);

and I want you to do this:

		x(A,
		  B);

[Priyanka] Got it. Thanks. Will correct this.

Re: [PATCH][XFRM] Replace rwlock on xfrm_policy_afinfo with rcu

From: Eric Dumazet <hidden>
Date: 2012-08-08 06:25:51

On Tue, 2012-08-07 at 10:51 +0530, Priyanka Jain wrote:

xfrm_policy_afinfo is read mosly data structure.
Write on xfrm_policy_afinfo is done only at the
time of configuration.
So rwlocks can be safely replaced with RCU.

RCUs usage optimizes the performance.

quoted hunk

 static struct xfrm_policy_afinfo *xfrm_policy_get_afinfo(unsigned short family)

@@ -2530,16 +2535,16 @@ static struct xfrm_policy_afinfo *xfrm_policy_get_afinfo(unsigned short family)
 	struct xfrm_policy_afinfo *afinfo;
 	if (unlikely(family >= NPROTO))
 		return NULL;
-	read_lock(&xfrm_policy_afinfo_lock);
-	afinfo = xfrm_policy_afinfo[family];
+	rcu_read_lock();
+	afinfo = rcu_dereference(xfrm_policy_afinfo[family]);
 	if (unlikely(!afinfo))
-		read_unlock(&xfrm_policy_afinfo_lock);
+		rcu_read_unlock();

This makes no sense to me : We cant safely return afinfo here. 

Note the current code is buggy as well, this is worrying.

As soon as we exit from xfrm_policy_get_afinfo(), pointer might be
invalid.

Really, RCU conversion should be the right moment to spot those bugs and
first fix them (for stable trees)

Re: [PATCH][XFRM] Replace rwlock on xfrm_policy_afinfo with rcu

From: Fan Du <hidden>
Date: 2012-08-08 06:39:41

First, sorry to jump in.

On 2012年08月08日 14:25, Eric Dumazet wrote:

On Tue, 2012-08-07 at 10:51 +0530, Priyanka Jain wrote:

quoted

xfrm_policy_afinfo is read mosly data structure.
Write on xfrm_policy_afinfo is done only at the
time of configuration.
So rwlocks can be safely replaced with RCU.

RCUs usage optimizes the performance.

quoted

  static struct xfrm_policy_afinfo *xfrm_policy_get_afinfo(unsigned short family)

@@ -2530,16 +2535,16 @@ static struct xfrm_policy_afinfo *xfrm_policy_get_afinfo(unsigned short family)
  	struct xfrm_policy_afinfo *afinfo;
  	if (unlikely(family>= NPROTO))
  		return NULL;
-	read_lock(&xfrm_policy_afinfo_lock);
-	afinfo = xfrm_policy_afinfo[family];
+	rcu_read_lock();
+	afinfo = rcu_dereference(xfrm_policy_afinfo[family]);
  	if (unlikely(!afinfo))
-		read_unlock(&xfrm_policy_afinfo_lock);
+		rcu_read_unlock();

This makes no sense to me : We cant safely return afinfo here.

Note the current code is buggy as well, this is worrying.

As soon as we exit from xfrm_policy_get_afinfo(), pointer might be
invalid.

Yes, it might be invalid, but all the callers have checked the return
value, thus use it in a sane way.
So I don't follow "Note the current code is buggy as well".

Am I missing something here?

Really, RCU conversion should be the right moment to spot those bugs and
first fix them (for stable trees)



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 

Love each day!
--fan

RE: [PATCH][XFRM] Replace rwlock on xfrm_policy_afinfo with rcu

From: Jain Priyanka-B32167 <hidden>
Date: 2012-08-10 04:29:13

Hello Eric Dumazet,

Like Fan Du, even I could not see the bug in the current code. Can you please elaborate.

Thanks
Priyanka

-----Original Message-----
From: Fan Du [mailto:fan.du@windriver.com] 
Sent: Wednesday, August 08, 2012 12:14 PM
To: Eric Dumazet
Cc: Jain Priyanka-B32167; netdev@vger.kernel.org
Subject: Re: [PATCH][XFRM] Replace rwlock on xfrm_policy_afinfo with rcu

First, sorry to jump in.

On 2012年08月08日 14:25, Eric Dumazet wrote:

On Tue, 2012-08-07 at 10:51 +0530, Priyanka Jain wrote:

quoted

xfrm_policy_afinfo is read mosly data structure.
Write on xfrm_policy_afinfo is done only at the time of 
configuration.
So rwlocks can be safely replaced with RCU.

RCUs usage optimizes the performance.

quoted

  static struct xfrm_policy_afinfo *xfrm_policy_get_afinfo(unsigned 
short family) @@ -2530,16 +2535,16 @@ static struct xfrm_policy_afinfo *xfrm_policy_get_afinfo(unsigned short family)
  	struct xfrm_policy_afinfo *afinfo;
  	if (unlikely(family>= NPROTO))
  		return NULL;
-	read_lock(&xfrm_policy_afinfo_lock);
-	afinfo = xfrm_policy_afinfo[family];
+	rcu_read_lock();
+	afinfo = rcu_dereference(xfrm_policy_afinfo[family]);
  	if (unlikely(!afinfo))
-		read_unlock(&xfrm_policy_afinfo_lock);
+		rcu_read_unlock();

This makes no sense to me : We cant safely return afinfo here.

Note the current code is buggy as well, this is worrying.

As soon as we exit from xfrm_policy_get_afinfo(), pointer might be 
invalid.

Yes, it might be invalid, but all the callers have checked the return value, thus use it in a sane way.
So I don't follow "Note the current code is buggy as well".

Am I missing something here?

Really, RCU conversion should be the right moment to spot those bugs 
and first fix them (for stable trees)



--
To unsubscribe from this list: send the line "unsubscribe netdev" in 
the body of a message to majordomo@vger.kernel.org More majordomo info 
at  http://vger.kernel.org/majordomo-info.html

-- 

Love each day!
--fan

RE: [PATCH][XFRM] Replace rwlock on xfrm_policy_afinfo with rcu

From: Eric Dumazet <hidden>
Date: 2012-08-10 06:26:38

On Fri, 2012-08-10 at 04:28 +0000, Jain Priyanka-B32167 wrote:

Hello Eric Dumazet,

Like Fan Du, even I could not see the bug in the current code. Can you please elaborate
Yes, it might be invalid, but all the callers have checked the return value, thus use it in a sane way.
So I don't follow "Note the current code is buggy as well".

Am I missing something here?

Hmm, I misread the code, sorry for the false alarm.

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help