eal: fix alignment of RISCV xmm vector type

PATCH Applied Tyler Retzlaff 5 participants 7 messages Nov 15, 2023 – Nov 18, 2023 RISC-V

lore

Share & tools

lore

Tyler Retzlaff

author

2023-11-15 21:16 UTC #0 lore raw

PATCH 1/1 eal: fix alignment of RISCV xmm vector type

Fix the alignment for rte_xmm_t it should be 16 instead of 8 bytes.

Fixes f22e705ebf12 ("eal/riscv: support RISC-V architecture")

Cc [email protected]

Signed-off-by Tyler Retzlaff <[email protected]>

---
 lib/eal/riscv/include/rte_vect.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

lib/eal/riscv/include/rte_vect.h +1 -1

index 2f97f43..da9092a 100644
@@ -29,7 +29,7 @@
 	uint32_t	u32[XMM_SIZE / sizeof(uint32_t)];
 	uint64_t	u64[XMM_SIZE / sizeof(uint64_t)];
 	double		pd[XMM_SIZE / sizeof(double)];
-} __rte_aligned(8) rte_xmm_t;
+} __rte_aligned(16) rte_xmm_t;
 
 static inline xmm_t
 vect_load_128(void *p)

Signature · 1 line

1.8.3.1

Morten Brørup

Reviewed-by on v1 1/1

2023-11-15 21:31 UTC #1 lore raw

Quoted 10 lines

From: Tyler Retzlaff [mailto:[email protected]]
Sent: Wednesday, 15 November 2023 22.17
Fix the alignment for rte_xmm_t it should be 16 instead of 8 bytes.
Fixes f22e705ebf12 ("eal/riscv: support RISC-V architecture")
Cc [email protected]
Cc [email protected]
Signed-off-by Tyler Retzlaff <[email protected]>
---

Reviewed-by Morten Brørup <[email protected]>

As mentioned in the other thread:

We need to urgently decide if this bug should live on in DPDK 23.11, or if the fix should be included although we are very late in the release process.

Stanislaw, what do you think?

Furthermore, I wonder if it can be backported to stable, and to what extent backporting it would break the ABI/API.

Stanisław Kardach

Reviewed-by

2023-11-15 23:21 UTC #2 lore raw

Quoted 12 lines · Morten Brørup

Quoted 11 lines
From: Tyler Retzlaff [mailto:[email protected]]
Sent: Wednesday, 15 November 2023 22.17
Fix the alignment for rte_xmm_t it should be 16 instead of 8 bytes.
Fixes f22e705ebf12 ("eal/riscv: support RISC-V architecture")
Cc [email protected]
Cc [email protected]
Signed-off-by Tyler Retzlaff <[email protected]>
---

Quoted 7 lines

Reviewed-by Morten Brørup <[email protected]>
As mentioned in the other thread:
We need to urgently decide if this bug should live on in DPDK 23.11, or if the fix should be included although we are very late in the release process.
Stanislaw, what do you think?

Good catch! As for backporting I'm not sure of the urgency given that our examples still use scalar instructions for handling xmm_t. The question is whether there is a platform in use which has vector extensions enabled and that utilizes DPDK. I'm not that sure of it though I'd be happy to be proven wrong.

Reviewed-by Stanislaw Kardach <[email protected]>

Quoted 3 lines

Furthermore, I wonder if it can be backported to stable, and to what extent backporting it would break the ABI/API.

--
Best Regards,
Stanisław Kardach

Morten Brørup

2023-11-16 07:45 UTC #3 lore raw

Quoted 25 lines

From: Stanisław Kardach [mailto:[email protected]]
Sent: Thursday, 16 November 2023 00.21
Quoted 12 lines · Morten Brørup
Quoted 11 lines
From: Tyler Retzlaff [mailto:[email protected]]
Sent: Wednesday, 15 November 2023 22.17
Fix the alignment for rte_xmm_t it should be 16 instead of 8 bytes.
Fixes f22e705ebf12 ("eal/riscv: support RISC-V architecture")
Cc [email protected]
Cc [email protected]
Signed-off-by Tyler Retzlaff <[email protected]>
---
Quoted 5 lines
Reviewed-by Morten Brørup <[email protected]>
As mentioned in the other thread:
We need to urgently decide if this bug should live on in DPDK 23.11,
or if the fix should be included although we are very late in the release process.
Quoted 1 line

Quoted 6 lines

Quoted 1 line
Stanislaw, what do you think?
Good catch! As for backporting I'm not sure of the urgency given that our examples still use scalar instructions for handling xmm_t. The question is whether there is a platform in use which has vector extensions enabled and that utilizes DPDK. I'm not that sure of it though I'd be happy to be proven wrong.

Can we extrapolate this, and also conclude that postponing this bugfix until the next ABI/API breaking release, DPDK 24.11, is not really going to hurt anyone?

Stanislaw, please confirm?

Bruce, I don't feel 100 % confident in making this postponement recommendation. Could you please provide a second opinion regarding the timing of fixing this bug? Or rather: do you have any strong arguments *against* postponing it to DPDK 24.11?

Quoted 6 lines

Reviewed-by Stanislaw Kardach <[email protected]>
Quoted 2 lines
Furthermore, I wonder if it can be backported to stable, and to what
extent backporting it would break the ABI/API.

Based on your input regarding the current deployment status, backporting the bugfix clearly isn't worth the effort. Excellent!

Quoted 6 lines

Quoted 1 line
--
Best Regards,
Stanisław Kardach

Bruce Richardson

2023-11-17 10:54 UTC #4 lore raw

Quoted 32 lines · Morten Brørup

Quoted 25 lines
From: Stanisław Kardach [mailto:[email protected]]
Sent: Thursday, 16 November 2023 00.21
Quoted 12 lines · Morten Brørup
Quoted 11 lines
From: Tyler Retzlaff [mailto:[email protected]]
Sent: Wednesday, 15 November 2023 22.17
Fix the alignment for rte_xmm_t it should be 16 instead of 8 bytes.
Fixes f22e705ebf12 ("eal/riscv: support RISC-V architecture")
Cc [email protected]
Cc [email protected]
Signed-off-by Tyler Retzlaff <[email protected]>
---
Quoted 5 lines
Reviewed-by Morten Brørup <[email protected]>
As mentioned in the other thread:
We need to urgently decide if this bug should live on in DPDK 23.11,
or if the fix should be included although we are very late in the release process.
Quoted 1 line
Quoted 6 lines
Quoted 1 line
Stanislaw, what do you think?
Good catch! As for backporting I'm not sure of the urgency given that our examples still use scalar instructions for handling xmm_t. The question is whether there is a platform in use which has vector extensions enabled and that utilizes DPDK. I'm not that sure of it though I'd be happy to be proven wrong.

Quoted 6 lines

Can we extrapolate this, and also conclude that postponing this bugfix until the next ABI/API breaking release, DPDK 24.11, is not really going to hurt anyone?
Stanislaw, please confirm?
Bruce, I don't feel 100 % confident in making this postponement recommendation. Could you please provide a second opinion regarding the timing of fixing this bug? Or rather: do you have any strong arguments *against* postponing it to DPDK 24.11?

Not sure I'm any better placed to make an argument either way! However, I would very much tend to say that we should include this in 23.11, on the basis that if it turns out to be important we can't backport it later without affecting ABI. Right now, the code looks broken to me, and I'm also struggling to find circumstances where increasing the alignment will actually stop something from working. There could well be performance implications of having extra padding, but things should still work, AFAIK. On the other hand, if we don't include the fix, it is possible that a system (possibly a future one) could break and segfault due to incorrect vector code. I'd take a possible slowdown over a segfault!

Is my assessment correct, or perhaps I'm missing some detail.

/Bruce

Morten Brørup

2023-11-17 11:18 UTC #5 lore raw

+CC Thomas, this patch is ready to be applied to 23.11.

Quoted 45 lines

From: Bruce Richardson [mailto:[email protected]]
Sent: Friday, 17 November 2023 11.55
Quoted 10 lines · Morten Brørup
Quoted 10 lines
From: Stanisław Kardach [mailto:[email protected]]
Sent: Thursday, 16 November 2023 00.21
Quoted 5 lines · Morten Brørup
Quoted 5 lines
From: Tyler Retzlaff [mailto:[email protected]]
Sent: Wednesday, 15 November 2023 22.17
Fix the alignment for rte_xmm_t it should be 16 instead of 8
bytes.
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
Fixes f22e705ebf12 ("eal/riscv: support RISC-V architecture")
Cc [email protected]
Cc [email protected]
Signed-off-by Tyler Retzlaff <[email protected]>
---
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Reviewed-by Morten Brørup <[email protected]>
As mentioned in the other thread:
We need to urgently decide if this bug should live on in DPDK
23.11,
Quoted 5 lines
Quoted 5 lines
or if the fix should be included although we are very late in the release process.
Quoted 2 lines
Stanislaw, what do you think?
Good catch! As for backporting I'm not sure of the urgency given
that
Quoted 6 lines
Quoted 4 lines
our examples still use scalar instructions for handling xmm_t. The question is whether there is a platform in use which has vector extensions enabled and that utilizes DPDK. I'm not that sure of it though I'd be happy to be proven wrong.
Can we extrapolate this, and also conclude that postponing this
bugfix until the next ABI/API breaking release, DPDK 24.11, is not really going to hurt anyone?
Quoted 3 lines
Stanislaw, please confirm?

Quoted 7 lines

Quoted 1 line
Bruce, I don't feel 100 % confident in making this postponement
recommendation. Could you please provide a second opinion regarding the timing of fixing this bug? Or rather: do you have any strong arguments *against* postponing it to DPDK 24.11?
Quoted 1 line
Not sure I'm any better placed to make an argument either way!

Bruce, I picked you because of your experience with vector code.

Quoted 6 lines

However, I
would very much tend to say that we should include this in 23.11, on the
basis that if it turns out to be important we can't backport it later without affecting ABI. Right now, the code looks broken to me, and I'm also

Quoted 8 lines

struggling to find circumstances where increasing the alignment will actually stop something from working. There could well be performance implications of having extra padding, but things should still work, AFAIK.
On the other hand, if we don't include the fix, it is possible that a system (possibly a future one) could break and segfault due to incorrect
vector code. I'd take a possible slowdown over a segfault!

The risk of slowdown isn't a factor for me at this stage.

I'm trying to balance the risk of fixing the bug vs. breaking something this late in the 23.11 release cycle.

You have a strong point that we also need to consider the bugfix in the context of the total lifetime of DPDK 23.11 in the wild. With RISC-V's current traction, that certainly speaks in favor of including it in 23.11.

Quoted 2 lines

Is my assessment correct, or perhaps I'm missing some detail.

Thank you for your valuable feedback, Bruce.

I was just being overly cautious... After all, 23.11 is still at a stage where bug fixes are accepted!

New conclusion: Let's get it into 23.11.

Thomas Monjalon

Applied

2023-11-18 08:04 UTC #6 lore raw

17/11/2023 12:18, Morten Brørup:

Quoted 80 lines

+CC Thomas, this patch is ready to be applied to 23.11.
Quoted 45 lines
From: Bruce Richardson [mailto:[email protected]]
Sent: Friday, 17 November 2023 11.55
Quoted 10 lines · Morten Brørup
Quoted 10 lines
From: Stanisław Kardach [mailto:[email protected]]
Sent: Thursday, 16 November 2023 00.21
Quoted 5 lines · Morten Brørup
Quoted 5 lines
From: Tyler Retzlaff [mailto:[email protected]]
Sent: Wednesday, 15 November 2023 22.17
Fix the alignment for rte_xmm_t it should be 16 instead of 8
bytes.
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
Fixes f22e705ebf12 ("eal/riscv: support RISC-V architecture")
Cc [email protected]
Cc [email protected]
Signed-off-by Tyler Retzlaff <[email protected]>
---
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Reviewed-by Morten Brørup <[email protected]>
As mentioned in the other thread:
We need to urgently decide if this bug should live on in DPDK
23.11,
Quoted 5 lines
Quoted 5 lines
or if the fix should be included although we are very late in the release process.
Quoted 2 lines
Stanislaw, what do you think?
Good catch! As for backporting I'm not sure of the urgency given
that
Quoted 6 lines
Quoted 4 lines
our examples still use scalar instructions for handling xmm_t. The question is whether there is a platform in use which has vector extensions enabled and that utilizes DPDK. I'm not that sure of it though I'd be happy to be proven wrong.
Can we extrapolate this, and also conclude that postponing this
bugfix until the next ABI/API breaking release, DPDK 24.11, is not really going to hurt anyone?
Quoted 3 lines
Stanislaw, please confirm?
Quoted 7 lines
Quoted 1 line
Bruce, I don't feel 100 % confident in making this postponement
recommendation. Could you please provide a second opinion regarding the timing of fixing this bug? Or rather: do you have any strong arguments *against* postponing it to DPDK 24.11?
Quoted 1 line
Not sure I'm any better placed to make an argument either way!
Bruce, I picked you because of your experience with vector code.
Quoted 6 lines
However, I
would very much tend to say that we should include this in 23.11, on the
basis that if it turns out to be important we can't backport it later without affecting ABI. Right now, the code looks broken to me, and I'm also
Quoted 8 lines
struggling to find circumstances where increasing the alignment will actually stop something from working. There could well be performance implications of having extra padding, but things should still work, AFAIK.
On the other hand, if we don't include the fix, it is possible that a system (possibly a future one) could break and segfault due to incorrect
vector code. I'd take a possible slowdown over a segfault!
The risk of slowdown isn't a factor for me at this stage.
I'm trying to balance the risk of fixing the bug vs. breaking something this late in the 23.11 release cycle.
You have a strong point that we also need to consider the bugfix in the context of the total lifetime of DPDK 23.11 in the wild. With RISC-V's current traction, that certainly speaks in favor of including it in 23.11.
Quoted 1 line

Quoted 7 lines

Quoted 1 line
Is my assessment correct, or perhaps I'm missing some detail.
Thank you for your valuable feedback, Bruce.
I was just being overly cautious... After all, 23.11 is still at a stage where bug fixes are accepted!
New conclusion: Let's get it into 23.11.

Applied, thanks.

I've kept CC:stable as it is a real fix.
I don't see a problem in breaking API/ABI for RISC-V which is quite new. Anyway stable maintainers will choose what to do.