* [PATCH] powerpc64[le]: Allocate extra stack frame on syscall.S
@ 2021-12-07 18:52 Matheus Castanho
2021-12-15 14:39 ` [PATCH v2] " Matheus Castanho
0 siblings, 1 reply; 7+ messages in thread
From: Matheus Castanho @ 2021-12-07 18:52 UTC (permalink / raw)
To: libc-alpha; +Cc: murphyp, tuliom
The syscall function does not allocate the extra stack frame for scv like other
assembly syscalls using DO_CALL_SCV. So after commit d120fb9941 changed the
offset that is used to save LR, syscall ended up using an invalid offset,
causing regressions on powerpc64. So make sure the extra stack frame is
allocated in syscall.S as well to make it consistent with other uses of
DO_CALL_SCV and avoid similar issues in the future.
Tested on powerpc, powerpc64, and powerpc64le
---
sysdeps/unix/sysv/linux/powerpc/syscall.S | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/sysdeps/unix/sysv/linux/powerpc/syscall.S b/sysdeps/unix/sysv/linux/powerpc/syscall.S
index a29652feaf..037dd3c4c6 100644
--- a/sysdeps/unix/sysv/linux/powerpc/syscall.S
+++ b/sysdeps/unix/sysv/linux/powerpc/syscall.S
@@ -26,8 +26,12 @@ ENTRY (syscall)
mr r7,r8
mr r8,r9
#if defined(USE_PPC_SCV) && !IS_IN(rtld) && (defined(__PPC64__) || defined(__powerpc64__))
+ stdu r1,-SCV_FRAME_SIZE(r1)
+ cfi_adjust_cfa_offset(SCV_FRAME_SIZE)
CHECK_SCV_SUPPORT r9 0f
DO_CALL_SCV
+ addi r1,r1,SCV_FRAME_SIZE
+ cfi_adjust_cfa_offset(-SCV_FRAME_SIZE)
RET_SCV
b 1f
#endif
--
2.31.1
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2] powerpc64[le]: Allocate extra stack frame on syscall.S
2021-12-07 18:52 [PATCH] powerpc64[le]: Allocate extra stack frame on syscall.S Matheus Castanho
@ 2021-12-15 14:39 ` Matheus Castanho
2021-12-15 17:18 ` Paul E Murphy
2021-12-16 17:57 ` Raphael M Zinsly
0 siblings, 2 replies; 7+ messages in thread
From: Matheus Castanho @ 2021-12-15 14:39 UTC (permalink / raw)
To: libc-alpha; +Cc: murphyp, tuliom, rzinsly
v1 was not working properly on hosts without scv support, so some tests were
failing in such systems. This is fixed now in this new version.
--8<--
The syscall function does not allocate the extra stack frame for scv like other
assembly syscalls using DO_CALL_SCV. So after commit d120fb9941 changed the
offset that is used to save LR, syscall ended up using an invalid offset,
causing regressions on powerpc64. So make sure the extra stack frame is
allocated in syscall.S as well to make it consistent with other uses of
DO_CALL_SCV and avoid similar issues in the future.
Tested on powerpc, powerpc64, and powerpc64le (with and without scv)
---
sysdeps/unix/sysv/linux/powerpc/syscall.S | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/sysdeps/unix/sysv/linux/powerpc/syscall.S b/sysdeps/unix/sysv/linux/powerpc/syscall.S
index a29652feaf..a5497c8370 100644
--- a/sysdeps/unix/sysv/linux/powerpc/syscall.S
+++ b/sysdeps/unix/sysv/linux/powerpc/syscall.S
@@ -27,7 +27,11 @@ ENTRY (syscall)
mr r8,r9
#if defined(USE_PPC_SCV) && !IS_IN(rtld) && (defined(__PPC64__) || defined(__powerpc64__))
CHECK_SCV_SUPPORT r9 0f
+ stdu r1,-SCV_FRAME_SIZE(r1)
+ cfi_adjust_cfa_offset(SCV_FRAME_SIZE)
DO_CALL_SCV
+ addi r1,r1,SCV_FRAME_SIZE
+ cfi_adjust_cfa_offset(-SCV_FRAME_SIZE)
RET_SCV
b 1f
#endif
--
2.31.1
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] powerpc64[le]: Allocate extra stack frame on syscall.S
2021-12-15 14:39 ` [PATCH v2] " Matheus Castanho
@ 2021-12-15 17:18 ` Paul E Murphy
2021-12-16 16:52 ` Matheus Castanho
2021-12-16 17:57 ` Raphael M Zinsly
1 sibling, 1 reply; 7+ messages in thread
From: Paul E Murphy @ 2021-12-15 17:18 UTC (permalink / raw)
To: Matheus Castanho, libc-alpha; +Cc: tuliom, rzinsly
On 12/15/21 8:39 AM, Matheus Castanho wrote:
> v1 was not working properly on hosts without scv support, so some tests were
> failing in such systems. This is fixed now in this new version.
>
> --8<--
>
> The syscall function does not allocate the extra stack frame for scv like other
> assembly syscalls using DO_CALL_SCV. So after commit d120fb9941 changed the
> offset that is used to save LR, syscall ended up using an invalid offset,
> causing regressions on powerpc64. So make sure the extra stack frame is
> allocated in syscall.S as well to make it consistent with other uses of
> DO_CALL_SCV and avoid similar issues in the future.
>
> Tested on powerpc, powerpc64, and powerpc64le (with and without scv)
> ---
> sysdeps/unix/sysv/linux/powerpc/syscall.S | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/sysdeps/unix/sysv/linux/powerpc/syscall.S b/sysdeps/unix/sysv/linux/powerpc/syscall.S
> index a29652feaf..a5497c8370 100644
> --- a/sysdeps/unix/sysv/linux/powerpc/syscall.S
> +++ b/sysdeps/unix/sysv/linux/powerpc/syscall.S
> @@ -27,7 +27,11 @@ ENTRY (syscall)
> mr r8,r9
> #if defined(USE_PPC_SCV) && !IS_IN(rtld) && (defined(__PPC64__) || defined(__powerpc64__))
> CHECK_SCV_SUPPORT r9 0f
> + stdu r1,-SCV_FRAME_SIZE(r1)
> + cfi_adjust_cfa_offset(SCV_FRAME_SIZE)
I think this fixes the issue, but it seems like a workaround of a
deficiency in the DO_CALL_SCV macro. Should DO_CALL_SCV take a parameter
with current frame size? It would avoid the need to push a dummy frame here.
> DO_CALL_SCV > + addi r1,r1,SCV_FRAME_SIZE
> + cfi_adjust_cfa_offset(-SCV_FRAME_SIZE)
> RET_SCV
> b 1f
> #endif
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] powerpc64[le]: Allocate extra stack frame on syscall.S
2021-12-15 17:18 ` Paul E Murphy
@ 2021-12-16 16:52 ` Matheus Castanho
2021-12-17 17:08 ` Paul E Murphy
0 siblings, 1 reply; 7+ messages in thread
From: Matheus Castanho @ 2021-12-16 16:52 UTC (permalink / raw)
To: Paul E Murphy; +Cc: libc-alpha, tuliom, rzinsly
Paul E Murphy <murphyp@linux.ibm.com> writes:
> On 12/15/21 8:39 AM, Matheus Castanho wrote:
>> v1 was not working properly on hosts without scv support, so some tests were
>> failing in such systems. This is fixed now in this new version.
>> --8<--
>> The syscall function does not allocate the extra stack frame for scv like
>> other
>> assembly syscalls using DO_CALL_SCV. So after commit d120fb9941 changed the
>> offset that is used to save LR, syscall ended up using an invalid offset,
>> causing regressions on powerpc64. So make sure the extra stack frame is
>> allocated in syscall.S as well to make it consistent with other uses of
>> DO_CALL_SCV and avoid similar issues in the future.
>> Tested on powerpc, powerpc64, and powerpc64le (with and without scv)
>> ---
>> sysdeps/unix/sysv/linux/powerpc/syscall.S | 4 ++++
>> 1 file changed, 4 insertions(+)
>> diff --git a/sysdeps/unix/sysv/linux/powerpc/syscall.S
>> b/sysdeps/unix/sysv/linux/powerpc/syscall.S
>> index a29652feaf..a5497c8370 100644
>> --- a/sysdeps/unix/sysv/linux/powerpc/syscall.S
>> +++ b/sysdeps/unix/sysv/linux/powerpc/syscall.S
>> @@ -27,7 +27,11 @@ ENTRY (syscall)
>> mr r8,r9
>> #if defined(USE_PPC_SCV) && !IS_IN(rtld) && (defined(__PPC64__) || defined(__powerpc64__))
>> CHECK_SCV_SUPPORT r9 0f
>> + stdu r1,-SCV_FRAME_SIZE(r1)
>> + cfi_adjust_cfa_offset(SCV_FRAME_SIZE)
>
> I think this fixes the issue, but it seems like a workaround of a deficiency in
> the DO_CALL_SCV macro. Should DO_CALL_SCV take a parameter with current frame
> size? It would avoid the need to push a dummy frame here.
>
That is an option. But I opted to allocate the dummy frame to make it
consistent other uses of DO_CALL_SCV. I believe this will be easier to
maintain, as there will be one less exceptional way to handle scv to
worry about.
And in the end, the benefit of not allocating the dummy frame is
probably negligible, as the overall syscall latency will be dominated by
the context switch + kernel-side handling.
--
Matheus Castanho
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] powerpc64[le]: Allocate extra stack frame on syscall.S
2021-12-15 14:39 ` [PATCH v2] " Matheus Castanho
2021-12-15 17:18 ` Paul E Murphy
@ 2021-12-16 17:57 ` Raphael M Zinsly
1 sibling, 0 replies; 7+ messages in thread
From: Raphael M Zinsly @ 2021-12-16 17:57 UTC (permalink / raw)
To: Matheus Castanho, libc-alpha; +Cc: murphyp, tuliom
LGTM
Reviewed-by: Raphael M Zinsly <rzinsly@linux.ibm.com>
On 15/12/2021 11:39, Matheus Castanho wrote:
> v1 was not working properly on hosts without scv support, so some tests were
> failing in such systems. This is fixed now in this new version.
>
> --8<--
>
> The syscall function does not allocate the extra stack frame for scv like other
> assembly syscalls using DO_CALL_SCV. So after commit d120fb9941 changed the
> offset that is used to save LR, syscall ended up using an invalid offset,
> causing regressions on powerpc64. So make sure the extra stack frame is
> allocated in syscall.S as well to make it consistent with other uses of
> DO_CALL_SCV and avoid similar issues in the future.
>
> Tested on powerpc, powerpc64, and powerpc64le (with and without scv)
> ---
> sysdeps/unix/sysv/linux/powerpc/syscall.S | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/sysdeps/unix/sysv/linux/powerpc/syscall.S b/sysdeps/unix/sysv/linux/powerpc/syscall.S
> index a29652feaf..a5497c8370 100644
> --- a/sysdeps/unix/sysv/linux/powerpc/syscall.S
> +++ b/sysdeps/unix/sysv/linux/powerpc/syscall.S
> @@ -27,7 +27,11 @@ ENTRY (syscall)
> mr r8,r9
> #if defined(USE_PPC_SCV) && !IS_IN(rtld) && (defined(__PPC64__) || defined(__powerpc64__))
> CHECK_SCV_SUPPORT r9 0f
> + stdu r1,-SCV_FRAME_SIZE(r1)
> + cfi_adjust_cfa_offset(SCV_FRAME_SIZE)
> DO_CALL_SCV
> + addi r1,r1,SCV_FRAME_SIZE
> + cfi_adjust_cfa_offset(-SCV_FRAME_SIZE)
> RET_SCV
> b 1f
> #endif
>
--
Raphael Moreira Zinsly
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] powerpc64[le]: Allocate extra stack frame on syscall.S
2021-12-16 16:52 ` Matheus Castanho
@ 2021-12-17 17:08 ` Paul E Murphy
2021-12-17 18:54 ` Matheus Castanho
0 siblings, 1 reply; 7+ messages in thread
From: Paul E Murphy @ 2021-12-17 17:08 UTC (permalink / raw)
To: Matheus Castanho; +Cc: libc-alpha, tuliom, rzinsly
On 12/16/21 10:52 AM, Matheus Castanho wrote:
>
> Paul E Murphy <murphyp@linux.ibm.com> writes:
>
>> On 12/15/21 8:39 AM, Matheus Castanho wrote:
>>> v1 was not working properly on hosts without scv support, so some tests were
>>> failing in such systems. This is fixed now in this new version.
>>> --8<--
>>> The syscall function does not allocate the extra stack frame for scv like
>>> other
>>> assembly syscalls using DO_CALL_SCV. So after commit d120fb9941 changed the
>>> offset that is used to save LR, syscall ended up using an invalid offset,
>>> causing regressions on powerpc64. So make sure the extra stack frame is
>>> allocated in syscall.S as well to make it consistent with other uses of
>>> DO_CALL_SCV and avoid similar issues in the future.
>>> Tested on powerpc, powerpc64, and powerpc64le (with and without scv)
>>> ---
>>> sysdeps/unix/sysv/linux/powerpc/syscall.S | 4 ++++
>>> 1 file changed, 4 insertions(+)
>>> diff --git a/sysdeps/unix/sysv/linux/powerpc/syscall.S
>>> b/sysdeps/unix/sysv/linux/powerpc/syscall.S
>>> index a29652feaf..a5497c8370 100644
>>> --- a/sysdeps/unix/sysv/linux/powerpc/syscall.S
>>> +++ b/sysdeps/unix/sysv/linux/powerpc/syscall.S
>>> @@ -27,7 +27,11 @@ ENTRY (syscall)
>>> mr r8,r9
>>> #if defined(USE_PPC_SCV) && !IS_IN(rtld) && (defined(__PPC64__) || defined(__powerpc64__))
>>> CHECK_SCV_SUPPORT r9 0f
>>> + stdu r1,-SCV_FRAME_SIZE(r1)
>>> + cfi_adjust_cfa_offset(SCV_FRAME_SIZE)
>>
>> I think this fixes the issue, but it seems like a workaround of a deficiency in
>> the DO_CALL_SCV macro. Should DO_CALL_SCV take a parameter with current frame
>> size? It would avoid the need to push a dummy frame here.
>>
>
> That is an option. But I opted to allocate the dummy frame to make it
> consistent other uses of DO_CALL_SCV. I believe this will be easier to
> maintain, as there will be one less exceptional way to handle scv to
> worry about.
>
> And in the end, the benefit of not allocating the dummy frame is
> probably negligible, as the overall syscall latency will be dominated by
> the context switch + kernel-side handling.
OK. I agree with that. LGTM.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] powerpc64[le]: Allocate extra stack frame on syscall.S
2021-12-17 17:08 ` Paul E Murphy
@ 2021-12-17 18:54 ` Matheus Castanho
0 siblings, 0 replies; 7+ messages in thread
From: Matheus Castanho @ 2021-12-17 18:54 UTC (permalink / raw)
To: Paul E Murphy; +Cc: libc-alpha, tuliom, rzinsly
Paul E Murphy <murphyp@linux.ibm.com> writes:
> On 12/16/21 10:52 AM, Matheus Castanho wrote:
>> Paul E Murphy <murphyp@linux.ibm.com> writes:
>>
>>> On 12/15/21 8:39 AM, Matheus Castanho wrote:
>>>> v1 was not working properly on hosts without scv support, so some tests were
>>>> failing in such systems. This is fixed now in this new version.
>>>> --8<--
>>>> The syscall function does not allocate the extra stack frame for scv like
>>>> other
>>>> assembly syscalls using DO_CALL_SCV. So after commit d120fb9941 changed the
>>>> offset that is used to save LR, syscall ended up using an invalid offset,
>>>> causing regressions on powerpc64. So make sure the extra stack frame is
>>>> allocated in syscall.S as well to make it consistent with other uses of
>>>> DO_CALL_SCV and avoid similar issues in the future.
>>>> Tested on powerpc, powerpc64, and powerpc64le (with and without scv)
>>>> ---
>>>> sysdeps/unix/sysv/linux/powerpc/syscall.S | 4 ++++
>>>> 1 file changed, 4 insertions(+)
>>>> diff --git a/sysdeps/unix/sysv/linux/powerpc/syscall.S
>>>> b/sysdeps/unix/sysv/linux/powerpc/syscall.S
>>>> index a29652feaf..a5497c8370 100644
>>>> --- a/sysdeps/unix/sysv/linux/powerpc/syscall.S
>>>> +++ b/sysdeps/unix/sysv/linux/powerpc/syscall.S
>>>> @@ -27,7 +27,11 @@ ENTRY (syscall)
>>>> mr r8,r9
>>>> #if defined(USE_PPC_SCV) && !IS_IN(rtld) && (defined(__PPC64__) || defined(__powerpc64__))
>>>> CHECK_SCV_SUPPORT r9 0f
>>>> + stdu r1,-SCV_FRAME_SIZE(r1)
>>>> + cfi_adjust_cfa_offset(SCV_FRAME_SIZE)
>>>
>>> I think this fixes the issue, but it seems like a workaround of a deficiency in
>>> the DO_CALL_SCV macro. Should DO_CALL_SCV take a parameter with current frame
>>> size? It would avoid the need to push a dummy frame here.
>>>
>> That is an option. But I opted to allocate the dummy frame to make it
>> consistent other uses of DO_CALL_SCV. I believe this will be easier to
>> maintain, as there will be one less exceptional way to handle scv to
>> worry about.
>> And in the end, the benefit of not allocating the dummy frame is
>> probably negligible, as the overall syscall latency will be dominated by
>> the context switch + kernel-side handling.
>
> OK. I agree with that. LGTM.
Pushed as ae91d3df24a4a1b1f264d101a71a298bff310d14
Thanks,
Matheus Castanho
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-12-17 18:55 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-07 18:52 [PATCH] powerpc64[le]: Allocate extra stack frame on syscall.S Matheus Castanho
2021-12-15 14:39 ` [PATCH v2] " Matheus Castanho
2021-12-15 17:18 ` Paul E Murphy
2021-12-16 16:52 ` Matheus Castanho
2021-12-17 17:08 ` Paul E Murphy
2021-12-17 18:54 ` Matheus Castanho
2021-12-16 17:57 ` Raphael M Zinsly
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).