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

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

[Priyanka Jain]

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 <Priyanka.Jain@freescale.com>
---
 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

[David Miller]

From: Priyanka Jain <Priyanka.Jain@freescale.com>
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 <Priyanka.Jain@freescale.com>

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

[Jain Priyanka-B32167]

-----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 <Priyanka.Jain@freescale.com>
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 <Priyanka.Jain@freescale.com>

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

[David Miller]

From: Jain Priyanka-B32167 <B32167@freescale.com>
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 <Priyanka.Jain@freescale.com>
> 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 <Priyanka.Jain@freescale.com>
> 
> 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.

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

[Eric Dumazet]

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.


>  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

[Fan Du]

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:
>> 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.
>
>
>>   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

[Jain Priyanka-B32167]

-----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 <B32167@freescale.com>
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 <Priyanka.Jain@freescale.com>
> 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 <Priyanka.Jain@freescale.com>
> 
> 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.

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

[Jain Priyanka-B32167]

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:
>> 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.
>
>
>>   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

[Eric Dumazet]

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.