| Message ID | 1344316904-2544-1-git-send-email-Priyanka.Jain@freescale.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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. -- 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
-----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 -- 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
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); -- 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
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) -- 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
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 >
-----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. -- 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
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
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. -- 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
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)
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(-)