[PATCH 3/9] net: xfrm: use this_cpu_ptr per-cpu helper

[PATCH 3/9] net: xfrm: use this_cpu_ptr per-cpu helper

[Shan Wei]

From: Shan Wei <davidshan@tencent.com>


Signed-off-by: Shan Wei <davidshan@tencent.com>
---
 net/xfrm/xfrm_ipcomp.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/net/xfrm/xfrm_ipcomp.c b/net/xfrm/xfrm_ipcomp.c
index e5246fb..af6c78a 100644
--- a/net/xfrm/xfrm_ipcomp.c
+++ b/net/xfrm/xfrm_ipcomp.c
@@ -276,14 +276,13 @@ static struct crypto_comp * __percpu *ipcomp_alloc_tfms(const char *alg_name)
 	struct crypto_comp * __percpu *tfms;
 	int cpu;
 
-	/* This can be any valid CPU ID so we don't need locking. */
-	cpu = raw_smp_processor_id();
-
 	list_for_each_entry(pos, &ipcomp_tfms_list, list) {
 		struct crypto_comp *tfm;
 
 		tfms = pos->tfms;
-		tfm = *per_cpu_ptr(tfms, cpu);
+
+		/* This can be any valid CPU ID so we don't need locking. */
+		tfm = *this_cpu_ptr(tfms);
 
 		if (!strcmp(crypto_comp_name(tfm), alg_name)) {
 			pos->users++;
-- 
1.7.1

Re: [PATCH 3/9] net: xfrm: use this_cpu_ptr per-cpu helper

[Christoph Lameter]

On Wed, 31 Oct 2012, Shan Wei wrote:

> -
>  	list_for_each_entry(pos, &ipcomp_tfms_list, list) {
>  		struct crypto_comp *tfm;
>
>  		tfms = pos->tfms;
> -		tfm = *per_cpu_ptr(tfms, cpu);
> +
> +		/* This can be any valid CPU ID so we don't need locking. */
> +		tfm = *this_cpu_ptr(tfms);

It would be better to use

	this_cpu_read(tfms)

since that would also make it atomic vs interrupts. The above code (both
original and modified) could determine a pointer to a per cpu structure
and then take an interrupt which would move the task. On return we would
be accessing the per cpu variable of another processor.

Re: [PATCH 3/9] net: xfrm: use this_cpu_ptr per-cpu helper

[Herbert Xu]

On Wed, Oct 31, 2012 at 05:35:46PM +0000, Christoph Lameter wrote:
> On Wed, 31 Oct 2012, Shan Wei wrote:
> 
> > -
> >  	list_for_each_entry(pos, &ipcomp_tfms_list, list) {
> >  		struct crypto_comp *tfm;
> >
> >  		tfms = pos->tfms;
> > -		tfm = *per_cpu_ptr(tfms, cpu);
> > +
> > +		/* This can be any valid CPU ID so we don't need locking. */
> > +		tfm = *this_cpu_ptr(tfms);
> 
> It would be better to use
> 
> 	this_cpu_read(tfms)
> 
> since that would also make it atomic vs interrupts. The above code (both
> original and modified) could determine a pointer to a per cpu structure
> and then take an interrupt which would move the task. On return we would
> be accessing the per cpu variable of another processor.

Please refer to the comment in the patch above.

But I think the patch is wrong anyway because it would introduce
a warning, no?

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 3/9] net: xfrm: use this_cpu_ptr per-cpu helper

[Shan Wei]

Herbert Xu said, at 2012/11/1 11:41:
> Please refer to the comment in the patch above.
> 
> But I think the patch is wrong anyway because it would introduce
> a warning, no?

yes, __this_cpu_ptr(or __this_cpu_read) is more reasonable 
which don't check preemption context.

> 
> Thanks,
> 

Re: [PATCH 3/9] net: xfrm: use this_cpu_ptr per-cpu helper

[Shan Wei]

Christoph Lameter said, at 2012/11/1 1:35:
> It would be better to use
> 
> 	this_cpu_read(tfms)
> 
> since that would also make it atomic vs interrupts. The above code (both
> original and modified) could determine a pointer to a per cpu structure
> and then take an interrupt which would move the task. On return we would
> be accessing the per cpu variable of another processor.

this_cpu_read
|-----_this_cpu_generic_read

#define _this_cpu_generic_read(pcp)                                     \
({      typeof(pcp) ret__;                                              \
        preempt_disable();                                              \
        ret__ = *this_cpu_ptr(&(pcp));                                  \
        preempt_enable();                                               \
        ret__;                                                          \
})


this_cpu_read operations locate per-cpu variable with preemption safe， not
disable interrupts. why is it atomic vs interrupts?

I have no idea whether we need to disable preemption for this code?
At least, xfrm code run well with per_cpu_ptr which don't disable preemption.

Re: [PATCH 3/9] net: xfrm: use this_cpu_ptr per-cpu helper

[Steffen Klassert]

On Thu, Nov 01, 2012 at 04:56:54PM +0800, Shan Wei wrote:
> Christoph Lameter said, at 2012/11/1 1:35:
> > It would be better to use
> > 
> > 	this_cpu_read(tfms)
> > 
> > since that would also make it atomic vs interrupts. The above code (both
> > original and modified) could determine a pointer to a per cpu structure
> > and then take an interrupt which would move the task. On return we would
> > be accessing the per cpu variable of another processor.
> 
> this_cpu_read
> |-----_this_cpu_generic_read
> 
> #define _this_cpu_generic_read(pcp)                                     \
> ({      typeof(pcp) ret__;                                              \
>         preempt_disable();                                              \
>         ret__ = *this_cpu_ptr(&(pcp));                                  \
>         preempt_enable();                                               \
>         ret__;                                                          \
> })
> 
> 
> this_cpu_read operations locate per-cpu variable with preemption safe， not
> disable interrupts. why is it atomic vs interrupts?
> 
> I have no idea whether we need to disable preemption for this code?
> At least, xfrm code run well with per_cpu_ptr which don't disable preemption.

We compare the name of the newly allocated crypto transform against the
existing ones. The name is the same on every percpu transform and it
does not change after the transform is allocated. So we don't care
if we get migrated or not, we can just take the transform from an
arbitrary cpu to read the name from it.

RE: [PATCH 3/9] net: xfrm: use this_cpu_ptr per-cpu helper

[David Laight]

> this_cpu_read
> |-----_this_cpu_generic_read
> 
> #define _this_cpu_generic_read(pcp)                                     \
> ({      typeof(pcp) ret__;                                              \
>         preempt_disable();                                              \
>         ret__ = *this_cpu_ptr(&(pcp));                                  \
>         preempt_enable();                                               \
>         ret__;                                                          \
> })
> 
> 
> this_cpu_read operations locate per-cpu variable with preemption safe， not
> disable interrupts. why is it atomic vs interrupts?

Hmmm...  what effect do those preemt_dis/enable() actually have?
Since a pre-empt can happen either side of them, the value
the caller sees can be for the wrong cpu anyway.

The only time I could see them being necessary is if
*this_cpu_ptr() itself needs mutex protection in order to
function correctly - and that is likely to be port specific.
On i386/amd64 where (I guess) it is an access offset by fs/gs
this isn't necessary and just wastes cpu cycles.

If the caller cares which cpu the value comes from (eg to
increment a counter) then the caller would need to disable
pre-emption across the whole operation.

	David