public inbox for libc-alpha@sourceware.org
 help / color / mirror / Atom feed
* [Patch V2] mips:  minior fix
@ 2020-11-14  6:41 Huang Pei
  2020-11-14  6:41 ` [PATCH 1/3] mips: add hp-timing support for MIPS R2 Huang Pei
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Huang Pei @ 2020-11-14  6:41 UTC (permalink / raw)
  To: Joseph Myers; +Cc: libc-alpha, Huacai Chen, Chenghua Xu

All three patched is tested on debian glibc-2.28, no regresssion
found.

here is the output of 'LD_DEBUG=statistics data' with Patch 1:

.....................................................................
depaulose@Board-3A3000:glibc-2.28$ LD_DEBUG=statistics date
	441:
	441:     runtime linker statistics:
	441:       total startup time in dynamic loader: 297131 cycles
	441:       time needed for relocation: 150697 cycles (50.7%)
	441:       number of relocations: 101
	441:       number of relocations from cache: 0
	441:       number of relative relocations: 0
	441:       time needed to load objects: 84028 cycles (28.2%)
Sat 14 Nov 2020 06:33:18 AM UTC
.....................................................................

Patch 2 make the sp follow N32/N64 ABI, however the root cause is the
reg s0 spill, which is unnecessary



^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/3] mips: add hp-timing support for MIPS R2
  2020-11-14  6:41 [Patch V2] mips: minior fix Huang Pei
@ 2020-11-14  6:41 ` Huang Pei
  2020-11-14  6:41 ` [PATCH 2/3] mips: make sp 16-byte aligned on N64/N32 Huang Pei
  2020-11-14  6:42 ` [PATCH 3/3] mips: remove useless register spill Huang Pei
  2 siblings, 0 replies; 8+ messages in thread
From: Huang Pei @ 2020-11-14  6:41 UTC (permalink / raw)
  To: Joseph Myers; +Cc: libc-alpha, Huacai Chen, Chenghua Xu

MIPS R2 only support 32 bit TSC(AKA "rdhwr %0, $2"), but it should be
enough for glibc

Signed-off-by: Huang Pei <huangpei@loongson.cn>
---
 sysdeps/mips/hp-timing.h | 43 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)
 create mode 100644 sysdeps/mips/hp-timing.h

diff --git a/sysdeps/mips/hp-timing.h b/sysdeps/mips/hp-timing.h
new file mode 100644
index 0000000000..128315d0bf
--- /dev/null
+++ b/sysdeps/mips/hp-timing.h
@@ -0,0 +1,43 @@
+/* High precision, low overhead timing functions. MIPS version.
+   Copyright (C) 2002-2020 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#ifndef _HP_TIMING_H
+#define _HP_TIMING_H	1
+
+#if __mips_isa_rev >= 2
+/* We always assume having the timestamp register.  */
+#define HP_TIMING_AVAIL		(0)
+#define HP_SMALL_TIMING_AVAIL	(1)
+
+/* We indeed have inlined functions.  */
+#define HP_TIMING_INLINE	(1)
+
+/* We use 32bit values for the times.  */
+typedef unsigned int hp_timing_t;
+
+/* Read the cp0 count, this maybe inaccurate.  */
+#define HP_TIMING_NOW(Var) \
+  ({ unsigned int _count; \
+     asm volatile ("rdhwr\t%0,$2" : "=r" (_count)); \
+     (Var) = _count; })
+
+#include <hp-timing-common.h>
+
+#endif
+
+#endif /* hp-timing.h */
-- 
2.17.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 2/3] mips: make sp 16-byte aligned on N64/N32
  2020-11-14  6:41 [Patch V2] mips: minior fix Huang Pei
  2020-11-14  6:41 ` [PATCH 1/3] mips: add hp-timing support for MIPS R2 Huang Pei
@ 2020-11-14  6:41 ` Huang Pei
  2020-11-14  6:42 ` [PATCH 3/3] mips: remove useless register spill Huang Pei
  2 siblings, 0 replies; 8+ messages in thread
From: Huang Pei @ 2020-11-14  6:41 UTC (permalink / raw)
  To: Joseph Myers; +Cc: libc-alpha, Huacai Chen, Chenghua Xu

MIPS N64/N32 ABI request stack pointer be 16-byte alinged

Signed-off-by: Huang Pei <huangpei@loongson.cn>
---
 sysdeps/unix/sysv/linux/mips/mips64/syscall.S | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/sysdeps/unix/sysv/linux/mips/mips64/syscall.S b/sysdeps/unix/sysv/linux/mips/mips64/syscall.S
index a9baff3c17..aab1f389aa 100644
--- a/sysdeps/unix/sysv/linux/mips/mips64/syscall.S
+++ b/sysdeps/unix/sysv/linux/mips/mips64/syscall.S
@@ -27,10 +27,10 @@
 
 	.text
 NESTED (syscall, SZREG, ra)
-	.mask 0x00010000, -SZREG
+	.mask 0x00010000, -2 * SZREG
 	.fmask 0x00000000, 0
-	PTR_ADDIU sp, -SZREG
-	cfi_adjust_cfa_offset (SZREG)
+	PTR_ADDIU sp, -2 * SZREG
+	cfi_adjust_cfa_offset (2 * SZREG)
 	REG_S s0, (sp)
 	cfi_rel_offset (s0, 0)
 
@@ -48,8 +48,8 @@ NESTED (syscall, SZREG, ra)
 
 	REG_L s0, (sp)
 	cfi_restore (s0)
-	PTR_ADDIU sp, SZREG
-	cfi_adjust_cfa_offset (-SZREG)
+	PTR_ADDIU sp, 2 * SZREG
+	cfi_adjust_cfa_offset (-2 * 2 * SZREG)
 	bne a3, zero, L(error)
 
 	ret
-- 
2.17.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 3/3] mips: remove useless register spill
  2020-11-14  6:41 [Patch V2] mips: minior fix Huang Pei
  2020-11-14  6:41 ` [PATCH 1/3] mips: add hp-timing support for MIPS R2 Huang Pei
  2020-11-14  6:41 ` [PATCH 2/3] mips: make sp 16-byte aligned on N64/N32 Huang Pei
@ 2020-11-14  6:42 ` Huang Pei
  2020-11-15  1:22   ` Maciej W. Rozycki
  2 siblings, 1 reply; 8+ messages in thread
From: Huang Pei @ 2020-11-14  6:42 UTC (permalink / raw)
  To: Joseph Myers; +Cc: libc-alpha, Huacai Chen, Chenghua Xu

Signed-off-by: Huang Pei <huangpei@loongson.cn>
---
 sysdeps/unix/sysv/linux/mips/mips64/syscall.S | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/sysdeps/unix/sysv/linux/mips/mips64/syscall.S b/sysdeps/unix/sysv/linux/mips/mips64/syscall.S
index aab1f389aa..089524a40b 100644
--- a/sysdeps/unix/sysv/linux/mips/mips64/syscall.S
+++ b/sysdeps/unix/sysv/linux/mips/mips64/syscall.S
@@ -27,14 +27,9 @@
 
 	.text
 NESTED (syscall, SZREG, ra)
-	.mask 0x00010000, -2 * SZREG
+	.mask 0x00000000, 0
 	.fmask 0x00000000, 0
-	PTR_ADDIU sp, -2 * SZREG
-	cfi_adjust_cfa_offset (2 * SZREG)
-	REG_S s0, (sp)
-	cfi_rel_offset (s0, 0)
-
-	move s0, a0
+	move v0, a0
 	move a0, a1		/* shift arg1 - arg7.  */
 	move a1, a2
 	move a2, a3
@@ -43,13 +38,8 @@ NESTED (syscall, SZREG, ra)
 	move a5, a6
 	move a6, a7
 
-	move v0, s0		/* Syscall number -> v0 */
 	syscall			/* Do the system call.  */
 
-	REG_L s0, (sp)
-	cfi_restore (s0)
-	PTR_ADDIU sp, 2 * SZREG
-	cfi_adjust_cfa_offset (-2 * 2 * SZREG)
 	bne a3, zero, L(error)
 
 	ret
-- 
2.17.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 3/3] mips: remove useless register spill
  2020-11-14  6:42 ` [PATCH 3/3] mips: remove useless register spill Huang Pei
@ 2020-11-15  1:22   ` Maciej W. Rozycki
       [not found]     ` <2020111522504369692527@loongson.cn>
  0 siblings, 1 reply; 8+ messages in thread
From: Maciej W. Rozycki @ 2020-11-15  1:22 UTC (permalink / raw)
  To: Huang Pei; +Cc: Joseph Myers, Huacai Chen, Chenghua Xu, libc-alpha

On Sat, 14 Nov 2020, Huang Pei wrote:

> Signed-off-by: Huang Pei <huangpei@loongson.cn>

 Please be a bit more verbose in the change description.  This used to be 
an implementation of the old syscall restart convention, where $v0 had to 
be (re)loaded by the instruction immediately preceding SYSCALL, obviously
from a source that does not get clobbered in syscall processing (i.e. 
either memory or a static register).  I imagine a good change description 
would say at least actually why this convention is no longer relevant.

 Refer to the comment in sysdeps/unix/sysv/linux/mips/mips32/sysdep.h or
sysdeps/unix/sysv/linux/mips/mips64/sysdep.h for further details.

 Also I am not sure if it makes sense to remove this single instance only, 
possibly for the least used syscall in user code, and leave all the other 
places unchanged.

  Maciej

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Re: [PATCH 3/3] mips: remove useless register spill
       [not found]     ` <2020111522504369692527@loongson.cn>
@ 2020-11-15 14:56       ` huangpei
  2020-12-04 11:24         ` Maciej W. Rozycki
  0 siblings, 1 reply; 8+ messages in thread
From: huangpei @ 2020-11-15 14:56 UTC (permalink / raw)
  To: Maciej W. Rozycki; +Cc: Myers, Chen, Xu, libc-alpha

Hi, Maciej,

I must admit that I did not realize that Patch 3  would cause compatibiliy
issue, that is Linux/MIPS kernel before 2.6.36 depended on
"move a0, s0 ' just preceding "syscall" to make SYSCALL Restart
work, while  Linux/MIPS after 2.6.36 do not

since glibc 2.24,the minimum kernel version is 3.2, is it ok to add Patch
3 for glibc after version 2.24? If so, I would send V3, which merge Patch
2 and Patch 3 and rewrite the change description with minimium kernel
version requirement.

Huang Pei

>Hi, Maciej,
>
>I must admit that I did not realize that Patch 3  would cause compatibiliy
>issue, that is Linux/MIPS kernel before 2.6.36 depended on
>"move a0, s0 ' just preceding "syscall" to make SYSCALL Restart
>work, while  Linux/MIPS after 2.6.36 do not
>
>since glibc 2.24,the minimum kernel version is 3.2, is it ok to add Patch
>3 for glibc after version 2.24? If so, I would send V3, which merge Patch
>2 and Patch 3 and rewrite the change description with minimium kernel
>version requirement.
>
>Huang Pei
>
>
>--------------
>huangpei@loongson.cn
>>On Sat, 14 Nov 2020, Huang Pei wrote:
>>
>>> Signed-off-by: Huang Pei <huangpei@loongson.cn>
>>
>> Please be a bit more verbose in the change description.  This used to be
>>an implementation of the old syscall restart convention, where $v0 had to
>>be (re)loaded by the instruction immediately preceding SYSCALL, obviously
>>from a source that does not get clobbered in syscall processing (i.e.
>>either memory or a static register).  I imagine a good change description
>>would say at least actually why this convention is no longer relevant.
>>
>> Refer to the comment in sysdeps/unix/sysv/linux/mips/mips32/sysdep.h or
>>sysdeps/unix/sysv/linux/mips/mips64/sysdep.h for further details.
>>
>> Also I am not sure if it makes sense to remove this single instance only,
>>possibly for the least used syscall in user code, and leave all the other
>>places unchanged.
>>
>>  Maciej

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Re: [PATCH 3/3] mips: remove useless register spill
  2020-11-15 14:56       ` huangpei
@ 2020-12-04 11:24         ` Maciej W. Rozycki
  2020-12-05  5:47           ` Huang Pei
  0 siblings, 1 reply; 8+ messages in thread
From: Maciej W. Rozycki @ 2020-12-04 11:24 UTC (permalink / raw)
  To: huangpei; +Cc: Myers, Chen, Xu, libc-alpha

Hi Huang Pei,

 Apologies for my late reply, I've had a hectic time recently and some GCC 
stuff had to take priority due to the end of Stage 1 there.

> I must admit that I did not realize that Patch 3  would cause compatibiliy
> issue, that is Linux/MIPS kernel before 2.6.36 depended on
> "move a0, s0 ' just preceding "syscall" to make SYSCALL Restart
> work, while  Linux/MIPS after 2.6.36 do not

 Or `move v0, s0' (in this case) to be correct.

 Yes, this has been quite obscure and used to be hardly documented up to 
the point that breakage happened while the old scheme was still supported.  
And given that syscall restarts are not at all that frequent any symptom 
would be limited to an occasional program crash or other odd behaviour as 
the wrong syscall was dispatched upon a restart.

 This is why I added the explanation along with fixes for BZ #15054 made 
with commit b82ba2f011fc ("MIPS: Respect the legacy syscall restart 
convention.").  Before that we only had Linux sources as a reference.

 NB the old scheme was removed from Linux with commit 8f5a00eb422e ("MIPS: 
Sanitize restart logics").

> since glibc 2.24,the minimum kernel version is 3.2, is it ok to add Patch
> 3 for glibc after version 2.24? If so, I would send V3, which merge Patch
> 2 and Patch 3 and rewrite the change description with minimium kernel
> version requirement.

 As I noted elsewhere I object such a change if made on its own, as it 
would only cause a mess.

 You can review my commit referred above, and also commit 43301bd3c281 
("Add support for building as MIPS16 code.") to see what has to be done to 
remove bits for the old feature in a consistent manner.

  Maciej

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 3/3] mips: remove useless register spill
  2020-12-04 11:24         ` Maciej W. Rozycki
@ 2020-12-05  5:47           ` Huang Pei
  0 siblings, 0 replies; 8+ messages in thread
From: Huang Pei @ 2020-12-05  5:47 UTC (permalink / raw)
  To: Maciej W. Rozycki; +Cc: Myers, Chen, Xu, libc-alpha

On Fri, Dec 04, 2020 at 11:24:24AM +0000, Maciej W. Rozycki wrote:
> Hi Huang Pei,
> 
>  Apologies for my late reply, I've had a hectic time recently and some GCC 
> stuff had to take priority due to the end of Stage 1 there.
> 
> > I must admit that I did not realize that Patch 3  would cause compatibiliy
> > issue, that is Linux/MIPS kernel before 2.6.36 depended on
> > "move a0, s0 ' just preceding "syscall" to make SYSCALL Restart
> > work, while  Linux/MIPS after 2.6.36 do not
> 
>  Or `move v0, s0' (in this case) to be correct.
Yes
> 
>  Yes, this has been quite obscure and used to be hardly documented up to 
> the point that breakage happened while the old scheme was still supported.  
> And given that syscall restarts are not at all that frequent any symptom 
> would be limited to an occasional program crash or other odd behaviour as 
> the wrong syscall was dispatched upon a restart.
> 
>  This is why I added the explanation along with fixes for BZ #15054 made 
> with commit b82ba2f011fc ("MIPS: Respect the legacy syscall restart 
> convention.").  Before that we only had Linux sources as a reference.
> 
>  NB the old scheme was removed from Linux with commit 8f5a00eb422e ("MIPS: 
> Sanitize restart logics").
> 
> > since glibc 2.24,the minimum kernel version is 3.2, is it ok to add Patch
> > 3 for glibc after version 2.24? If so, I would send V3, which merge Patch
> > 2 and Patch 3 and rewrite the change description with minimium kernel
> > version requirement.
> 
>  As I noted elsewhere I object such a change if made on its own, as it 
> would only cause a mess.

This one was still a mess, since the ".set noreorder" is within
b82ba2f011fc, but missing here(I hate '.set noreorder', which bite me
too much)

I came to this one accidentally when I addressed a unaligned access issue, 
but the fix is not the key point(see Patch V1 2/3), and Patch V1 3/3
without touching ".set noreorder"(I focus on) is.

PS:

I think it would be mutch better without both ".set reorder" and ".set 
noreorder", and with 8f5a00eb422e, we can remove more ".set noreorder"


> 
>  You can review my commit referred above, and also commit 43301bd3c281 
> ("Add support for building as MIPS16 code.") to see what has to be done to 
> remove bits for the old feature in a consistent manner.
> 
>   Maciej

Now it is OK to remove the mess as a whole, I will send V3 to get rid of it
all


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2020-12-05  5:47 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-14  6:41 [Patch V2] mips: minior fix Huang Pei
2020-11-14  6:41 ` [PATCH 1/3] mips: add hp-timing support for MIPS R2 Huang Pei
2020-11-14  6:41 ` [PATCH 2/3] mips: make sp 16-byte aligned on N64/N32 Huang Pei
2020-11-14  6:42 ` [PATCH 3/3] mips: remove useless register spill Huang Pei
2020-11-15  1:22   ` Maciej W. Rozycki
     [not found]     ` <2020111522504369692527@loongson.cn>
2020-11-15 14:56       ` huangpei
2020-12-04 11:24         ` Maciej W. Rozycki
2020-12-05  5:47           ` Huang Pei

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).