net: xfrm: use __this_cpu_read per-cpu helper

PATCH Applied Shan Wei 2 participants 6 messages Nov 13, 2012 – Nov 14, 2012

Subsystems (3)

Networking Network drivers Networking IPsec

lore

Share & tools

lore

Shan Wei

author

2012-11-13 01:52 UTC #0 lore raw

PATCH v4 3/9 net: xfrm: use __this_cpu_read per-cpu helper

From: Shan Wei <[email protected]>

Signed-off-by Shan Wei <[email protected]>

---
v4:
derefrence pointer before reading to avoid compile warning.

---
 net/xfrm/xfrm_ipcomp.c |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)

net/xfrm/xfrm_ipcomp.c +3 -5

index e5246fb..2906d52 100644
@@ -276,18 +276,16 @@ 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_read(*pos->tfms);
 
 		if (!strcmp(crypto_comp_name(tfm), alg_name)) {
 			pos->users++;
-			return tfms;
+			return pos->tfms;
 		}
 	}

Signature · 1 line

1.7.1

Steffen Klassert

maintainer

2012-11-13 07:21 UTC #1 lore raw

Quoted 2 lines · Shan Wei

From: Shan Wei <[email protected]>

Please add a proper commit message, explaining why you do this change.

Quoted 17 lines

Signed-off-by Shan Wei <[email protected]>
---
v4:
derefrence pointer before reading to avoid compile warning.
---
 net/xfrm/xfrm_ipcomp.c |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)
net/xfrm/xfrm_ipcomp.c +0 -0
index e5246fb..2906d52 100644
@@ -276,18 +276,16 @@ static struct crypto_comp * __percpu *ipcomp_alloc_tfms(const char *alg_name)
 	struct crypto_comp * __percpu *tfms;
 	int cpu;
 

net/xfrm/xfrm_ipcomp.c static struct crypto_comp * __percpu *ipcomp_alloc_tfms(const char *alg_name) · line 284

Quoted 10 lines

net/xfrm/xfrm_ipcomp.c +2 -4

-	/* 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_read(*pos->tfms);

This should just fetch the tfm pointer, so why exactly __this_cpu_read is better than __this_cpu_ptr? Please keep in mind that performance is not the most important thing here. It's much more important that it works in any case.

Shan Wei

author

2012-11-13 09:33 UTC #2 lore raw

Steffen Klassert said, at 2012/11/13 15:21:

Quoted 5 lines

This should just fetch the tfm pointer, so why exactly __this_cpu_read is better than __this_cpu_ptr? Please keep in mind that performance is not the most important thing here. It's much more important that it works in any case.

[0/9] describes why we should submit this series patches. you are not included in original mail.

this_cpu_ptr relocates and address. this_cpu_read() relocates the address and performs the fetch. If you want to operate on rda(defined as per_cpu) then you can only use this_cpu_ptr. this_cpu_read() saves you more instructions since it can do the relocation and the fetch in one instruction.

More info, please refer to http://www.spinics.net/lists/kernel/msg1435153.html

Steffen Klassert

maintainer

2012-11-13 10:48 UTC #3 lore raw

Quoted 7 lines · Shan Wei

Steffen Klassert said, at 2012/11/13 15:21:
Quoted 5 lines
This should just fetch the tfm pointer, so why exactly __this_cpu_read is better than __this_cpu_ptr? Please keep in mind that performance is not the most important thing here. It's much more important that it works in any case.

Quoted 8 lines

[0/9] describes why we should submit this series patches. you are not included in original mail.
this_cpu_ptr relocates and address. this_cpu_read() relocates the address and performs the fetch. If you want to operate on rda(defined as per_cpu) then you can only use this_cpu_ptr. this_cpu_read() saves you more instructions since it can do the relocation and the fetch in one instruction.

Ok, so please add a commit message to describe your changes.

Thanks.

Shan Wei

author

2012-11-13 12:36 UTC #4 lore raw

Steffen Klassert said, at 2012/11/13 18:48:

Quoted 5 lines

Ok, so please add a commit message to describe your changes.
Thanks.

[PATCH v5] net: xfrm: use __this_cpu_read per-cpu helper

this_cpu_ptr/this_cpu_read is faster than per_cpu_ptr(p, smp_processor_id()) and can reduce memory accesses.
The latter helper needs to find the offset for current cpu,
and needs more assembler instructions which objdump shows in following.

this_cpu_ptr relocates and address. this_cpu_read() relocates the address and performs the fetch. this_cpu_read() saves you more instructions since it can do the relocation and the fetch in one instruction.

per_cpu_ptr(p, smp_processor_id())：

  1e:   65 8b 04 25 00 00 00 00         mov    %gs:0x0,%eax
  26:   48 98                           cltq
  28:   31 f6                           xor    %esi,%esi
  2a:   48 c7 c7 00 00 00 00            mov    $0x0,%rdi
  31:   48 8b 04 c5 00 00 00 00         mov    0x0(,%rax,8),%rax
  39:   c7 44 10 04 14 00 00 00         movl   $0x14,0x4(%rax,%rdx,1)

this_cpu_ptr(p)

  1e:   65 48 03 14 25 00 00 00 00      add    %gs:0x0,%rdx
  27:   31 f6                           xor    %esi,%esi
  29:   c7 42 04 14 00 00 00            movl   $0x14,0x4(%rdx)
  30:   48 c7 c7 00 00 00 00            mov    $0x0,%rdi

Signed-off-by Shan Wei <[email protected]>

---
 net/xfrm/xfrm_ipcomp.c |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)

net/xfrm/xfrm_ipcomp.c +3 -5

index e5246fb..2906d52 100644
@@ -276,18 +276,16 @@ 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_read(*pos->tfms);
 
 		if (!strcmp(crypto_comp_name(tfm), alg_name)) {
 			pos->users++;
-			return tfms;
+			return pos->tfms;
 		}
 	}

Signature · 1 line

1.7.1

Steffen Klassert

maintainer Applied

2012-11-14 08:43 UTC #5 lore raw

Quoted 26 lines · Shan Wei

Steffen Klassert said, at 2012/11/13 18:48:
Quoted 5 lines
Ok, so please add a commit message to describe your changes.
Thanks.
[PATCH v5] net: xfrm: use __this_cpu_read per-cpu helper
this_cpu_ptr/this_cpu_read is faster than per_cpu_ptr(p, smp_processor_id()) and can reduce memory accesses.
The latter helper needs to find the offset for current cpu,
and needs more assembler instructions which objdump shows in following.
this_cpu_ptr relocates and address. this_cpu_read() relocates the address and performs the fetch. this_cpu_read() saves you more instructions since it can do the relocation and the fetch in one instruction.
per_cpu_ptr(p, smp_processor_id())：
  1e:   65 8b 04 25 00 00 00 00         mov    %gs:0x0,%eax
  26:   48 98                           cltq
  28:   31 f6                           xor    %esi,%esi
  2a:   48 c7 c7 00 00 00 00            mov    $0x0,%rdi
  31:   48 8b 04 c5 00 00 00 00         mov    0x0(,%rax,8),%rax
  39:   c7 44 10 04 14 00 00 00         movl   $0x14,0x4(%rax,%rdx,1)

Quoted 7 lines

this_cpu_ptr(p)

  1e:   65 48 03 14 25 00 00 00 00      add    %gs:0x0,%rdx
  27:   31 f6                           xor    %esi,%esi
  29:   c7 42 04 14 00 00 00            movl   $0x14,0x4(%rdx)
  30:   48 c7 c7 00 00 00 00            mov    $0x0,%rdi

Signed-off-by Shan Wei <[email protected]>

Applied to ipsec-next, thanks!