powerpc/vphn: fix endian issue in NUMA device node code
PATCH v2 1/1 powerpc/vphn: fix endian issue in NUMA device node code
The associativity domain numbers are obtained from the hypervisor through registers and written into memory by the guest: the packed array passed to vphn_unpack_associativity() is then native-endian, unlike what was assumed in the following commit:
commit b08a2a12e44eaec5024b2b969f4fcb98169d1ca3
Author: Alistair Popple <[email protected]> Date: Wed Aug 7 02:01:44 2013 +1000
powerpc: Make NUMA device node code endian safe
If a CPU home node changes, the topology gets filled with bogus values. This leads to severe performance breakdowns.
This patch does two things:
- extract values from the packed array with shifts, in order to be endian neutral
- convert the resulting values to be32 as expected
---
Changes in v2:
- removed the left out __be16 *field declaration
- removed the left out be16_to_cpup() call
- updated the comment of the magic formula
Thanks again Nish... the two left outs probably explain why PowerVM wasn't happy that patch. :P
--
Greg
arch/powerpc/mm/numa.c | 64 +++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 52 insertions(+), 12 deletions(-)
arch/powerpc/mm/numa.c +52 -12
@@ -1369,38 +1369,78 @@ static int update_cpu_associativity_changes_mask(void)#define VPHN_ASSOC_BUFSIZE (6*sizeof(u64)/sizeof(u32) + 1)/* +* The associativity values are either 16-bit (VPHN_FIELD_MSB) or 32-bit (data +* or VPHN_FIELD_UNUSED). We hence need to parse the packed array into 16-bit +* chunks. Let's do that with bit shifts to be endian neutral. +* +* --- 16-bit chunks --> +* _________________________ +* | 0 | 1 | 2 | 3 | packed[0] +* ------------------------- +* _________________________ +* | 4 | 5 | 6 | 7 | packed[1] +* ------------------------- +* ... +* _________________________ +* | 20 | 21 | 22 | 23 | packed[5] +* ------------------------- +* 48 32 16 0 +* <------ bits -------- +* +* We need 48-bit shift for chunks 0,4,8,16,20 +* 32-bit shift for chunks 1,5,9,17,21 +* 16-bit shift for chunks 2,6,10,18,22 +* no shift for chunks 3,7,11,19,23 +* +* The 2 lo bits of the chunk index multiplied by 16 give the shift. +* The remaining hi bits divided by 4 give the index in packed[]. +* +* For example: +* chunk 0 = packed[0/4] >> (~0b00000 & 0b11) * 16 = bits 48..63 in packed[0] +* chunk 5 = packed[5/4] >> (~0b00101 & 0b11) * 16 = bits 32..47 in packed[1] +* chunk 22 = packed[22/4] >> (~0b10110 & 0b11) * 16 = bits 16..31 in packed[5] +*/ +static inline u16 read_vphn_chunk (const long *packed,unsigned int i) +{ +return packed[i >>2 ] >> ((~i &3 ) <<4 ); +} + +/* * Convert the associativity domain numbers returned from the hypervisor * to the sequence they would appear in the ibm,associativity property. */ static int vphn_unpack_associativity (const long *packed,__be32 *unpacked) { -int i, nr_assoc_doms =0 ; -const __be16 *field = (const __be16 *) packed; +unsigned int i, j, nr_assoc_doms =0 ;#define VPHN_FIELD_UNUSED (0xffff)#define VPHN_FIELD_MSB (0x8000)#define VPHN_FIELD_MASK (~VPHN_FIELD_MSB) -for (i =1 ; i <VPHN_ASSOC_BUFSIZE ; i++) { -if (be16_to_cpup (field) ==VPHN_FIELD_UNUSED ) { +for (i =1 , j =0 ; i <VPHN_ASSOC_BUFSIZE ; i++) { +u16 field =read_vphn_chunk (packed, j); + +if (field ==VPHN_FIELD_UNUSED ) {/* All significant fields processed, and remaining * fields contain the reserved value of all 1's. * Just store them. */ - unpacked[i] = *((__be32 *)field); - field +=2 ; - }else if (be16_to_cpup (field) &VPHN_FIELD_MSB ) { + unpacked[i] = (VPHN_FIELD_UNUSED <<16 | +VPHN_FIELD_UNUSED ); + j +=2 ; + }else if (field &VPHN_FIELD_MSB ) {/* Data is in the lower 15 bits of this field */ - unpacked[i] =cpu_to_be32 ( -be16_to_cpup (field) &VPHN_FIELD_MASK ); - field++; + unpacked[i] =cpu_to_be32 (field &VPHN_FIELD_MASK ); + j++; nr_assoc_doms++; }else {/* Data is in the lower 15 bits of this field * concatenated with the next 16 bit field */ - unpacked[i] = *((__be32 *)field); - field +=2 ; + unpacked[i] = +cpu_to_be32 ((u32 ) field <<16 | +read_vphn_chunk (packed, j +1 )); + j +=2 ; nr_assoc_doms++; } }
Quoted 14 lines · Greg Kurz
The associativity domain numbers are obtained from the hypervisor through registers and written into memory by the guest: the packed array passed to
vphn_unpack_associativity()is then native-endian, unlike what was assumed in the following commit:commit
b08a2a12e44eaec5024b2b969f4fcb98169d1ca3Author: Alistair Popple <[email protected]> Date: Wed Aug 7 02:01:44 2013 +1000powerpc: Make NUMA device node code endian safe
If a CPU home node changes, the topology gets filled with bogus values. This leads to severe performance breakdowns.
Quoted 7 lines
This patch does two things:
- extract values from the packed array with shifts, in order to be endian neutral
- convert the resulting values to be32 as expectedSuggested-by Anton Blanchard <[email protected]>Signed-off-by Greg Kurz <[email protected]>
Quoted 9 lines
---
Changes in v2:
- removed the left out__be16*field declaration
- removed the left outbe16_to_cpup()call
- updated the comment of the magic formulaThanks again Nish... the two left outs probably explain why PowerVM wasn't happy that patch. :P
Yep, I've tested this v2 patch successfully now!
-Nish
Michael Ellerman
reviewerQuoted 10 lines · Greg Kurz
The associativity domain numbers are obtained from the hypervisor through registers and written into memory by the guest: the packed array passed to
vphn_unpack_associativity()is then native-endian, unlike what was assumed in the following commit:This patch does two things:
- extract values from the packed array with shifts, in order to be endian neutral
- convert the resulting values to be32 as expected
Quoted 4 lines
Suggested-by Anton Blanchard <[email protected]>Signed-off-by Greg Kurz <[email protected]>Reviewed-by Nishanth Aravamudan <[email protected]>Tested-by Nishanth Aravamudan <[email protected]>
Hi Greg,
I'm a bit dense, it's after 8pm, but this seems like it's more complicated than it needs to be?
We get six 64-bit registers back from the hypervisor, they're cpu endian obviously, and each is defined to consist of four 2 byte fields.
So to unpack them, can't we just iterate over those six 64-bit values, which if we load them as 64-bit values will be back in cpu endian?
cheers
Quoted 22 lines · On Tue, 7 Oct 2014 20:28:23 +1100 (EST) Michael Ellerman <[email protected]>…
Quoted 10 lines · Greg Kurz
The associativity domain numbers are obtained from the hypervisor through registers and written into memory by the guest: the packed array passed to
vphn_unpack_associativity()is then native-endian, unlike what was assumed in the following commit:This patch does two things:
- extract values from the packed array with shifts, in order to be endian neutral
- convert the resulting values to be32 as expectedQuoted 4 lines
Suggested-by Anton Blanchard <[email protected]>Signed-off-by Greg Kurz <[email protected]>Reviewed-by Nishanth Aravamudan <[email protected]>Tested-by Nishanth Aravamudan <[email protected]>Hi Greg,
I'm a bit dense, it's after 8pm, but this seems like it's more complicated than it needs to be?
Quoted 8 lines
We get six 64-bit registers back from the hypervisor, they're cpu endian obviously, and each is defined to consist of four 2 byte fields.
So to unpack them, can't we just iterate over those six 64-bit values, which if we load them as 64-bit values will be back in cpu endian?
cheers
First, I was sure I had Cc'd Benjamin... sorry for this omission :)
Hi Michael,
Do you mean something like the following ?
arch/powerpc/mm/numa.c +3 -0
@@ -1421,8 +1421,11 @@ static long hcall_vphn(unsigned long cpu, __be32 *associativity)long retbuf[PLPAR_HCALL9_BUFSIZE ] = {0 };u64 flags =1 ;int hwcpu =get_hard_smp_processor_id (cpu); +int i; rc =plpar_hcall9 (H_HOME_NODE_ASSOCIATIVITY , retbuf, flags, hwcpu); +for (i =0 ; i <6 ; i++) + retbuf[i] =cpu_to_be64 (retbuf[i]);vphn_unpack_associativity (retbuf, associativity);return rc;
Sure it also works and is a lot simplier... but it adds an extra loop. Also, if the 3 first elements of the array contain 12 VPHN_FIELD_MSB fields, then we don't even need to swap the remaining elements: only the parsing code knows.
On the other hand, I understand this is not a hot path... so what should we do ?
Cheers.
--
Greg
Michael Ellerman
reviewerQuoted 14 lines · Greg Kurz
Quoted 11 lines · On Tue, 7 Oct 2014 20:28:23 +1100 (EST) Michael Ellerman <[email protected]>…
Quoted 9 lines · Greg Kurz
The associativity domain numbers are obtained from the hypervisor through registers and written into memory by the guest: the packed array passed to
vphn_unpack_associativity()is then native-endian, unlike what was assumed in the following commit:This patch does two things:
- extract values from the packed array with shifts, in order to be endian neutral
- convert the resulting values to be32 as expected
Quoted 6 lines
Quoted 2 lines
So to unpack them, can't we just iterate over those six 64-bit values, which if we load them as 64-bit values will be back in cpu endian?
Hi Michael,
Do you mean something like the following ?
Not exactly, but that's certainly a lot simpler than the original patch.
Quoted 7 lines
Sure it also works and is a lot simplier... but it adds an extra loop. Also, if the 3 first elements of the array contain 12
VPHN_FIELD_MSBfields, then we don't even need to swap the remaining elements: only the parsing code knows.On the other hand, I understand this is not a hot path... so what should we do ?
So I prefer this to your original patch. I know the original was cool and tricky but it was also complicated :)
What I was thinking of was that the actual parsing code, in vphn_unpack_associativity() would do the 64-bit loads. That will require changing the parsing loop quite a bit, but I think it might be worth it.
So can you resend this version with a proper changelog, and I'll merge that straight away as a bug fix. Then if you can look at reworking the parsing loop to avoid all the endian swapping on the input side that'd be great.
cheers