* [PATCH v2 0/4] Move libio lock single-thread optimization to generic libc-lock
@ 2022-04-26 19:15 Adhemerval Zanella
2022-04-26 19:15 ` [PATCH v2 1/4] libio: Assume _IO_MTSAFE_IO Adhemerval Zanella
` (3 more replies)
0 siblings, 4 replies; 23+ messages in thread
From: Adhemerval Zanella @ 2022-04-26 19:15 UTC (permalink / raw)
To: libc-alpha, Florian Weimer
This patchset removes some old unused code (_IO_lock_inexpensive
and _IO_MTSAFE_IO), consolidate the stdio-lock.h and move the
single-thread lock optimization to generic code instead of
per-function usage.
Adhemerval Zanella (4):
libio: Assume _IO_MTSAFE_IO
Consolidate stdio-lock.h
Move libio lock single-thread optimization to generic libc-lock (BZ
#27842)
Assume _LIBC and libc module for libc-lock.h
Makeconfig | 2 -
debug/Makefile | 43 +++++--------
grp/Makefile | 4 +-
gshadow/Makefile | 4 +-
htl/Makefile | 2 -
hurd/vpprintf.c | 4 --
include/stdio.h | 2 +-
libio/Makefile | 2 -
libio/Versions | 3 -
libio/clearerr.c | 4 --
libio/feof.c | 7 ---
libio/ferror.c | 6 --
libio/fileno.c | 2 +-
libio/fputc.c | 7 ---
libio/genops.c | 66 -------------------
libio/getc.c | 8 ---
libio/getchar.c | 7 ---
libio/iofdopen.c | 4 --
libio/iofflush.c | 7 ---
libio/iofgets.c | 7 ---
libio/iofopen.c | 4 --
libio/iofopncook.c | 6 --
libio/iofputs.c | 7 ---
libio/iofread.c | 6 --
libio/iofwrite.c | 6 +-
libio/iopopen.c | 14 -----
libio/ioungetc.c | 2 -
libio/iovdprintf.c | 2 -
libio/iovsprintf.c | 2 -
libio/libio.h | 30 +++------
libio/libioP.h | 30 ++-------
libio/memstream.c | 7 ---
libio/obprintf.c | 2 -
libio/oldiofdopen.c | 4 --
libio/oldiofopen.c | 4 --
libio/oldiopopen.c | 14 -----
libio/oldstdfiles.c | 6 --
libio/putc.c | 7 ---
libio/putchar.c | 5 --
libio/stdfiles.c | 11 +---
libio/vasprintf.c | 2 -
libio/vsnprintf.c | 2 -
libio/vswprintf.c | 2 -
libio/wgenops.c | 2 -
libio/wmemstream.c | 7 ---
nptl/Makefile | 3 -
nptl/pthread_create.c | 3 -
pwd/Makefile | 1 -
shadow/Makefile | 4 +-
stdio-common/Makefile | 2 -
stdio-common/flockfile.c | 1 -
stdio-common/vfprintf-internal.c | 4 --
stdlib/Makefile | 9 ---
stdlib/strfmon_l.c | 2 -
stdlib/strfrom-skeleton.c | 2 -
sysdeps/generic/stdio-lock.h | 25 +++-----
sysdeps/htl/stdio-lock.h | 57 -----------------
sysdeps/ieee754/float128/Makefile | 4 --
sysdeps/mach/libc-lock.h | 18 ++----
sysdeps/nptl/libc-lock.h | 74 ++++------------------
sysdeps/nptl/stdio-lock.h | 101 ------------------------------
sysdeps/unix/sysv/linux/Makefile | 1 -
wcsmbs/Makefile | 2 -
63 files changed, 65 insertions(+), 623 deletions(-)
delete mode 100644 sysdeps/htl/stdio-lock.h
delete mode 100644 sysdeps/nptl/stdio-lock.h
--
2.34.1
^ permalink raw reply [flat|nested] 23+ messages in thread* [PATCH v2 1/4] libio: Assume _IO_MTSAFE_IO 2022-04-26 19:15 [PATCH v2 0/4] Move libio lock single-thread optimization to generic libc-lock Adhemerval Zanella @ 2022-04-26 19:15 ` Adhemerval Zanella 2022-04-27 12:34 ` Florian Weimer 2022-04-26 19:15 ` [PATCH v2 2/4] Consolidate stdio-lock.h Adhemerval Zanella ` (2 subsequent siblings) 3 siblings, 1 reply; 23+ messages in thread From: Adhemerval Zanella @ 2022-04-26 19:15 UTC (permalink / raw) To: libc-alpha, Florian Weimer It is already set by default on all supported architectures and it is an expectation that stdio works on multi-threaded environments. --- Makeconfig | 2 -- debug/Makefile | 43 ++++++++++++------------------- grp/Makefile | 4 +-- gshadow/Makefile | 4 +-- htl/Makefile | 2 -- hurd/vpprintf.c | 4 --- include/stdio.h | 2 +- libio/Makefile | 2 -- libio/clearerr.c | 4 --- libio/feof.c | 5 ---- libio/ferror.c | 4 --- libio/fileno.c | 2 +- libio/fputc.c | 5 ---- libio/genops.c | 38 --------------------------- libio/getc.c | 6 ----- libio/getchar.c | 7 +---- libio/iofdopen.c | 4 --- libio/iofflush.c | 7 ----- libio/iofgets.c | 7 ----- libio/iofopen.c | 4 --- libio/iofopncook.c | 4 --- libio/iofputs.c | 7 ----- libio/iofread.c | 6 ----- libio/iofwrite.c | 6 +---- libio/iopopen.c | 14 ---------- libio/iovdprintf.c | 2 -- libio/iovsprintf.c | 2 -- libio/libio.h | 26 ++++++------------- libio/libioP.h | 30 +++------------------ libio/memstream.c | 4 --- libio/obprintf.c | 2 -- libio/oldiofdopen.c | 4 --- libio/oldiofopen.c | 4 --- libio/oldiopopen.c | 14 ---------- libio/oldstdfiles.c | 6 ----- libio/putc.c | 5 ---- libio/putchar.c | 5 ---- libio/stdfiles.c | 11 +------- libio/vasprintf.c | 2 -- libio/vsnprintf.c | 2 -- libio/vswprintf.c | 2 -- libio/wgenops.c | 2 -- libio/wmemstream.c | 4 --- nptl/Makefile | 3 --- pwd/Makefile | 1 - shadow/Makefile | 4 +-- stdio-common/Makefile | 2 -- stdio-common/vfprintf-internal.c | 4 --- stdlib/Makefile | 9 ------- stdlib/strfmon_l.c | 2 -- stdlib/strfrom-skeleton.c | 2 -- sysdeps/generic/stdio-lock.h | 4 +-- sysdeps/ieee754/float128/Makefile | 4 --- sysdeps/nptl/libc-lock.h | 2 +- sysdeps/unix/sysv/linux/Makefile | 1 - wcsmbs/Makefile | 2 -- 56 files changed, 43 insertions(+), 317 deletions(-) diff --git a/Makeconfig b/Makeconfig index 0aa5fb0099..14fd013d57 100644 --- a/Makeconfig +++ b/Makeconfig @@ -1377,8 +1377,6 @@ endif # A sysdeps Makeconfig fragment may set libc-reentrant to yes. ifeq (yes,$(libc-reentrant)) defines += -D_LIBC_REENTRANT - -libio-mtsafe = -D_IO_MTSAFE_IO endif # The name to give to a test in test results summaries. diff --git a/debug/Makefile b/debug/Makefile index 96029f32ee..6b7c5a1fcb 100644 --- a/debug/Makefile +++ b/debug/Makefile @@ -68,32 +68,23 @@ elide-routines.o := stack_chk_fail_local CFLAGS-stack_chk_fail.c += $(no-stack-protector) CFLAGS-backtrace.c += -fno-omit-frame-pointer -funwind-tables -CFLAGS-sprintf_chk.c += $(libio-mtsafe) -CFLAGS-snprintf_chk.c += $(libio-mtsafe) -CFLAGS-vsprintf_chk.c += $(libio-mtsafe) -CFLAGS-vsnprintf_chk.c += $(libio-mtsafe) -CFLAGS-asprintf_chk.c += $(libio-mtsafe) -CFLAGS-vasprintf_chk.c += $(libio-mtsafe) -CFLAGS-obprintf_chk.c += $(libio-mtsafe) -CFLAGS-dprintf_chk.c += $(libio-mtsafe) -fexceptions -CFLAGS-vdprintf_chk.c += $(libio-mtsafe) -fexceptions -CFLAGS-printf_chk.c += $(libio-mtsafe) -fexceptions -CFLAGS-fprintf_chk.c += $(libio-mtsafe) -fexceptions -CFLAGS-vprintf_chk.c += $(libio-mtsafe) -fexceptions -CFLAGS-vfprintf_chk.c += $(libio-mtsafe) -fexceptions -CFLAGS-gets_chk.c += $(libio-mtsafe) -fexceptions -CFLAGS-fgets_chk.c += $(libio-mtsafe) -fexceptions -CFLAGS-fgets_u_chk.c += $(libio-mtsafe) -fexceptions -CFLAGS-fread_chk.c += $(libio-mtsafe) -fexceptions -CFLAGS-fread_u_chk.c += $(libio-mtsafe) -fexceptions -CFLAGS-swprintf_chk.c += $(libio-mtsafe) -CFLAGS-vswprintf_chk.c += $(libio-mtsafe) -CFLAGS-wprintf_chk.c += $(libio-mtsafe) -fexceptions -CFLAGS-fwprintf_chk.c += $(libio-mtsafe) -fexceptions -CFLAGS-vwprintf_chk.c += $(libio-mtsafe) -fexceptions -CFLAGS-vfwprintf_chk.c += $(libio-mtsafe) -fexceptions -CFLAGS-fgetws_chk.c += $(libio-mtsafe) -fexceptions -CFLAGS-fgetws_u_chk.c += $(libio-mtsafe) -fexceptions +CFLAGS-dprintf_chk.c += -fexceptions +CFLAGS-vdprintf_chk.c += -fexceptions +CFLAGS-printf_chk.c += -fexceptions +CFLAGS-fprintf_chk.c += -fexceptions +CFLAGS-vprintf_chk.c += -fexceptions +CFLAGS-vfprintf_chk.c += -fexceptions +CFLAGS-gets_chk.c += -fexceptions +CFLAGS-fgets_chk.c += -fexceptions +CFLAGS-fgets_u_chk.c += -fexceptions +CFLAGS-fread_chk.c += -fexceptions +CFLAGS-fread_u_chk.c += -fexceptions +CFLAGS-wprintf_chk.c += -fexceptions +CFLAGS-fwprintf_chk.c += -fexceptions +CFLAGS-vwprintf_chk.c += -fexceptions +CFLAGS-vfwprintf_chk.c += -fexceptions +CFLAGS-fgetws_chk.c += -fexceptions +CFLAGS-fgetws_u_chk.c += -fexceptions CFLAGS-read_chk.c += -fexceptions -fasynchronous-unwind-tables CFLAGS-pread_chk.c += -fexceptions -fasynchronous-unwind-tables CFLAGS-pread64_chk.c += -fexceptions -fasynchronous-unwind-tables diff --git a/grp/Makefile b/grp/Makefile index 48f3914e29..a3dc650cde 100644 --- a/grp/Makefile +++ b/grp/Makefile @@ -52,8 +52,8 @@ CFLAGS-getgrnam_r.c += -fexceptions CFLAGS-getgrent_r.c += -fexceptions CFLAGS-getgrent.c += -fexceptions CFLAGS-fgetgrent.c += -fexceptions -CFLAGS-fgetgrent_r.c += -fexceptions $(libio-mtsafe) -CFLAGS-putgrent.c += -fexceptions $(libio-mtsafe) +CFLAGS-fgetgrent_r.c += -fexceptions +CFLAGS-putgrent.c += -fexceptions CFLAGS-initgroups.c += -fexceptions CFLAGS-getgrgid.c += -fexceptions diff --git a/gshadow/Makefile b/gshadow/Makefile index eff303f538..3f9274ba99 100644 --- a/gshadow/Makefile +++ b/gshadow/Makefile @@ -31,8 +31,8 @@ tests = tst-gshadow tst-putsgent tst-fgetsgent_r CFLAGS-getsgent_r.c += -fexceptions CFLAGS-getsgent.c += -fexceptions CFLAGS-fgetsgent.c += -fexceptions -CFLAGS-fgetsgent_r.c += -fexceptions $(libio-mtsafe) -CFLAGS-putsgent.c += -fexceptions $(libio-mtsafe) +CFLAGS-fgetsgent_r.c += -fexceptions +CFLAGS-putsgent.c += -fexceptions CFLAGS-getsgnam.c += -fexceptions CFLAGS-getsgnam_r.c += -fexceptions diff --git a/htl/Makefile b/htl/Makefile index 0b403e2fca..cdc20141bf 100644 --- a/htl/Makefile +++ b/htl/Makefile @@ -174,8 +174,6 @@ install-lib-ldscripts := libpthread_syms.a include ../Makeconfig -CFLAGS-lockfile.c = -D_IO_MTSAFE_IO - all: # Make this the default target; it will be defined in Rules. subdir_install: $(inst_libdir)/libpthread2.a $(inst_libdir)/libpthread_syms.a diff --git a/hurd/vpprintf.c b/hurd/vpprintf.c index 67450399f5..7dbd4fca85 100644 --- a/hurd/vpprintf.c +++ b/hurd/vpprintf.c @@ -42,13 +42,9 @@ vpprintf (io_t port, const char *format, va_list arg) struct locked_FILE { struct _IO_cookie_file cfile; -#ifdef _IO_MTSAFE_IO _IO_lock_t lock; -#endif } temp_f; -#ifdef _IO_MTSAFE_IO temp_f.cfile.__fp.file._lock = &temp_f.lock; -#endif _IO_cookie_init (&temp_f.cfile, _IO_NO_READS, (void *) port, (cookie_io_functions_t) { write: do_write }); diff --git a/include/stdio.h b/include/stdio.h index 23b7fd288c..e9a7c77c24 100644 --- a/include/stdio.h +++ b/include/stdio.h @@ -1,5 +1,5 @@ #ifndef _STDIO_H -# if !defined _ISOMAC && defined _IO_MTSAFE_IO +# if !defined _ISOMAC # include <stdio-lock.h> # endif diff --git a/libio/Makefile b/libio/Makefile index e97387743f..88b4c20da6 100644 --- a/libio/Makefile +++ b/libio/Makefile @@ -92,8 +92,6 @@ routines += clearerr_u feof_u ferror_u fputc_u getc_u getchar_u \ iofputs_u endif -CPPFLAGS += $(libio-mtsafe) - # Support for exception handling. CFLAGS-fileops.c += -fexceptions CFLAGS-fputc.c += -fexceptions diff --git a/libio/clearerr.c b/libio/clearerr.c index 47d9ceae1a..5aecee096b 100644 --- a/libio/clearerr.c +++ b/libio/clearerr.c @@ -26,7 +26,3 @@ clearerr (FILE *fp) _IO_clearerr (fp); _IO_funlockfile (fp); } - -#ifndef _IO_MTSAFE_IO -weak_alias (clearerr, clearerr_unlocked) -#endif diff --git a/libio/feof.c b/libio/feof.c index d6b95f40e8..7f79b2795e 100644 --- a/libio/feof.c +++ b/libio/feof.c @@ -41,8 +41,3 @@ _IO_feof (FILE *fp) } weak_alias (_IO_feof, feof) - -#ifndef _IO_MTSAFE_IO -#undef feof_unlocked -weak_alias (_IO_feof, feof_unlocked) -#endif diff --git a/libio/ferror.c b/libio/ferror.c index bee2ef8f07..d2489c54d3 100644 --- a/libio/ferror.c +++ b/libio/ferror.c @@ -42,7 +42,3 @@ _IO_ferror (FILE *fp) weak_alias (_IO_ferror, ferror) -#ifndef _IO_MTSAFE_IO -#undef ferror_unlocked -weak_alias (_IO_ferror, ferror_unlocked) -#endif diff --git a/libio/fileno.c b/libio/fileno.c index ec479bedcf..3e5138fa0b 100644 --- a/libio/fileno.c +++ b/libio/fileno.c @@ -46,6 +46,6 @@ libc_hidden_weak (fileno) /* The fileno implementation for libio does not require locking because it only accesses once a single variable and this is already atomic - (at least at thread level). Therefore we don't test _IO_MTSAFE_IO here. */ + (at least at thread level). */ weak_alias (__fileno, fileno_unlocked) diff --git a/libio/fputc.c b/libio/fputc.c index 8143f2c096..d29a9d0aee 100644 --- a/libio/fputc.c +++ b/libio/fputc.c @@ -39,8 +39,3 @@ fputc (int c, FILE *fp) _IO_release_lock (fp); return result; } - -#ifndef _IO_MTSAFE_IO -#undef fputc_unlocked -weak_alias (fputc, fputc_unlocked) -#endif diff --git a/libio/genops.c b/libio/genops.c index 1b629eb695..ccfbcd149d 100644 --- a/libio/genops.c +++ b/libio/genops.c @@ -32,13 +32,10 @@ #include <stdbool.h> #include <sched.h> -#ifdef _IO_MTSAFE_IO static _IO_lock_t list_all_lock = _IO_lock_initializer; -#endif static FILE *run_fp; -#ifdef _IO_MTSAFE_IO static void flush_cleanup (void *not_used) { @@ -46,7 +43,6 @@ flush_cleanup (void *not_used) _IO_funlockfile (run_fp); _IO_lock_unlock (list_all_lock); } -#endif void _IO_un_link (struct _IO_FILE_plus *fp) @@ -54,12 +50,10 @@ _IO_un_link (struct _IO_FILE_plus *fp) if (fp->file._flags & _IO_LINKED) { FILE **f; -#ifdef _IO_MTSAFE_IO _IO_cleanup_region_start_noarg (flush_cleanup); _IO_lock_lock (list_all_lock); run_fp = (FILE *) fp; _IO_flockfile ((FILE *) fp); -#endif if (_IO_list_all == NULL) ; else if (fp == _IO_list_all) @@ -72,12 +66,10 @@ _IO_un_link (struct _IO_FILE_plus *fp) break; } fp->file._flags &= ~_IO_LINKED; -#ifdef _IO_MTSAFE_IO _IO_funlockfile ((FILE *) fp); run_fp = NULL; _IO_lock_unlock (list_all_lock); _IO_cleanup_region_end (0); -#endif } } libc_hidden_def (_IO_un_link) @@ -88,20 +80,16 @@ _IO_link_in (struct _IO_FILE_plus *fp) if ((fp->file._flags & _IO_LINKED) == 0) { fp->file._flags |= _IO_LINKED; -#ifdef _IO_MTSAFE_IO _IO_cleanup_region_start_noarg (flush_cleanup); _IO_lock_lock (list_all_lock); run_fp = (FILE *) fp; _IO_flockfile ((FILE *) fp); -#endif fp->file._chain = (FILE *) _IO_list_all; _IO_list_all = fp; -#ifdef _IO_MTSAFE_IO _IO_funlockfile ((FILE *) fp); run_fp = NULL; _IO_lock_unlock (list_all_lock); _IO_cleanup_region_end (0); -#endif } } libc_hidden_def (_IO_link_in) @@ -551,10 +539,8 @@ _IO_old_init (FILE *fp, int flags) #if _IO_JUMPS_OFFSET fp->_vtable_offset = 0; #endif -#ifdef _IO_MTSAFE_IO if (fp->_lock != NULL) _IO_lock_init (*fp->_lock); -#endif } void @@ -617,10 +603,8 @@ _IO_default_finish (FILE *fp, int dummy) _IO_un_link ((struct _IO_FILE_plus *) fp); -#ifdef _IO_MTSAFE_IO if (fp->_lock != NULL) _IO_lock_fini (*fp->_lock); -#endif } libc_hidden_def (_IO_default_finish) @@ -687,10 +671,8 @@ _IO_flush_all_lockp (int do_lock) int result = 0; FILE *fp; -#ifdef _IO_MTSAFE_IO _IO_cleanup_region_start_noarg (flush_cleanup); _IO_lock_lock (list_all_lock); -#endif for (fp = (FILE *) _IO_list_all; fp != NULL; fp = fp->_chain) { @@ -711,10 +693,8 @@ _IO_flush_all_lockp (int do_lock) run_fp = NULL; } -#ifdef _IO_MTSAFE_IO _IO_lock_unlock (list_all_lock); _IO_cleanup_region_end (0); -#endif return result; } @@ -733,10 +713,8 @@ _IO_flush_all_linebuffered (void) { FILE *fp; -#ifdef _IO_MTSAFE_IO _IO_cleanup_region_start_noarg (flush_cleanup); _IO_lock_lock (list_all_lock); -#endif for (fp = (FILE *) _IO_list_all; fp != NULL; fp = fp->_chain) { @@ -750,10 +728,8 @@ _IO_flush_all_linebuffered (void) run_fp = NULL; } -#ifdef _IO_MTSAFE_IO _IO_lock_unlock (list_all_lock); _IO_cleanup_region_end (0); -#endif } libc_hidden_def (_IO_flush_all_linebuffered) weak_alias (_IO_flush_all_linebuffered, _flushlbf) @@ -782,10 +758,8 @@ _IO_unbuffer_all (void) { FILE *fp; -#ifdef _IO_MTSAFE_IO _IO_cleanup_region_start_noarg (flush_cleanup); _IO_lock_lock (list_all_lock); -#endif for (fp = (FILE *) _IO_list_all; fp; fp = fp->_chain) { @@ -800,7 +774,6 @@ _IO_unbuffer_all (void) /* Iff stream is un-orientated, it wasn't used. */ && (legacy || fp->_mode != 0)) { -#ifdef _IO_MTSAFE_IO int cnt; #define MAXTRIES 2 for (cnt = 0; cnt < MAXTRIES; ++cnt) @@ -810,7 +783,6 @@ _IO_unbuffer_all (void) /* Give the other thread time to finish up its use of the stream. */ __sched_yield (); -#endif if (! legacy && ! dealloc_buffers && !(fp->_flags & _IO_USER_BUF)) { @@ -826,10 +798,8 @@ _IO_unbuffer_all (void) if (! legacy && fp->_mode > 0) _IO_wsetb (fp, NULL, NULL, 0); -#ifdef _IO_MTSAFE_IO if (cnt < MAXTRIES && fp->_lock != NULL) _IO_lock_unlock (*fp->_lock); -#endif } /* Make sure that never again the wide char functions can be @@ -838,10 +808,8 @@ _IO_unbuffer_all (void) fp->_mode = -1; } -#ifdef _IO_MTSAFE_IO _IO_lock_unlock (list_all_lock); _IO_cleanup_region_end (0); -#endif } @@ -1092,27 +1060,21 @@ libc_hidden_def (_IO_iter_file) void _IO_list_lock (void) { -#ifdef _IO_MTSAFE_IO _IO_lock_lock (list_all_lock); -#endif } libc_hidden_def (_IO_list_lock) void _IO_list_unlock (void) { -#ifdef _IO_MTSAFE_IO _IO_lock_unlock (list_all_lock); -#endif } libc_hidden_def (_IO_list_unlock) void _IO_list_resetlock (void) { -#ifdef _IO_MTSAFE_IO _IO_lock_init (list_all_lock); -#endif } libc_hidden_def (_IO_list_resetlock) diff --git a/libio/getc.c b/libio/getc.c index 55a6cc479a..8c79cf702f 100644 --- a/libio/getc.c +++ b/libio/getc.c @@ -46,9 +46,3 @@ _IO_getc (FILE *fp) weak_alias (_IO_getc, getc) weak_alias (_IO_getc, fgetc) - -#ifndef _IO_MTSAFE_IO -#undef getc_unlocked -weak_alias (_IO_getc, getc_unlocked) -weak_alias (_IO_getc, fgetc_unlocked) -#endif diff --git a/libio/getchar.c b/libio/getchar.c index 6ceff06a84..e0f4f471b3 100644 --- a/libio/getchar.c +++ b/libio/getchar.c @@ -39,9 +39,4 @@ getchar (void) result = _IO_getc_unlocked (stdin); _IO_release_lock (stdin); return result; -} - -#ifndef _IO_MTSAFE_IO -#undef getchar_unlocked -weak_alias (getchar, getchar_unlocked) -#endif +} \ No newline at end of file diff --git a/libio/iofdopen.c b/libio/iofdopen.c index 47b602d3c2..b8d082ebc5 100644 --- a/libio/iofdopen.c +++ b/libio/iofdopen.c @@ -37,9 +37,7 @@ _IO_new_fdopen (int fd, const char *mode) struct locked_FILE { struct _IO_FILE_plus fp; -#ifdef _IO_MTSAFE_IO _IO_lock_t lock; -#endif struct _IO_wide_data wd; } *new_f; int i; @@ -122,9 +120,7 @@ _IO_new_fdopen (int fd, const char *mode) new_f = (struct locked_FILE *) malloc (sizeof (struct locked_FILE)); if (new_f == NULL) return NULL; -#ifdef _IO_MTSAFE_IO new_f->fp.file._lock = &new_f->lock; -#endif _IO_no_init (&new_f->fp.file, 0, 0, &new_f->wd, #if _G_HAVE_MMAP (use_mmap && (read_write & _IO_NO_WRITES)) diff --git a/libio/iofflush.c b/libio/iofflush.c index 5b542664a8..6ea046cf48 100644 --- a/libio/iofflush.c +++ b/libio/iofflush.c @@ -46,10 +46,3 @@ libc_hidden_def (_IO_fflush) weak_alias (_IO_fflush, fflush) libc_hidden_weak (fflush) - -#ifndef _IO_MTSAFE_IO -strong_alias (_IO_fflush, __fflush_unlocked) -libc_hidden_def (__fflush_unlocked) -weak_alias (_IO_fflush, fflush_unlocked) -libc_hidden_weak (fflush_unlocked) -#endif diff --git a/libio/iofgets.c b/libio/iofgets.c index 58d30fca96..f19526de46 100644 --- a/libio/iofgets.c +++ b/libio/iofgets.c @@ -66,10 +66,3 @@ _IO_fgets (char *buf, int n, FILE *fp) } weak_alias (_IO_fgets, fgets) - -# ifndef _IO_MTSAFE_IO -strong_alias (_IO_fgets, __fgets_unlocked) -libc_hidden_def (__fgets_unlocked) -weak_alias (_IO_fgets, fgets_unlocked) -libc_hidden_weak (fgets_unlocked) -# endif diff --git a/libio/iofopen.c b/libio/iofopen.c index e7471fa3ae..777f8c9e1c 100644 --- a/libio/iofopen.c +++ b/libio/iofopen.c @@ -58,17 +58,13 @@ __fopen_internal (const char *filename, const char *mode, int is32) struct locked_FILE { struct _IO_FILE_plus fp; -#ifdef _IO_MTSAFE_IO _IO_lock_t lock; -#endif struct _IO_wide_data wd; } *new_f = (struct locked_FILE *) malloc (sizeof (struct locked_FILE)); if (new_f == NULL) return NULL; -#ifdef _IO_MTSAFE_IO new_f->fp.file._lock = &new_f->lock; -#endif _IO_no_init (&new_f->fp.file, 0, 0, &new_f->wd, &_IO_wfile_jumps); _IO_JUMPS (&new_f->fp) = &_IO_file_jumps; _IO_new_file_init_internal (&new_f->fp); diff --git a/libio/iofopncook.c b/libio/iofopncook.c index e108ad2199..16bcc7f4a3 100644 --- a/libio/iofopncook.c +++ b/libio/iofopncook.c @@ -179,9 +179,7 @@ _IO_fopencookie (void *cookie, const char *mode, struct locked_FILE { struct _IO_cookie_file cfile; -#ifdef _IO_MTSAFE_IO _IO_lock_t lock; -#endif } *new_f; switch (*mode++) @@ -205,9 +203,7 @@ _IO_fopencookie (void *cookie, const char *mode, new_f = (struct locked_FILE *) malloc (sizeof (struct locked_FILE)); if (new_f == NULL) return NULL; -#ifdef _IO_MTSAFE_IO new_f->cfile.__fp.file._lock = &new_f->lock; -#endif _IO_cookie_init (&new_f->cfile, read_write, cookie, io_functions); diff --git a/libio/iofputs.c b/libio/iofputs.c index 999e069877..f8e0ee7200 100644 --- a/libio/iofputs.c +++ b/libio/iofputs.c @@ -44,10 +44,3 @@ libc_hidden_def (_IO_fputs) weak_alias (_IO_fputs, fputs) libc_hidden_weak (fputs) - -# ifndef _IO_MTSAFE_IO -strong_alias (_IO_fputs, __fputs_unlocked) -libc_hidden_def (__fputs_unlocked) -weak_alias (_IO_fputs, fputs_unlocked) -libc_hidden_ver (_IO_fputs, fputs_unlocked) -# endif diff --git a/libio/iofread.c b/libio/iofread.c index d358136953..66f69ac93f 100644 --- a/libio/iofread.c +++ b/libio/iofread.c @@ -42,9 +42,3 @@ _IO_fread (void *buf, size_t size, size_t count, FILE *fp) libc_hidden_def (_IO_fread) weak_alias (_IO_fread, fread) - -# ifndef _IO_MTSAFE_IO -strong_alias (_IO_fread, __fread_unlocked) -libc_hidden_def (__fread_unlocked) -weak_alias (_IO_fread, fread_unlocked) -# endif diff --git a/libio/iofwrite.c b/libio/iofwrite.c index f86b5458da..ff741fec8e 100644 --- a/libio/iofwrite.c +++ b/libio/iofwrite.c @@ -24,6 +24,7 @@ This exception applies to code released by its copyright holders in files containing the exception. */ +#include <stdio.h> #include "libioP.h" size_t @@ -49,10 +50,5 @@ _IO_fwrite (const void *buf, size_t size, size_t count, FILE *fp) } libc_hidden_def (_IO_fwrite) -# include <stdio.h> weak_alias (_IO_fwrite, fwrite) libc_hidden_weak (fwrite) -# ifndef _IO_MTSAFE_IO -weak_alias (_IO_fwrite, fwrite_unlocked) -libc_hidden_weak (fwrite_unlocked) -# endif diff --git a/libio/iopopen.c b/libio/iopopen.c index 06778cf110..676c739aba 100644 --- a/libio/iopopen.c +++ b/libio/iopopen.c @@ -49,7 +49,6 @@ static const struct _IO_jump_t _IO_proc_jumps; static struct _IO_proc_file *proc_file_chain; -#ifdef _IO_MTSAFE_IO static _IO_lock_t proc_file_chain_lock = _IO_lock_initializer; static void @@ -57,7 +56,6 @@ unlock (void *not_used) { _IO_lock_unlock (proc_file_chain_lock); } -#endif /* POSIX states popen shall ensure that any streams from previous popen() calls that remain open in the parent process should be closed in the new @@ -189,16 +187,12 @@ _IO_new_proc_open (FILE *fp, const char *command, const char *mode) child_pipe_fd) != 0) goto spawn_failure; -#ifdef _IO_MTSAFE_IO _IO_cleanup_region_start_noarg (unlock); _IO_lock_lock (proc_file_chain_lock); -#endif spawn_ok = spawn_process (&fa, fp, command, do_cloexec, pipe_fds, parent_end, child_end, child_pipe_fd); -#ifdef _IO_MTSAFE_IO _IO_lock_unlock (proc_file_chain_lock); _IO_cleanup_region_end (0); -#endif __posix_spawn_file_actions_destroy (&fa); @@ -221,18 +215,14 @@ _IO_new_popen (const char *command, const char *mode) struct locked_FILE { struct _IO_proc_file fpx; -#ifdef _IO_MTSAFE_IO _IO_lock_t lock; -#endif } *new_f; FILE *fp; new_f = (struct locked_FILE *) malloc (sizeof (struct locked_FILE)); if (new_f == NULL) return NULL; -#ifdef _IO_MTSAFE_IO new_f->fpx.file.file._lock = &new_f->lock; -#endif fp = &new_f->fpx.file.file; _IO_init_internal (fp, 0); _IO_JUMPS (&new_f->fpx.file) = &_IO_proc_jumps; @@ -254,10 +244,8 @@ _IO_new_proc_close (FILE *fp) int status = -1; /* Unlink from proc_file_chain. */ -#ifdef _IO_MTSAFE_IO _IO_cleanup_region_start_noarg (unlock); _IO_lock_lock (proc_file_chain_lock); -#endif for ( ; *ptr != NULL; ptr = &(*ptr)->next) { if (*ptr == (_IO_proc_file *) fp) @@ -267,10 +255,8 @@ _IO_new_proc_close (FILE *fp) break; } } -#ifdef _IO_MTSAFE_IO _IO_lock_unlock (proc_file_chain_lock); _IO_cleanup_region_end (0); -#endif if (status < 0 || __close_nocancel (_IO_fileno(fp)) < 0) return -1; diff --git a/libio/iovdprintf.c b/libio/iovdprintf.c index 1e483868c9..4c5a90e58d 100644 --- a/libio/iovdprintf.c +++ b/libio/iovdprintf.c @@ -35,9 +35,7 @@ __vdprintf_internal (int d, const char *format, va_list arg, struct _IO_wide_data wd; int done; -#ifdef _IO_MTSAFE_IO tmpfil.file._lock = NULL; -#endif _IO_no_init (&tmpfil.file, _IO_USER_LOCK, 0, &wd, &_IO_wfile_jumps); _IO_JUMPS (&tmpfil) = &_IO_file_jumps; _IO_new_file_init_internal (&tmpfil); diff --git a/libio/iovsprintf.c b/libio/iovsprintf.c index 72c67bf27b..cbe59f6fec 100644 --- a/libio/iovsprintf.c +++ b/libio/iovsprintf.c @@ -73,9 +73,7 @@ __vsprintf_internal (char *string, size_t maxlen, _IO_strfile sf; int ret; -#ifdef _IO_MTSAFE_IO sf._sbf._f._lock = NULL; -#endif _IO_no_init (&sf._sbf._f, _IO_USER_LOCK, -1, NULL, NULL); /* When called from fortified sprintf/vsprintf, erase the destination buffer and try to detect overflows. When called from regular diff --git a/libio/libio.h b/libio/libio.h index d0184df878..42ff6143af 100644 --- a/libio/libio.h +++ b/libio/libio.h @@ -36,7 +36,7 @@ #include <stdio.h> -#if defined _IO_MTSAFE_IO && !defined _IO_lock_t_defined +#if !defined _IO_lock_t_defined # error "Someone forgot to include stdio-lock.h" #endif @@ -198,10 +198,13 @@ extern void _IO_flockfile (FILE *) __THROW; extern void _IO_funlockfile (FILE *) __THROW; extern int _IO_ftrylockfile (FILE *) __THROW; -#define _IO_peekc(_fp) _IO_peekc_unlocked (_fp) -#define _IO_flockfile(_fp) /**/ -#define _IO_funlockfile(_fp) ((void) 0) -#define _IO_ftrylockfile(_fp) /**/ +#define _IO_peekc(_fp) _IO_peekc_locked (_fp) +#define _IO_flockfile(_fp) \ + if (((_fp)->_flags & _IO_USER_LOCK) == 0) _IO_lock_lock (*(_fp)->_lock) +#define _IO_funlockfile(_fp) \ + if (((_fp)->_flags & _IO_USER_LOCK) == 0) _IO_lock_unlock (*(_fp)->_lock) +#define _IO_ftrylockfile(__fp) \ + if (((_fp)->_flags & _IO_USER_LOCK) == 0) _IO_lock_trylock (*(_fp)->_lock) #ifndef _IO_cleanup_region_start #define _IO_cleanup_region_start(_fct, _fp) /**/ #endif @@ -272,17 +275,4 @@ libc_hidden_proto (_IO_padn) libc_hidden_proto (_IO_putc) libc_hidden_proto (_IO_sgetn) -#ifdef _IO_MTSAFE_IO -# undef _IO_peekc -# undef _IO_flockfile -# undef _IO_funlockfile -# undef _IO_ftrylockfile - -# define _IO_peekc(_fp) _IO_peekc_locked (_fp) -# define _IO_flockfile(_fp) \ - if (((_fp)->_flags & _IO_USER_LOCK) == 0) _IO_lock_lock (*(_fp)->_lock) -# define _IO_funlockfile(_fp) \ - if (((_fp)->_flags & _IO_USER_LOCK) == 0) _IO_lock_unlock (*(_fp)->_lock) -#endif /* _IO_MTSAFE_IO */ - #endif /* _LIBIO_H */ diff --git a/libio/libioP.h b/libio/libioP.h index ba4fdbd200..114084b83a 100644 --- a/libio/libioP.h +++ b/libio/libioP.h @@ -803,33 +803,18 @@ extern int __vfwscanf_internal (FILE *fp, const wchar_t *format, va_list argp, extern int _IO_vscanf (const char *, va_list) __THROW; -#ifdef _IO_MTSAFE_IO /* check following! */ -# ifdef _IO_USE_OLD_IO_FILE -# define FILEBUF_LITERAL(CHAIN, FLAGS, FD, WDP) \ +#ifdef _IO_USE_OLD_IO_FILE +# define FILEBUF_LITERAL(CHAIN, FLAGS, FD, WDP) \ { _IO_MAGIC+_IO_LINKED+_IO_IS_FILEBUF+FLAGS, \ 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, (FILE *) CHAIN, FD, \ 0, _IO_pos_BAD, 0, 0, { 0 }, &_IO_stdfile_##FD##_lock } -# else -# define FILEBUF_LITERAL(CHAIN, FLAGS, FD, WDP) \ - { _IO_MAGIC+_IO_LINKED+_IO_IS_FILEBUF+FLAGS, \ - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, (FILE *) CHAIN, FD, \ - 0, _IO_pos_BAD, 0, 0, { 0 }, &_IO_stdfile_##FD##_lock, _IO_pos_BAD,\ - NULL, WDP, 0 } -# endif #else -# ifdef _IO_USE_OLD_IO_FILE -# define FILEBUF_LITERAL(CHAIN, FLAGS, FD, WDP) \ +# define FILEBUF_LITERAL(CHAIN, FLAGS, FD, WDP) \ { _IO_MAGIC+_IO_LINKED+_IO_IS_FILEBUF+FLAGS, \ 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, (FILE *) CHAIN, FD, \ - 0, _IO_pos_BAD } -# else -# define FILEBUF_LITERAL(CHAIN, FLAGS, FD, WDP) \ - { _IO_MAGIC+_IO_LINKED+_IO_IS_FILEBUF+FLAGS, \ - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, (FILE *) CHAIN, FD, \ - 0, _IO_pos_BAD, 0, 0, { 0 }, 0, _IO_pos_BAD, \ + 0, _IO_pos_BAD, 0, 0, { 0 }, &_IO_stdfile_##FD##_lock, _IO_pos_BAD,\ NULL, WDP, 0 } -# endif #endif #if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1) @@ -884,13 +869,6 @@ _IO_acquire_lock_fct (FILE **p) _IO_funlockfile (fp); } -#if !defined _IO_MTSAFE_IO && IS_IN (libc) -# define _IO_acquire_lock(_fp) \ - do { -# define _IO_release_lock(_fp) \ - } while (0) -#endif - /* Collect all vtables in a special section for vtable verification. These symbols cover the extent of this section. */ symbol_set_declare (__libc_IO_vtables) diff --git a/libio/memstream.c b/libio/memstream.c index 1ae8cd9544..7ab61876ba 100644 --- a/libio/memstream.c +++ b/libio/memstream.c @@ -66,9 +66,7 @@ __open_memstream (char **bufloc, size_t *sizeloc) struct locked_FILE { struct _IO_FILE_memstream fp; -#ifdef _IO_MTSAFE_IO _IO_lock_t lock; -#endif struct _IO_wide_data wd; } *new_f; char *buf; @@ -76,9 +74,7 @@ __open_memstream (char **bufloc, size_t *sizeloc) new_f = (struct locked_FILE *) malloc (sizeof (struct locked_FILE)); if (new_f == NULL) return NULL; -#ifdef _IO_MTSAFE_IO new_f->fp._sf._sbf._f._lock = &new_f->lock; -#endif buf = calloc (1, BUFSIZ); if (buf == NULL) diff --git a/libio/obprintf.c b/libio/obprintf.c index c220be9adc..ed0f39d85a 100644 --- a/libio/obprintf.c +++ b/libio/obprintf.c @@ -127,9 +127,7 @@ __obstack_vprintf_internal (struct obstack *obstack, const char *format, int size; int room; -#ifdef _IO_MTSAFE_IO new_f.ofile.file.file._lock = NULL; -#endif _IO_no_init (&new_f.ofile.file.file, _IO_USER_LOCK, -1, NULL, NULL); _IO_JUMPS (&new_f.ofile.file) = &_IO_obstack_jumps; diff --git a/libio/oldiofdopen.c b/libio/oldiofdopen.c index 1ce050f1f4..0b0097e9e2 100644 --- a/libio/oldiofdopen.c +++ b/libio/oldiofdopen.c @@ -41,9 +41,7 @@ _IO_old_fdopen (int fd, const char *mode) struct locked_FILE { struct _IO_FILE_complete_plus fp; -#ifdef _IO_MTSAFE_IO _IO_lock_t lock; -#endif } *new_f; int fd_flags; @@ -96,9 +94,7 @@ _IO_old_fdopen (int fd, const char *mode) new_f = (struct locked_FILE *) malloc (sizeof (struct locked_FILE)); if (new_f == NULL) return NULL; -#ifdef _IO_MTSAFE_IO new_f->fp.file._file._lock = &new_f->lock; -#endif _IO_old_init (&new_f->fp.file._file, 0); _IO_JUMPS_FILE_plus (&new_f->fp) = &_IO_old_file_jumps; _IO_old_file_init_internal ((struct _IO_FILE_plus *) &new_f->fp); diff --git a/libio/oldiofopen.c b/libio/oldiofopen.c index c73911fdd8..f3abd9ee07 100644 --- a/libio/oldiofopen.c +++ b/libio/oldiofopen.c @@ -39,16 +39,12 @@ _IO_old_fopen (const char *filename, const char *mode) struct locked_FILE { struct _IO_FILE_complete_plus fp; -#ifdef _IO_MTSAFE_IO _IO_lock_t lock; -#endif } *new_f = (struct locked_FILE *) malloc (sizeof (struct locked_FILE)); if (new_f == NULL) return NULL; -#ifdef _IO_MTSAFE_IO new_f->fp.file._file._lock = &new_f->lock; -#endif _IO_old_init (&new_f->fp.file._file, 0); _IO_JUMPS_FILE_plus (&new_f->fp) = &_IO_old_file_jumps; _IO_old_file_init_internal ((struct _IO_FILE_plus *) &new_f->fp); diff --git a/libio/oldiopopen.c b/libio/oldiopopen.c index f9149413e8..e24acf752e 100644 --- a/libio/oldiopopen.c +++ b/libio/oldiopopen.c @@ -47,7 +47,6 @@ typedef struct _IO_proc_file _IO_proc_file; static struct _IO_proc_file *old_proc_file_chain; -#ifdef _IO_MTSAFE_IO static _IO_lock_t proc_file_chain_lock = _IO_lock_initializer; static void @@ -55,7 +54,6 @@ unlock (void *not_used) { _IO_lock_unlock (proc_file_chain_lock); } -#endif FILE * attribute_compat_text_section @@ -118,16 +116,12 @@ _IO_old_proc_open (FILE *fp, const char *command, const char *mode) _IO_fileno (fp) = parent_end; /* Link into old_proc_file_chain. */ -#ifdef _IO_MTSAFE_IO _IO_cleanup_region_start_noarg (unlock); _IO_lock_lock (proc_file_chain_lock); -#endif ((_IO_proc_file *) fp)->next = old_proc_file_chain; old_proc_file_chain = (_IO_proc_file *) fp; -#ifdef _IO_MTSAFE_IO _IO_lock_unlock (proc_file_chain_lock); _IO_cleanup_region_end (0); -#endif _IO_mask_flags (fp, read_or_write, _IO_NO_READS|_IO_NO_WRITES); return fp; @@ -140,18 +134,14 @@ _IO_old_popen (const char *command, const char *mode) struct locked_FILE { struct _IO_proc_file fpx; -#ifdef _IO_MTSAFE_IO _IO_lock_t lock; -#endif } *new_f; FILE *fp; new_f = (struct locked_FILE *) malloc (sizeof (struct locked_FILE)); if (new_f == NULL) return NULL; -#ifdef _IO_MTSAFE_IO new_f->fpx.file.file._file._lock = &new_f->lock; -#endif fp = &new_f->fpx.file.file._file; _IO_old_init (fp, 0); _IO_JUMPS_FILE_plus (&new_f->fpx.file) = &_IO_old_proc_jumps; @@ -174,10 +164,8 @@ _IO_old_proc_close (FILE *fp) int status = -1; /* Unlink from old_proc_file_chain. */ -#ifdef _IO_MTSAFE_IO _IO_cleanup_region_start_noarg (unlock); _IO_lock_lock (proc_file_chain_lock); -#endif for ( ; *ptr != NULL; ptr = &(*ptr)->next) { if (*ptr == (_IO_proc_file *) fp) @@ -187,10 +175,8 @@ _IO_old_proc_close (FILE *fp) break; } } -#ifdef _IO_MTSAFE_IO _IO_lock_unlock (proc_file_chain_lock); _IO_cleanup_region_end (0); -#endif if (status < 0 || __close (_IO_fileno(fp)) < 0) return -1; diff --git a/libio/oldstdfiles.c b/libio/oldstdfiles.c index 81a7f4f64d..30d8e39ef9 100644 --- a/libio/oldstdfiles.c +++ b/libio/oldstdfiles.c @@ -33,16 +33,10 @@ #define _IO_USE_OLD_IO_FILE #include "libioP.h" -#ifdef _IO_MTSAFE_IO #define DEF_STDFILE(NAME, FD, CHAIN, FLAGS) \ static _IO_lock_t _IO_stdfile_##FD##_lock = _IO_lock_initializer; \ struct _IO_FILE_plus NAME \ = {FILEBUF_LITERAL(CHAIN, FLAGS, FD, NULL), &_IO_old_file_jumps}; -#else -#define DEF_STDFILE(NAME, FD, CHAIN, FLAGS) \ - struct _IO_FILE_plus NAME \ - = {FILEBUF_LITERAL(CHAIN, FLAGS, FD, NULL), &_IO_old_file_jumps}; -#endif DEF_STDFILE(_IO_stdin_, 0, 0, _IO_NO_WRITES); DEF_STDFILE(_IO_stdout_, 1, &_IO_stdin_, _IO_NO_READS); diff --git a/libio/putc.c b/libio/putc.c index 7036c5b6bb..44e30649c9 100644 --- a/libio/putc.c +++ b/libio/putc.c @@ -37,8 +37,3 @@ libc_hidden_def (_IO_putc) #undef putc weak_alias (_IO_putc, putc) - -#ifndef _IO_MTSAFE_IO -#undef putc_unlocked -weak_alias (_IO_putc, putc_unlocked) -#endif diff --git a/libio/putchar.c b/libio/putchar.c index 7eff3d5f94..9aff59c090 100644 --- a/libio/putchar.c +++ b/libio/putchar.c @@ -29,8 +29,3 @@ putchar (int c) _IO_release_lock (stdout); return result; } - -#if defined weak_alias && !defined _IO_MTSAFE_IO -#undef putchar_unlocked -weak_alias (putchar, putchar_unlocked) -#endif diff --git a/libio/stdfiles.c b/libio/stdfiles.c index 31cf72dfd0..b5c1dc4208 100644 --- a/libio/stdfiles.c +++ b/libio/stdfiles.c @@ -32,22 +32,13 @@ #include "libioP.h" -#ifdef _IO_MTSAFE_IO -# define DEF_STDFILE(NAME, FD, CHAIN, FLAGS) \ +#define DEF_STDFILE(NAME, FD, CHAIN, FLAGS) \ static _IO_lock_t _IO_stdfile_##FD##_lock = _IO_lock_initializer; \ static struct _IO_wide_data _IO_wide_data_##FD \ = { ._wide_vtable = &_IO_wfile_jumps }; \ struct _IO_FILE_plus NAME \ = {FILEBUF_LITERAL(CHAIN, FLAGS, FD, &_IO_wide_data_##FD), \ &_IO_file_jumps}; -#else -# define DEF_STDFILE(NAME, FD, CHAIN, FLAGS) \ - static struct _IO_wide_data _IO_wide_data_##FD \ - = { ._wide_vtable = &_IO_wfile_jumps }; \ - struct _IO_FILE_plus NAME \ - = {FILEBUF_LITERAL(CHAIN, FLAGS, FD, &_IO_wide_data_##FD), \ - &_IO_file_jumps}; -#endif DEF_STDFILE(_IO_2_1_stdin_, 0, 0, _IO_NO_WRITES); DEF_STDFILE(_IO_2_1_stdout_, 1, &_IO_2_1_stdin_, _IO_NO_READS); diff --git a/libio/vasprintf.c b/libio/vasprintf.c index 4430a266c6..c90a21cb6f 100644 --- a/libio/vasprintf.c +++ b/libio/vasprintf.c @@ -45,9 +45,7 @@ __vasprintf_internal (char **result_ptr, const char *format, va_list args, string = (char *) malloc (init_string_size); if (string == NULL) return -1; -#ifdef _IO_MTSAFE_IO sf._sbf._f._lock = NULL; -#endif _IO_no_init (&sf._sbf._f, _IO_USER_LOCK, -1, NULL, NULL); _IO_JUMPS (&sf._sbf) = &_IO_str_jumps; _IO_str_init_static_internal (&sf, string, init_string_size, string); diff --git a/libio/vsnprintf.c b/libio/vsnprintf.c index 8dae66761d..5aa01665d8 100644 --- a/libio/vsnprintf.c +++ b/libio/vsnprintf.c @@ -95,9 +95,7 @@ __vsnprintf_internal (char *string, size_t maxlen, const char *format, { _IO_strnfile sf; int ret; -#ifdef _IO_MTSAFE_IO sf.f._sbf._f._lock = NULL; -#endif /* We need to handle the special case where MAXLEN is 0. Use the overflow buffer right from the start. */ diff --git a/libio/vswprintf.c b/libio/vswprintf.c index 5a9ccdff3e..c6f4706573 100644 --- a/libio/vswprintf.c +++ b/libio/vswprintf.c @@ -95,9 +95,7 @@ __vswprintf_internal (wchar_t *string, size_t maxlen, const wchar_t *format, _IO_wstrnfile sf; int ret; struct _IO_wide_data wd; -#ifdef _IO_MTSAFE_IO sf.f._sbf._f._lock = NULL; -#endif if (maxlen == 0) /* Since we have to write at least the terminating L'\0' a buffer diff --git a/libio/wgenops.c b/libio/wgenops.c index e7ea75aed2..96e10f6d1c 100644 --- a/libio/wgenops.c +++ b/libio/wgenops.c @@ -185,10 +185,8 @@ _IO_wdefault_finish (FILE *fp, int dummy) fp->_IO_save_base = NULL; } -#ifdef _IO_MTSAFE_IO if (fp->_lock != NULL) _IO_lock_fini (*fp->_lock); -#endif _IO_un_link ((struct _IO_FILE_plus *) fp); } diff --git a/libio/wmemstream.c b/libio/wmemstream.c index e7f898c7fb..9366ef4aad 100644 --- a/libio/wmemstream.c +++ b/libio/wmemstream.c @@ -67,9 +67,7 @@ open_wmemstream (wchar_t **bufloc, size_t *sizeloc) struct locked_FILE { struct _IO_FILE_wmemstream fp; -#ifdef _IO_MTSAFE_IO _IO_lock_t lock; -#endif struct _IO_wide_data wd; } *new_f; wchar_t *buf; @@ -77,9 +75,7 @@ open_wmemstream (wchar_t **bufloc, size_t *sizeloc) new_f = (struct locked_FILE *) malloc (sizeof (struct locked_FILE)); if (new_f == NULL) return NULL; -#ifdef _IO_MTSAFE_IO new_f->fp._sf._sbf._f._lock = &new_f->lock; -#endif buf = calloc (1, BUFSIZ); if (buf == NULL) diff --git a/nptl/Makefile b/nptl/Makefile index b585663974..a85da416de 100644 --- a/nptl/Makefile +++ b/nptl/Makefile @@ -432,9 +432,6 @@ $(objpfx)multidir.mk: $(common-objpfx)config.make endif -CFLAGS-ftrylockfile.c += $(libio-mtsafe) -CFLAGS-funlockfile.c += $(libio-mtsafe) - link-libc-static := $(common-objpfx)libc.a $(static-gnulib) \ $(common-objpfx)libc.a diff --git a/pwd/Makefile b/pwd/Makefile index 848e7d0e12..da8e84c46c 100644 --- a/pwd/Makefile +++ b/pwd/Makefile @@ -37,6 +37,5 @@ ifeq ($(have-thread-library),yes) CFLAGS-getpwent_r.c += -fexceptions CFLAGS-getpwent.c += -fexceptions CFLAGS-getpw.c += -fexceptions -CFLAGS-fgetpwent_r.c += $(libio-mtsafe) endif diff --git a/shadow/Makefile b/shadow/Makefile index b9faa3aa53..1b4ee5d50f 100644 --- a/shadow/Makefile +++ b/shadow/Makefile @@ -32,8 +32,8 @@ tests = tst-shadow tst-putspent CFLAGS-getspent_r.c += -fexceptions CFLAGS-getspent.c += -fexceptions CFLAGS-fgetspent.c += -fexceptions -CFLAGS-fgetspent_r.c += -fexceptions $(libio-mtsafe) -CFLAGS-putspent.c += -fexceptions $(libio-mtsafe) +CFLAGS-fgetspent_r.c += -fexceptions +CFLAGS-putspent.c += -fexceptions CFLAGS-getspnam.c += -fexceptions CFLAGS-getspnam_r.c += -fexceptions diff --git a/stdio-common/Makefile b/stdio-common/Makefile index a1603e82fe..ddeacb1342 100644 --- a/stdio-common/Makefile +++ b/stdio-common/Makefile @@ -357,8 +357,6 @@ CFLAGS-tst-gets.c += -Wno-deprecated-declarations # the fortified version had the same bug. CFLAGS-tst-bz11319-fortify2.c += -D_FORTIFY_SOURCE=2 -CPPFLAGS += $(libio-mtsafe) - $(objpfx)tst-setvbuf1.out: /dev/null $(objpfx)tst-setvbuf1 $(test-program-cmd) > $@ 2>&1; \ $(evaluate-test) diff --git a/stdio-common/vfprintf-internal.c b/stdio-common/vfprintf-internal.c index 59bd76c890..b915803c66 100644 --- a/stdio-common/vfprintf-internal.c +++ b/stdio-common/vfprintf-internal.c @@ -2141,9 +2141,7 @@ struct helper_file struct _IO_wide_data _wide_data; #endif FILE *_put_stream; -#ifdef _IO_MTSAFE_IO _IO_lock_t lock; -#endif }; static int @@ -2251,9 +2249,7 @@ buffered_vfprintf (FILE *s, const CHAR_T *format, va_list args, #if _IO_JUMPS_OFFSET hp->_vtable_offset = 0; #endif -#ifdef _IO_MTSAFE_IO hp->_lock = NULL; -#endif hp->_flags2 = s->_flags2; _IO_JUMPS (&helper._f) = (struct _IO_jump_t *) &_IO_helper_jumps; diff --git a/stdlib/Makefile b/stdlib/Makefile index 60fc59c12c..1a10b18931 100644 --- a/stdlib/Makefile +++ b/stdlib/Makefile @@ -359,15 +359,6 @@ CFLAGS-system.c += -fexceptions CFLAGS-system.os = -fomit-frame-pointer CFLAGS-fmtmsg.c += -fexceptions -CFLAGS-strfmon.c += $(libio-mtsafe) -CFLAGS-strfmon_l.c += $(libio-mtsafe) - -# The strfrom class of functions call __printf_fp in order to convert the -# floating-point value to characters. This requires the value of IO_MTSAFE_IO. -CFLAGS-strfromd.c += $(libio-mtsafe) -CFLAGS-strfromf.c += $(libio-mtsafe) -CFLAGS-strfroml.c += $(libio-mtsafe) - CFLAGS-tst-bsearch.c += $(stack-align-test-flags) CFLAGS-tst-qsort.c += $(stack-align-test-flags) CFLAGS-tst-makecontext.c += -funwind-tables diff --git a/stdlib/strfmon_l.c b/stdlib/strfmon_l.c index d9b22088c7..e6fb378877 100644 --- a/stdlib/strfmon_l.c +++ b/stdlib/strfmon_l.c @@ -523,9 +523,7 @@ __vstrfmon_l_internal (char *s, size_t maxsize, locale_t loc, out_string (sign_string); /* Print the number. */ -#ifdef _IO_MTSAFE_IO f._sbf._f._lock = NULL; -#endif _IO_init_internal (&f._sbf._f, 0); _IO_JUMPS (&f._sbf) = &_IO_str_jumps; _IO_str_init_static_internal (&f, dest, (s + maxsize) - dest, dest); diff --git a/stdlib/strfrom-skeleton.c b/stdlib/strfrom-skeleton.c index 1fba04bf6a..0ccdd756cd 100644 --- a/stdlib/strfrom-skeleton.c +++ b/stdlib/strfrom-skeleton.c @@ -37,9 +37,7 @@ int STRFROM (char *dest, size_t size, const char *format, FLOAT f) { _IO_strnfile sfile; -#ifdef _IO_MTSAFE_IO sfile.f._sbf._f._lock = NULL; -#endif int done; diff --git a/sysdeps/generic/stdio-lock.h b/sysdeps/generic/stdio-lock.h index cfdaf82577..14cf458bdd 100644 --- a/sysdeps/generic/stdio-lock.h +++ b/sysdeps/generic/stdio-lock.h @@ -27,8 +27,8 @@ __libc_lock_define_recursive (typedef, _IO_lock_t) /* We need recursive (counting) mutexes. */ #ifdef _LIBC_LOCK_RECURSIVE_INITIALIZER # define _IO_lock_initializer _LIBC_LOCK_RECURSIVE_INITIALIZER -#elif _IO_MTSAFE_IO - #error libio needs recursive mutexes for _IO_MTSAFE_IO +#else +# error libio needs recursive mutexes #endif #define _IO_lock_init(_name) __libc_lock_init_recursive (_name) diff --git a/sysdeps/ieee754/float128/Makefile b/sysdeps/ieee754/float128/Makefile index 571a841809..166e630290 100644 --- a/sysdeps/ieee754/float128/Makefile +++ b/sysdeps/ieee754/float128/Makefile @@ -1,10 +1,6 @@ ifeq ($(subdir),stdlib) routines += float1282mpn strfromf128 routines += strtof128 strtof128_l strtof128_nan mpn2float128 - -# The strfrom class of functions call __printf_fp in order to convert the -# floating-point value to characters. This requires the value of IO_MTSAFE_IO. -CFLAGS-strfromf128.c += $(libio-mtsafe) endif ifeq ($(subdir),wcsmbs) diff --git a/sysdeps/nptl/libc-lock.h b/sysdeps/nptl/libc-lock.h index 5af476c48b..c186375c31 100644 --- a/sysdeps/nptl/libc-lock.h +++ b/sysdeps/nptl/libc-lock.h @@ -25,7 +25,7 @@ /* Mutex type. */ -#if defined _LIBC || defined _IO_MTSAFE_IO +#if defined _LIBC # if (!IS_IN (libc) && !IS_IN (libpthread)) || !defined _LIBC typedef struct { pthread_mutex_t mutex; } __libc_lock_recursive_t; # else diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile index ca953804d0..b3bf4217a5 100644 --- a/sysdeps/unix/sysv/linux/Makefile +++ b/sysdeps/unix/sysv/linux/Makefile @@ -317,7 +317,6 @@ tests += tst-affinity tst-affinity-pid tests-static := tst-affinity-static tests += $(tests-static) -CFLAGS-fork.c = $(libio-mtsafe) CFLAGS-getpid.o = -fomit-frame-pointer CFLAGS-getpid.os = -fomit-frame-pointer endif diff --git a/wcsmbs/Makefile b/wcsmbs/Makefile index df9a85f4a9..afb0610b55 100644 --- a/wcsmbs/Makefile +++ b/wcsmbs/Makefile @@ -103,8 +103,6 @@ CFLAGS-isoc99_fwscanf.c += -fexceptions CFLAGS-isoc99_vwscanf.c += -fexceptions CFLAGS-isoc99_vfwscanf.c += -fexceptions -CPPFLAGS += $(libio-mtsafe) - # We need to find the default version of strtold_l in stdlib. CPPFLAGS-wcstold_l.c += -I../stdlib -- 2.34.1 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/4] libio: Assume _IO_MTSAFE_IO 2022-04-26 19:15 ` [PATCH v2 1/4] libio: Assume _IO_MTSAFE_IO Adhemerval Zanella @ 2022-04-27 12:34 ` Florian Weimer 2022-04-27 18:40 ` Adhemerval Zanella 0 siblings, 1 reply; 23+ messages in thread From: Florian Weimer @ 2022-04-27 12:34 UTC (permalink / raw) To: Adhemerval Zanella via Libc-alpha * Adhemerval Zanella via Libc-alpha: > It is already set by default on all supported architectures and it is > an expectation that stdio works on multi-threaded environments. So … this cleanup has got stuck in the past because it's actually more than just a cleanup. We actually build most of glibc without libio locking. Looks like misc/ nss/ posix/ have not been covered before. Maybe we should just file a bug for the missing locking and fix this with this commit? > index 5af476c48b..c186375c31 100644 > --- a/sysdeps/nptl/libc-lock.h > +++ b/sysdeps/nptl/libc-lock.h > @@ -25,7 +25,7 @@ > > > /* Mutex type. */ > -#if defined _LIBC || defined _IO_MTSAFE_IO > +#if defined _LIBC > # if (!IS_IN (libc) && !IS_IN (libpthread)) || !defined _LIBC > typedef struct { pthread_mutex_t mutex; } __libc_lock_recursive_t; > # else This doesn't look quite right. Would we want to compile this unconditionally now? There's also some weirdness I can't explain. I get strange before/after symbol differences: DIFF eu-readelf -s after strip: nptl/pthread_rwlock_unlock.os --- /tmp/Left-ltjeu50v.o 2022-04-27 14:29:39.070599437 +0200 +++ /tmp/Right-jee20kfm.o 2022-04-27 14:29:39.074599395 +0200 @@ -1,5 +1,5 @@ -Symbol table [11] '.symtab' contains 14 entries: +Symbol table [11] '.symtab' contains 13 entries: 4 local symbols String table: [12] '.strtab' Num: Value Size Type Bind Vis Ndx Name 0: 0000000000000000 0 NOTYPE LOCAL DEFAULT UNDEF @@ -12,7 +12,6 @@ 7: 0000000000000000 463 FUNC GLOBAL DEFAULT 1 ___pthread_rwlock_unlo> 8: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UNDEF __GI___libc_fatal 9: 0000000000000000 463 FUNC GLOBAL DEFAULT 1 __GI___pthread_rwlock_> - 10: 0000000000000000 463 FUNC GLOBAL DEFAULT 1 __pthread_rwlock_unlock - 11: 0000000000000000 463 FUNC GLOBAL DEFAULT 1 __pthread_rwlock_unloc> - 12: 0000000000000000 463 FUNC GLOBAL DEFAULT 1 pthread_rwlock_unlock@> - 13: 0000000000000000 463 FUNC GLOBAL DEFAULT 1 pthread_rwlock_unlock@> + 10: 0000000000000000 463 FUNC GLOBAL DEFAULT 1 __pthread_rwlock_unloc> + 11: 0000000000000000 463 FUNC GLOBAL DEFAULT 1 pthread_rwlock_unlock@> + 12: 0000000000000000 463 FUNC GLOBAL DEFAULT 1 pthread_rwlock_unlock@> Can you reproduce this? Thanks, Florian ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/4] libio: Assume _IO_MTSAFE_IO 2022-04-27 12:34 ` Florian Weimer @ 2022-04-27 18:40 ` Adhemerval Zanella 2022-04-27 19:35 ` Adhemerval Zanella 0 siblings, 1 reply; 23+ messages in thread From: Adhemerval Zanella @ 2022-04-27 18:40 UTC (permalink / raw) To: Florian Weimer, Adhemerval Zanella via Libc-alpha On 27/04/2022 09:34, Florian Weimer wrote: > * Adhemerval Zanella via Libc-alpha: > >> It is already set by default on all supported architectures and it is >> an expectation that stdio works on multi-threaded environments. > > So … this cleanup has got stuck in the past because it's actually more > than just a cleanup. We actually build most of glibc without libio > locking. Looks like misc/ nss/ posix/ have not been covered before. > > Maybe we should just file a bug for the missing locking and fix this > with this commit? It seems a better approach, I will check which files as missing support and open a bug report. > >> index 5af476c48b..c186375c31 100644 >> --- a/sysdeps/nptl/libc-lock.h >> +++ b/sysdeps/nptl/libc-lock.h >> @@ -25,7 +25,7 @@ >> >> >> /* Mutex type. */ >> -#if defined _LIBC || defined _IO_MTSAFE_IO >> +#if defined _LIBC >> # if (!IS_IN (libc) && !IS_IN (libpthread)) || !defined _LIBC >> typedef struct { pthread_mutex_t mutex; } __libc_lock_recursive_t; >> # else > > This doesn't look quite right. Would we want to compile this > unconditionally now? Afaiu using __libc_lock_recursive without _IO_MTSAFE_IO will resulting in using __libc_lock_recursive_opaque__, which will result in a undefined type. Also, I really think we should not tie stdio code with general lock primitives (as _IO_MTSAFE_IO is doing here). > > There's also some weirdness I can't explain. I get strange before/after > symbol differences: > > DIFF eu-readelf -s after strip: nptl/pthread_rwlock_unlock.os > --- /tmp/Left-ltjeu50v.o 2022-04-27 14:29:39.070599437 +0200 > +++ /tmp/Right-jee20kfm.o 2022-04-27 14:29:39.074599395 +0200 > @@ -1,5 +1,5 @@ > > -Symbol table [11] '.symtab' contains 14 entries: > +Symbol table [11] '.symtab' contains 13 entries: > 4 local symbols String table: [12] '.strtab' > Num: Value Size Type Bind Vis Ndx Name > 0: 0000000000000000 0 NOTYPE LOCAL DEFAULT UNDEF > @@ -12,7 +12,6 @@ > 7: 0000000000000000 463 FUNC GLOBAL DEFAULT 1 ___pthread_rwlock_unlo> > 8: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UNDEF __GI___libc_fatal > 9: 0000000000000000 463 FUNC GLOBAL DEFAULT 1 __GI___pthread_rwlock_> > - 10: 0000000000000000 463 FUNC GLOBAL DEFAULT 1 __pthread_rwlock_unlock > - 11: 0000000000000000 463 FUNC GLOBAL DEFAULT 1 __pthread_rwlock_unloc> > - 12: 0000000000000000 463 FUNC GLOBAL DEFAULT 1 pthread_rwlock_unlock@> > - 13: 0000000000000000 463 FUNC GLOBAL DEFAULT 1 pthread_rwlock_unlock@> > + 10: 0000000000000000 463 FUNC GLOBAL DEFAULT 1 __pthread_rwlock_unloc> > + 11: 0000000000000000 463 FUNC GLOBAL DEFAULT 1 pthread_rwlock_unlock@> > + 12: 0000000000000000 463 FUNC GLOBAL DEFAULT 1 pthread_rwlock_unlock@> > > Can you reproduce this? I didn't check the symbol generation, I will take a look. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/4] libio: Assume _IO_MTSAFE_IO 2022-04-27 18:40 ` Adhemerval Zanella @ 2022-04-27 19:35 ` Adhemerval Zanella 0 siblings, 0 replies; 23+ messages in thread From: Adhemerval Zanella @ 2022-04-27 19:35 UTC (permalink / raw) To: Florian Weimer, Adhemerval Zanella via Libc-alpha On 27/04/2022 15:40, Adhemerval Zanella wrote: > > > On 27/04/2022 09:34, Florian Weimer wrote: >> * Adhemerval Zanella via Libc-alpha: >> >>> It is already set by default on all supported architectures and it is >>> an expectation that stdio works on multi-threaded environments. >> >> So … this cleanup has got stuck in the past because it's actually more >> than just a cleanup. We actually build most of glibc without libio >> locking. Looks like misc/ nss/ posix/ have not been covered before. >> >> Maybe we should just file a bug for the missing locking and fix this >> with this commit? > > It seems a better approach, I will check which files as missing support > and open a bug report. > >> >>> index 5af476c48b..c186375c31 100644 >>> --- a/sysdeps/nptl/libc-lock.h >>> +++ b/sysdeps/nptl/libc-lock.h >>> @@ -25,7 +25,7 @@ >>> >>> >>> /* Mutex type. */ >>> -#if defined _LIBC || defined _IO_MTSAFE_IO >>> +#if defined _LIBC >>> # if (!IS_IN (libc) && !IS_IN (libpthread)) || !defined _LIBC >>> typedef struct { pthread_mutex_t mutex; } __libc_lock_recursive_t; >>> # else >> >> This doesn't look quite right. Would we want to compile this >> unconditionally now? > > Afaiu using __libc_lock_recursive without _IO_MTSAFE_IO will resulting > in using __libc_lock_recursive_opaque__, which will result in a undefined > type. > > Also, I really think we should not tie stdio code with general lock > primitives (as _IO_MTSAFE_IO is doing here). > >> >> There's also some weirdness I can't explain. I get strange before/after >> symbol differences: >> >> DIFF eu-readelf -s after strip: nptl/pthread_rwlock_unlock.os >> --- /tmp/Left-ltjeu50v.o 2022-04-27 14:29:39.070599437 +0200 >> +++ /tmp/Right-jee20kfm.o 2022-04-27 14:29:39.074599395 +0200 >> @@ -1,5 +1,5 @@ >> >> -Symbol table [11] '.symtab' contains 14 entries: >> +Symbol table [11] '.symtab' contains 13 entries: >> 4 local symbols String table: [12] '.strtab' >> Num: Value Size Type Bind Vis Ndx Name >> 0: 0000000000000000 0 NOTYPE LOCAL DEFAULT UNDEF >> @@ -12,7 +12,6 @@ >> 7: 0000000000000000 463 FUNC GLOBAL DEFAULT 1 ___pthread_rwlock_unlo> >> 8: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UNDEF __GI___libc_fatal >> 9: 0000000000000000 463 FUNC GLOBAL DEFAULT 1 __GI___pthread_rwlock_> >> - 10: 0000000000000000 463 FUNC GLOBAL DEFAULT 1 __pthread_rwlock_unlock >> - 11: 0000000000000000 463 FUNC GLOBAL DEFAULT 1 __pthread_rwlock_unloc> >> - 12: 0000000000000000 463 FUNC GLOBAL DEFAULT 1 pthread_rwlock_unlock@> >> - 13: 0000000000000000 463 FUNC GLOBAL DEFAULT 1 pthread_rwlock_unlock@> >> + 10: 0000000000000000 463 FUNC GLOBAL DEFAULT 1 __pthread_rwlock_unloc> >> + 11: 0000000000000000 463 FUNC GLOBAL DEFAULT 1 pthread_rwlock_unlock@> >> + 12: 0000000000000000 463 FUNC GLOBAL DEFAULT 1 pthread_rwlock_unlock@> >> >> Can you reproduce this? > > I didn't check the symbol generation, I will take a look. I think it might be from: diff --git a/include/stdio.h b/include/stdio.h index 23b7fd288c..e9a7c77c24 100644 --- a/include/stdio.h +++ b/include/stdio.h @@ -1,5 +1,5 @@ #ifndef _STDIO_H -# if !defined _ISOMAC && defined _IO_MTSAFE_IO +# if !defined _ISOMAC # include <stdio-lock.h> # endif Where tt now pulls libc-lockP.h from from libc-lock.h. And libc-lockP.h contains a lot of duplicated definition already preseted on pthreadP.h, and worse, it it defined some hidden_proto not presented in pthreadP.h. I will send a patch to consolidate this definitions prior the one that removes _IO_MTSAFE_IO. ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 2/4] Consolidate stdio-lock.h 2022-04-26 19:15 [PATCH v2 0/4] Move libio lock single-thread optimization to generic libc-lock Adhemerval Zanella 2022-04-26 19:15 ` [PATCH v2 1/4] libio: Assume _IO_MTSAFE_IO Adhemerval Zanella @ 2022-04-26 19:15 ` Adhemerval Zanella 2022-04-27 13:25 ` Florian Weimer 2022-04-26 19:15 ` [PATCH v2 3/4] Move libio lock single-thread optimization to generic libc-lock (BZ #27842) Adhemerval Zanella 2022-04-26 19:15 ` [PATCH v2 4/4] Assume _LIBC and libc module for libc-lock.h Adhemerval Zanella 3 siblings, 1 reply; 23+ messages in thread From: Adhemerval Zanella @ 2022-04-26 19:15 UTC (permalink / raw) To: libc-alpha, Florian Weimer The NPTL replicate the sysdeps/nptl/libc-lock.h recursive lock. Use the Hurd one as generic one, which already uses the __libc_lock functions. The libc_malloc_debug is the only module that uses the __libc_lock_init_recursive directly in the code and it is kept to use the internal lock definitions. Checked on x86_64-linux-gnu and i686-linux-gnu. --- sysdeps/generic/stdio-lock.h | 21 +++----- sysdeps/htl/stdio-lock.h | 57 -------------------- sysdeps/nptl/libc-lock.h | 20 +++---- sysdeps/nptl/stdio-lock.h | 101 ----------------------------------- 4 files changed, 15 insertions(+), 184 deletions(-) delete mode 100644 sysdeps/htl/stdio-lock.h delete mode 100644 sysdeps/nptl/stdio-lock.h diff --git a/sysdeps/generic/stdio-lock.h b/sysdeps/generic/stdio-lock.h index 14cf458bdd..fd61f0b5b7 100644 --- a/sysdeps/generic/stdio-lock.h +++ b/sysdeps/generic/stdio-lock.h @@ -45,20 +45,13 @@ __libc_lock_define_recursive (typedef, _IO_lock_t) #define _IO_cleanup_region_end(_doit) \ __libc_cleanup_region_end (_doit) -#if defined _LIBC && IS_IN (libc) - -# ifdef __EXCEPTIONS -# define _IO_acquire_lock(_fp) \ +#define _IO_acquire_lock(_fp) \ do { \ - FILE *_IO_acquire_lock_file \ - __attribute__((cleanup (_IO_acquire_lock_fct))) \ - = (_fp); \ - _IO_flockfile (_IO_acquire_lock_file); -# else -# define _IO_acquire_lock(_fp) _IO_acquire_lock_needs_exceptions_enabled -# endif -# define _IO_release_lock(_fp) ; } while (0) - -#endif + _IO_cleanup_region_start((void (*) (void *)) &_IO_funlockfile, _fp); \ + _IO_flockfile (_fp); +#define _IO_release_lock(_fp) \ + _IO_funlockfile (_fp); \ + _IO_cleanup_region_end (0); \ + } while (0) #endif /* stdio-lock.h */ diff --git a/sysdeps/htl/stdio-lock.h b/sysdeps/htl/stdio-lock.h deleted file mode 100644 index c0a03c5a13..0000000000 --- a/sysdeps/htl/stdio-lock.h +++ /dev/null @@ -1,57 +0,0 @@ -/* Thread package specific definitions of stream lock type. Hurd version. - Copyright (C) 2000-2022 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 - <https://www.gnu.org/licenses/>. */ - -#ifndef _STDIO_LOCK_H -#define _STDIO_LOCK_H 1 - -#include <libc-lock.h> - -__libc_lock_define_recursive (typedef, _IO_lock_t) -#define _IO_lock_t_defined 1 - -/* We need recursive (counting) mutexes. */ -# define _IO_lock_initializer _LIBC_LOCK_RECURSIVE_INITIALIZER - -#define _IO_lock_init(_name) __libc_lock_init_recursive (_name) -#define _IO_lock_fini(_name) __libc_lock_fini_recursive (_name) -#define _IO_lock_lock(_name) __libc_lock_lock_recursive (_name) -#define _IO_lock_trylock(_name) __libc_lock_trylock_recursive (_name) -#define _IO_lock_unlock(_name) __libc_lock_unlock_recursive (_name) - - -#define _IO_cleanup_region_start(_fct, _fp) \ - __libc_cleanup_region_start (((_fp)->_flags & _IO_USER_LOCK) == 0, _fct, _fp) -#define _IO_cleanup_region_start_noarg(_fct) \ - __libc_cleanup_region_start (1, _fct, NULL) -#define _IO_cleanup_region_end(_doit) \ - __libc_cleanup_region_end (_doit) - -#if defined _LIBC && IS_IN (libc) - -# define _IO_acquire_lock(_fp) \ - do { \ - _IO_cleanup_region_start((void (*) (void *)) &_IO_funlockfile, _fp); \ - _IO_flockfile (_fp); -# define _IO_release_lock(_fp) \ - _IO_funlockfile (_fp); \ - _IO_cleanup_region_end (0); \ - } while (0) - -#endif - -#endif /* stdio-lock.h */ diff --git a/sysdeps/nptl/libc-lock.h b/sysdeps/nptl/libc-lock.h index c186375c31..6c2d6acfd1 100644 --- a/sysdeps/nptl/libc-lock.h +++ b/sysdeps/nptl/libc-lock.h @@ -25,14 +25,10 @@ /* Mutex type. */ -#if defined _LIBC -# if (!IS_IN (libc) && !IS_IN (libpthread)) || !defined _LIBC +#if !IS_IN (libc) && !IS_IN (libc_malloc_debug) typedef struct { pthread_mutex_t mutex; } __libc_lock_recursive_t; -# else -typedef struct { int lock; int cnt; void *owner; } __libc_lock_recursive_t; -# endif #else -typedef struct __libc_lock_recursive_opaque__ __libc_lock_recursive_t; +typedef struct { int lock; int cnt; void *owner; } __libc_lock_recursive_t; #endif /* Define a lock variable NAME with storage class CLASS. The lock must be @@ -47,7 +43,7 @@ typedef struct __libc_lock_recursive_opaque__ __libc_lock_recursive_t; /* Define an initialized recursive lock variable NAME with storage class CLASS. */ -#if defined _LIBC && (IS_IN (libc) || IS_IN (libpthread)) +#if IS_IN (libc) || IS_IN (libc_malloc_debug) # define __libc_lock_define_initialized_recursive(CLASS, NAME) \ CLASS __libc_lock_recursive_t NAME = _LIBC_LOCK_RECURSIVE_INITIALIZER; # define _LIBC_LOCK_RECURSIVE_INITIALIZER \ @@ -60,7 +56,7 @@ typedef struct __libc_lock_recursive_opaque__ __libc_lock_recursive_t; #endif /* Initialize a recursive mutex. */ -#if defined _LIBC && (IS_IN (libc) || IS_IN (libpthread)) +#if IS_IN (libc) || IS_IN (libc_malloc_debug) # define __libc_lock_init_recursive(NAME) \ ((void) ((NAME) = (__libc_lock_recursive_t) _LIBC_LOCK_RECURSIVE_INITIALIZER)) #else @@ -78,7 +74,7 @@ typedef struct __libc_lock_recursive_opaque__ __libc_lock_recursive_t; #endif /* Finalize recursive named lock. */ -#if defined _LIBC && (IS_IN (libc) || IS_IN (libpthread)) +#if IS_IN (libc) || IS_IN (libc_malloc_debug) # define __libc_lock_fini_recursive(NAME) ((void) 0) #else # define __libc_lock_fini_recursive(NAME) \ @@ -86,7 +82,7 @@ typedef struct __libc_lock_recursive_opaque__ __libc_lock_recursive_t; #endif /* Lock the recursive named lock variable. */ -#if defined _LIBC && (IS_IN (libc) || IS_IN (libpthread)) +#if IS_IN (libc) || IS_IN (libc_malloc_debug) # define __libc_lock_lock_recursive(NAME) \ do { \ void *self = THREAD_SELF; \ @@ -103,7 +99,7 @@ typedef struct __libc_lock_recursive_opaque__ __libc_lock_recursive_t; #endif /* Try to lock the recursive named lock variable. */ -#if defined _LIBC && (IS_IN (libc) || IS_IN (libpthread)) +#if IS_IN (libc) || IS_IN (libc_malloc_debug) # define __libc_lock_trylock_recursive(NAME) \ ({ \ int result = 0; \ @@ -128,7 +124,7 @@ typedef struct __libc_lock_recursive_opaque__ __libc_lock_recursive_t; #endif /* Unlock the recursive named lock variable. */ -#if defined _LIBC && (IS_IN (libc) || IS_IN (libpthread)) +#if IS_IN (libc) || IS_IN (libc_malloc_debug) /* We do no error checking here. */ # define __libc_lock_unlock_recursive(NAME) \ do { \ diff --git a/sysdeps/nptl/stdio-lock.h b/sysdeps/nptl/stdio-lock.h deleted file mode 100644 index afa0b779c8..0000000000 --- a/sysdeps/nptl/stdio-lock.h +++ /dev/null @@ -1,101 +0,0 @@ -/* Thread package specific definitions of stream lock type. NPTL version. - Copyright (C) 2000-2022 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 - <https://www.gnu.org/licenses/>. */ - -#ifndef _STDIO_LOCK_H -#define _STDIO_LOCK_H 1 - -#include <libc-lock.h> -#include <lowlevellock.h> - - -typedef struct { int lock; int cnt; void *owner; } _IO_lock_t; -#define _IO_lock_t_defined 1 - -#define _IO_lock_initializer { LLL_LOCK_INITIALIZER, 0, NULL } - -#define _IO_lock_init(_name) \ - ((void) ((_name) = (_IO_lock_t) _IO_lock_initializer)) - -#define _IO_lock_fini(_name) \ - ((void) 0) - -#define _IO_lock_lock(_name) \ - do { \ - void *__self = THREAD_SELF; \ - if ((_name).owner != __self) \ - { \ - lll_lock ((_name).lock, LLL_PRIVATE); \ - (_name).owner = __self; \ - } \ - ++(_name).cnt; \ - } while (0) - -#define _IO_lock_trylock(_name) \ - ({ \ - int __result = 0; \ - void *__self = THREAD_SELF; \ - if ((_name).owner != __self) \ - { \ - if (lll_trylock ((_name).lock) == 0) \ - { \ - (_name).owner = __self; \ - (_name).cnt = 1; \ - } \ - else \ - __result = EBUSY; \ - } \ - else \ - ++(_name).cnt; \ - __result; \ - }) - -#define _IO_lock_unlock(_name) \ - do { \ - if (--(_name).cnt == 0) \ - { \ - (_name).owner = NULL; \ - lll_unlock ((_name).lock, LLL_PRIVATE); \ - } \ - } while (0) - - - -#define _IO_cleanup_region_start(_fct, _fp) \ - __libc_cleanup_region_start (((_fp)->_flags & _IO_USER_LOCK) == 0, _fct, _fp) -#define _IO_cleanup_region_start_noarg(_fct) \ - __libc_cleanup_region_start (1, _fct, NULL) -#define _IO_cleanup_region_end(_doit) \ - __libc_cleanup_region_end (_doit) - -#if defined _LIBC && IS_IN (libc) - -# ifdef __EXCEPTIONS -# define _IO_acquire_lock(_fp) \ - do { \ - FILE *_IO_acquire_lock_file \ - __attribute__((cleanup (_IO_acquire_lock_fct))) \ - = (_fp); \ - _IO_flockfile (_IO_acquire_lock_file); -# else -# define _IO_acquire_lock(_fp) _IO_acquire_lock_needs_exceptions_enabled -# endif -# define _IO_release_lock(_fp) ; } while (0) - -#endif - -#endif /* stdio-lock.h */ -- 2.34.1 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 2/4] Consolidate stdio-lock.h 2022-04-26 19:15 ` [PATCH v2 2/4] Consolidate stdio-lock.h Adhemerval Zanella @ 2022-04-27 13:25 ` Florian Weimer 2022-04-27 16:15 ` Adhemerval Zanella 0 siblings, 1 reply; 23+ messages in thread From: Florian Weimer @ 2022-04-27 13:25 UTC (permalink / raw) To: Adhemerval Zanella via Libc-alpha * Adhemerval Zanella via Libc-alpha: > diff --git a/sysdeps/generic/stdio-lock.h b/sysdeps/generic/stdio-lock.h > index 14cf458bdd..fd61f0b5b7 100644 > --- a/sysdeps/generic/stdio-lock.h > +++ b/sysdeps/generic/stdio-lock.h > @@ -45,20 +45,13 @@ __libc_lock_define_recursive (typedef, _IO_lock_t) > #define _IO_cleanup_region_end(_doit) \ > __libc_cleanup_region_end (_doit) > > -#if defined _LIBC && IS_IN (libc) > - > -# ifdef __EXCEPTIONS > -# define _IO_acquire_lock(_fp) \ > +#define _IO_acquire_lock(_fp) \ > do { \ > - FILE *_IO_acquire_lock_file \ > - __attribute__((cleanup (_IO_acquire_lock_fct))) \ > - = (_fp); \ > - _IO_flockfile (_IO_acquire_lock_file); > -# else > -# define _IO_acquire_lock(_fp) _IO_acquire_lock_needs_exceptions_enabled > -# endif > -# define _IO_release_lock(_fp) ; } while (0) > - > -#endif > + _IO_cleanup_region_start((void (*) (void *)) &_IO_funlockfile, _fp); \ > + _IO_flockfile (_fp); > +#define _IO_release_lock(_fp) \ > + _IO_funlockfile (_fp); \ > + _IO_cleanup_region_end (0); \ > + } while (0) > > #endif /* stdio-lock.h */ I think this change replaces unwind tables for -fexceptions builds. If GCC can't turn the indirect call to the unlock function into a direct call, this will result in a loss of hardening due to the additional indirect function call. This change may also lose C++ unwinding compatibility for some fopencookie use cases, I think. Thanks, Florian ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 2/4] Consolidate stdio-lock.h 2022-04-27 13:25 ` Florian Weimer @ 2022-04-27 16:15 ` Adhemerval Zanella 2022-04-27 18:00 ` Florian Weimer 0 siblings, 1 reply; 23+ messages in thread From: Adhemerval Zanella @ 2022-04-27 16:15 UTC (permalink / raw) To: Florian Weimer, Adhemerval Zanella via Libc-alpha On 27/04/2022 10:25, Florian Weimer wrote: > * Adhemerval Zanella via Libc-alpha: > >> diff --git a/sysdeps/generic/stdio-lock.h b/sysdeps/generic/stdio-lock.h >> index 14cf458bdd..fd61f0b5b7 100644 >> --- a/sysdeps/generic/stdio-lock.h >> +++ b/sysdeps/generic/stdio-lock.h >> @@ -45,20 +45,13 @@ __libc_lock_define_recursive (typedef, _IO_lock_t) >> #define _IO_cleanup_region_end(_doit) \ >> __libc_cleanup_region_end (_doit) >> >> -#if defined _LIBC && IS_IN (libc) >> - >> -# ifdef __EXCEPTIONS >> -# define _IO_acquire_lock(_fp) \ >> +#define _IO_acquire_lock(_fp) \ >> do { \ >> - FILE *_IO_acquire_lock_file \ >> - __attribute__((cleanup (_IO_acquire_lock_fct))) \ >> - = (_fp); \ >> - _IO_flockfile (_IO_acquire_lock_file); >> -# else >> -# define _IO_acquire_lock(_fp) _IO_acquire_lock_needs_exceptions_enabled >> -# endif >> -# define _IO_release_lock(_fp) ; } while (0) >> - >> -#endif >> + _IO_cleanup_region_start((void (*) (void *)) &_IO_funlockfile, _fp); \ >> + _IO_flockfile (_fp); >> +#define _IO_release_lock(_fp) \ >> + _IO_funlockfile (_fp); \ >> + _IO_cleanup_region_end (0); \ >> + } while (0) >> >> #endif /* stdio-lock.h */ > > I think this change replaces unwind tables for -fexceptions builds. If > GCC can't turn the indirect call to the unlock function into a direct > call, this will result in a loss of hardening due to the additional > indirect function call. > > This change may also lose C++ unwinding compatibility for some > fopencookie use cases, I think. This is an internal header where if __EXCEPTIONS is not defined we will get a compiler error because of an undefined symbol (_IO_acquire_lock_needs_exceptions_enabled). So internally all _IO_acquire_lock usage already requires __EXCEPTIONS, so the fallback is just unused definitions. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 2/4] Consolidate stdio-lock.h 2022-04-27 16:15 ` Adhemerval Zanella @ 2022-04-27 18:00 ` Florian Weimer 2022-04-27 18:35 ` Adhemerval Zanella 0 siblings, 1 reply; 23+ messages in thread From: Florian Weimer @ 2022-04-27 18:00 UTC (permalink / raw) To: Adhemerval Zanella; +Cc: Adhemerval Zanella via Libc-alpha * Adhemerval Zanella: > On 27/04/2022 10:25, Florian Weimer wrote: >> * Adhemerval Zanella via Libc-alpha: >> >>> diff --git a/sysdeps/generic/stdio-lock.h b/sysdeps/generic/stdio-lock.h >>> index 14cf458bdd..fd61f0b5b7 100644 >>> --- a/sysdeps/generic/stdio-lock.h >>> +++ b/sysdeps/generic/stdio-lock.h >>> @@ -45,20 +45,13 @@ __libc_lock_define_recursive (typedef, _IO_lock_t) >>> #define _IO_cleanup_region_end(_doit) \ >>> __libc_cleanup_region_end (_doit) >>> >>> -#if defined _LIBC && IS_IN (libc) >>> - >>> -# ifdef __EXCEPTIONS >>> -# define _IO_acquire_lock(_fp) \ >>> +#define _IO_acquire_lock(_fp) \ >>> do { \ >>> - FILE *_IO_acquire_lock_file \ >>> - __attribute__((cleanup (_IO_acquire_lock_fct))) \ >>> - = (_fp); \ >>> - _IO_flockfile (_IO_acquire_lock_file); >>> -# else >>> -# define _IO_acquire_lock(_fp) _IO_acquire_lock_needs_exceptions_enabled >>> -# endif >>> -# define _IO_release_lock(_fp) ; } while (0) >>> - >>> -#endif >>> + _IO_cleanup_region_start((void (*) (void *)) &_IO_funlockfile, _fp); \ >>> + _IO_flockfile (_fp); >>> +#define _IO_release_lock(_fp) \ >>> + _IO_funlockfile (_fp); \ >>> + _IO_cleanup_region_end (0); \ >>> + } while (0) >>> >>> #endif /* stdio-lock.h */ >> >> I think this change replaces unwind tables for -fexceptions builds. If >> GCC can't turn the indirect call to the unlock function into a direct >> call, this will result in a loss of hardening due to the additional >> indirect function call. >> >> This change may also lose C++ unwinding compatibility for some >> fopencookie use cases, I think. > > This is an internal header where if __EXCEPTIONS is not defined we will > get a compiler error because of an undefined symbol > (_IO_acquire_lock_needs_exceptions_enabled). So internally all > _IO_acquire_lock usage already requires __EXCEPTIONS, so the fallback > is just unused definitions. I see this code generation change in libio/fputc.os. The new code uses an on-stack pointer saved at the start of the cleanup region: + 90: 48 8b 05 00 00 00 00 mov 0x0(%rip),%rax # 97 <fputc+0x97> + 93: R_X86_64_REX_GOTPCRELX _IO_funlockfile-0x4 + 97: 48 89 e7 mov %rsp,%rdi + 9a: 48 89 04 24 mov %rax,(%rsp) + 9e: e8 00 00 00 00 call a3 <fputc+0xa3> + 9f: R_X86_64_PLT32 __GI___libc_cleanup_push_defer-0x4 + a3: 8b 45 00 mov 0x0(%rbp),%eax + a6: 25 00 80 00 00 and $0x8000,%eax + ab: 0f 85 d1 00 00 00 jne 182 <fputc+0x182> + b1: 64 4c 8b 2c 25 10 00 mov %fs:0x10,%r13 + b8: 00 00 + ba: 48 8b bd 88 00 00 00 mov 0x88(%rbp),%rdi + c1: 4c 39 6f 08 cmp %r13,0x8(%rdi) + c5: 74 1a je e1 <fputc+0xe1> + c7: ba 01 00 00 00 mov $0x1,%edx + cc: f0 0f b1 17 lock cmpxchg %edx,(%rdi) + d0: 0f 85 a2 00 00 00 jne 178 <fputc+0x178> + d6: 48 8b bd 88 00 00 00 mov 0x88(%rbp),%rdi + dd: 4c 89 6f 08 mov %r13,0x8(%rdi) + e1: 83 47 04 01 addl $0x1,0x4(%rdi) + e5: 41 bd 01 00 00 00 mov $0x1,%r13d + eb: e9 3d ff ff ff jmp 2d <fputc+0x2d> + f0: 48 89 e7 mov %rsp,%rdi + f3: e8 00 00 00 00 call f8 <fputc+0xf8> + f4: R_X86_64_PLT32 __GI___libc_cleanup_pop_restore-0x4 This is a from a build with CFLAGS="-O2 -fexceptions -s -DNDEBUG" (for comparison purposes). The old code just inlined the _IO_funlockfile fast path. (We seem to lack libc_hidden_proto/libc_hidden_def for _IO_funlockfile.) Thanks, Florian ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 2/4] Consolidate stdio-lock.h 2022-04-27 18:00 ` Florian Weimer @ 2022-04-27 18:35 ` Adhemerval Zanella 2022-04-27 18:44 ` Florian Weimer 0 siblings, 1 reply; 23+ messages in thread From: Adhemerval Zanella @ 2022-04-27 18:35 UTC (permalink / raw) To: Florian Weimer; +Cc: Adhemerval Zanella via Libc-alpha On 27/04/2022 15:00, Florian Weimer wrote: > * Adhemerval Zanella: > >> On 27/04/2022 10:25, Florian Weimer wrote: >>> * Adhemerval Zanella via Libc-alpha: >>> >>>> diff --git a/sysdeps/generic/stdio-lock.h b/sysdeps/generic/stdio-lock.h >>>> index 14cf458bdd..fd61f0b5b7 100644 >>>> --- a/sysdeps/generic/stdio-lock.h >>>> +++ b/sysdeps/generic/stdio-lock.h >>>> @@ -45,20 +45,13 @@ __libc_lock_define_recursive (typedef, _IO_lock_t) >>>> #define _IO_cleanup_region_end(_doit) \ >>>> __libc_cleanup_region_end (_doit) >>>> >>>> -#if defined _LIBC && IS_IN (libc) >>>> - >>>> -# ifdef __EXCEPTIONS >>>> -# define _IO_acquire_lock(_fp) \ >>>> +#define _IO_acquire_lock(_fp) \ >>>> do { \ >>>> - FILE *_IO_acquire_lock_file \ >>>> - __attribute__((cleanup (_IO_acquire_lock_fct))) \ >>>> - = (_fp); \ >>>> - _IO_flockfile (_IO_acquire_lock_file); >>>> -# else >>>> -# define _IO_acquire_lock(_fp) _IO_acquire_lock_needs_exceptions_enabled >>>> -# endif >>>> -# define _IO_release_lock(_fp) ; } while (0) >>>> - >>>> -#endif >>>> + _IO_cleanup_region_start((void (*) (void *)) &_IO_funlockfile, _fp); \ >>>> + _IO_flockfile (_fp); >>>> +#define _IO_release_lock(_fp) \ >>>> + _IO_funlockfile (_fp); \ >>>> + _IO_cleanup_region_end (0); \ >>>> + } while (0) >>>> >>>> #endif /* stdio-lock.h */ >>> >>> I think this change replaces unwind tables for -fexceptions builds. If >>> GCC can't turn the indirect call to the unlock function into a direct >>> call, this will result in a loss of hardening due to the additional >>> indirect function call. >>> >>> This change may also lose C++ unwinding compatibility for some >>> fopencookie use cases, I think. >> >> This is an internal header where if __EXCEPTIONS is not defined we will >> get a compiler error because of an undefined symbol >> (_IO_acquire_lock_needs_exceptions_enabled). So internally all >> _IO_acquire_lock usage already requires __EXCEPTIONS, so the fallback >> is just unused definitions. > > I see this code generation change in libio/fputc.os. The new code uses > an on-stack pointer saved at the start of the cleanup region: > > + 90: 48 8b 05 00 00 00 00 mov 0x0(%rip),%rax # 97 <fputc+0x97> > + 93: R_X86_64_REX_GOTPCRELX _IO_funlockfile-0x4 > + 97: 48 89 e7 mov %rsp,%rdi > + 9a: 48 89 04 24 mov %rax,(%rsp) > + 9e: e8 00 00 00 00 call a3 <fputc+0xa3> > + 9f: R_X86_64_PLT32 __GI___libc_cleanup_push_defer-0x4 > + a3: 8b 45 00 mov 0x0(%rbp),%eax > + a6: 25 00 80 00 00 and $0x8000,%eax > + ab: 0f 85 d1 00 00 00 jne 182 <fputc+0x182> > + b1: 64 4c 8b 2c 25 10 00 mov %fs:0x10,%r13 > + b8: 00 00 > + ba: 48 8b bd 88 00 00 00 mov 0x88(%rbp),%rdi > + c1: 4c 39 6f 08 cmp %r13,0x8(%rdi) > + c5: 74 1a je e1 <fputc+0xe1> > + c7: ba 01 00 00 00 mov $0x1,%edx > + cc: f0 0f b1 17 lock cmpxchg %edx,(%rdi) > + d0: 0f 85 a2 00 00 00 jne 178 <fputc+0x178> > + d6: 48 8b bd 88 00 00 00 mov 0x88(%rbp),%rdi > + dd: 4c 89 6f 08 mov %r13,0x8(%rdi) > + e1: 83 47 04 01 addl $0x1,0x4(%rdi) > + e5: 41 bd 01 00 00 00 mov $0x1,%r13d > + eb: e9 3d ff ff ff jmp 2d <fputc+0x2d> > + f0: 48 89 e7 mov %rsp,%rdi > + f3: e8 00 00 00 00 call f8 <fputc+0xf8> > + f4: R_X86_64_PLT32 __GI___libc_cleanup_pop_restore-0x4 > > This is a from a build with CFLAGS="-O2 -fexceptions -s -DNDEBUG" > (for comparison purposes). > > The old code just inlined the _IO_funlockfile fast path. > > (We seem to lack libc_hidden_proto/libc_hidden_def for _IO_funlockfile.) You are right. The change now uses __libc_cleanup_region_start from libc-lock.h instead of the cleanup version. I am not sure about hardening, but afaiu __libc_cleanup_push_defer should handle C++ unwinding for fopencookie. It seems that the Hurd version is indeed to the best option, so I think for generic it would be better to just use: diff --git a/sysdeps/generic/stdio-lock.h b/sysdeps/generic/stdio-lock.h index 14cf458bdd..a42131f5a5 100644 --- a/sysdeps/generic/stdio-lock.h +++ b/sysdeps/generic/stdio-lock.h @@ -45,20 +45,12 @@ __libc_lock_define_recursive (typedef, _IO_lock_t) #define _IO_cleanup_region_end(_doit) \ __libc_cleanup_region_end (_doit) -#if defined _LIBC && IS_IN (libc) - -# ifdef __EXCEPTIONS -# define _IO_acquire_lock(_fp) \ +#define _IO_acquire_lock(_fp) \ do { \ FILE *_IO_acquire_lock_file \ __attribute__((cleanup (_IO_acquire_lock_fct))) \ = (_fp); \ _IO_flockfile (_IO_acquire_lock_file); -# else -# define _IO_acquire_lock(_fp) _IO_acquire_lock_needs_exceptions_enabled -# endif -# define _IO_release_lock(_fp) ; } while (0) - -#endif +#define _IO_release_lock(_fp) ; } while (0) #endif /* stdio-lock.h */ ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 2/4] Consolidate stdio-lock.h 2022-04-27 18:35 ` Adhemerval Zanella @ 2022-04-27 18:44 ` Florian Weimer 2022-04-27 18:49 ` Adhemerval Zanella 0 siblings, 1 reply; 23+ messages in thread From: Florian Weimer @ 2022-04-27 18:44 UTC (permalink / raw) To: Adhemerval Zanella; +Cc: Adhemerval Zanella via Libc-alpha * Adhemerval Zanella: > You are right. The change now uses __libc_cleanup_region_start from > libc-lock.h instead of the cleanup version. I am not sure about > hardening, but afaiu __libc_cleanup_push_defer should handle C++ > unwinding for fopencookie. No, C++ unwinding will right go through these frames. We only interleave in the other direction (pthread_cancel unwinding running C++ destructors). > It seems that the Hurd version is indeed to the best option, so I think for > generic it would be better to just use: > > diff --git a/sysdeps/generic/stdio-lock.h b/sysdeps/generic/stdio-lock.h > index 14cf458bdd..a42131f5a5 100644 > --- a/sysdeps/generic/stdio-lock.h > +++ b/sysdeps/generic/stdio-lock.h > @@ -45,20 +45,12 @@ __libc_lock_define_recursive (typedef, _IO_lock_t) > #define _IO_cleanup_region_end(_doit) \ > __libc_cleanup_region_end (_doit) > > -#if defined _LIBC && IS_IN (libc) > - > -# ifdef __EXCEPTIONS > -# define _IO_acquire_lock(_fp) \ > +#define _IO_acquire_lock(_fp) \ > do { \ > FILE *_IO_acquire_lock_file \ > __attribute__((cleanup (_IO_acquire_lock_fct))) \ > = (_fp); \ > _IO_flockfile (_IO_acquire_lock_file); > -# else > -# define _IO_acquire_lock(_fp) _IO_acquire_lock_needs_exceptions_enabled > -# endif > -# define _IO_release_lock(_fp) ; } while (0) > - > -#endif > +#define _IO_release_lock(_fp) ; } while (0) > > #endif /* stdio-lock.h */ I suppose so. Thanks, Florian ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 2/4] Consolidate stdio-lock.h 2022-04-27 18:44 ` Florian Weimer @ 2022-04-27 18:49 ` Adhemerval Zanella 0 siblings, 0 replies; 23+ messages in thread From: Adhemerval Zanella @ 2022-04-27 18:49 UTC (permalink / raw) To: Florian Weimer; +Cc: Adhemerval Zanella via Libc-alpha On 27/04/2022 15:44, Florian Weimer wrote: > * Adhemerval Zanella: > >> You are right. The change now uses __libc_cleanup_region_start from >> libc-lock.h instead of the cleanup version. I am not sure about >> hardening, but afaiu __libc_cleanup_push_defer should handle C++ >> unwinding for fopencookie. > > No, C++ unwinding will right go through these frames. We only > interleave in the other direction (pthread_cancel unwinding running C++ > destructors). Right, so Hurd version is indeed broken. > >> It seems that the Hurd version is indeed to the best option, so I think for >> generic it would be better to just use: >> >> diff --git a/sysdeps/generic/stdio-lock.h b/sysdeps/generic/stdio-lock.h >> index 14cf458bdd..a42131f5a5 100644 >> --- a/sysdeps/generic/stdio-lock.h >> +++ b/sysdeps/generic/stdio-lock.h >> @@ -45,20 +45,12 @@ __libc_lock_define_recursive (typedef, _IO_lock_t) >> #define _IO_cleanup_region_end(_doit) \ >> __libc_cleanup_region_end (_doit) >> >> -#if defined _LIBC && IS_IN (libc) >> - >> -# ifdef __EXCEPTIONS >> -# define _IO_acquire_lock(_fp) \ >> +#define _IO_acquire_lock(_fp) \ >> do { \ >> FILE *_IO_acquire_lock_file \ >> __attribute__((cleanup (_IO_acquire_lock_fct))) \ >> = (_fp); \ >> _IO_flockfile (_IO_acquire_lock_file); >> -# else >> -# define _IO_acquire_lock(_fp) _IO_acquire_lock_needs_exceptions_enabled >> -# endif >> -# define _IO_release_lock(_fp) ; } while (0) >> - >> -#endif >> +#define _IO_release_lock(_fp) ; } while (0) >> >> #endif /* stdio-lock.h */ > > I suppose so. > I will update the patch. ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 3/4] Move libio lock single-thread optimization to generic libc-lock (BZ #27842) 2022-04-26 19:15 [PATCH v2 0/4] Move libio lock single-thread optimization to generic libc-lock Adhemerval Zanella 2022-04-26 19:15 ` [PATCH v2 1/4] libio: Assume _IO_MTSAFE_IO Adhemerval Zanella 2022-04-26 19:15 ` [PATCH v2 2/4] Consolidate stdio-lock.h Adhemerval Zanella @ 2022-04-26 19:15 ` Adhemerval Zanella 2022-04-27 13:30 ` Florian Weimer 2022-04-26 19:15 ` [PATCH v2 4/4] Assume _LIBC and libc module for libc-lock.h Adhemerval Zanella 3 siblings, 1 reply; 23+ messages in thread From: Adhemerval Zanella @ 2022-04-26 19:15 UTC (permalink / raw) To: libc-alpha, Florian Weimer This patch moves the single thread stdio optimization (d2e04918833d9) to the libc-lock. With generic support there is no need to add per-function code to handle the single-thread case, and it allows to removes the _IO_enable_locks (since once process goes multithread the locks will be always used). It also handles the memory streams requirement (de895ddcd7fc), since the SINGLE_THREAD_P already contains te information whether the process is multithread (so there is no need to disable the optimization because such stream are listed in _IO_list_all). Finally it also removed the flockfile uses a read-modify-write operation on _flags2 outside a lock region (BZ #27842). Checked on x86_64-linux-gnu, i686-linux-gnu, and aarch64-linux-gnu. --- libio/Versions | 3 --- libio/feof.c | 2 -- libio/ferror.c | 2 -- libio/fputc.c | 2 -- libio/genops.c | 28 ---------------------------- libio/getc.c | 2 -- libio/getchar.c | 4 +--- libio/iofopncook.c | 2 -- libio/ioungetc.c | 2 -- libio/libio.h | 4 ---- libio/memstream.c | 3 --- libio/putc.c | 2 -- libio/wmemstream.c | 3 --- nptl/pthread_create.c | 3 --- stdio-common/flockfile.c | 1 - sysdeps/mach/libc-lock.h | 9 +++++---- sysdeps/nptl/libc-lock.h | 7 ++++--- 17 files changed, 10 insertions(+), 69 deletions(-) diff --git a/libio/Versions b/libio/Versions index b91a7bc914..b528fae670 100644 --- a/libio/Versions +++ b/libio/Versions @@ -159,9 +159,6 @@ libc { # Used by NPTL and librt __libc_fatal; - # Used by NPTL - _IO_enable_locks; - __fseeko64; __ftello64; } diff --git a/libio/feof.c b/libio/feof.c index 7f79b2795e..41e7beeca7 100644 --- a/libio/feof.c +++ b/libio/feof.c @@ -32,8 +32,6 @@ _IO_feof (FILE *fp) { int result; CHECK_FILE (fp, EOF); - if (!_IO_need_lock (fp)) - return _IO_feof_unlocked (fp); _IO_flockfile (fp); result = _IO_feof_unlocked (fp); _IO_funlockfile (fp); diff --git a/libio/ferror.c b/libio/ferror.c index d2489c54d3..a60efc3f0f 100644 --- a/libio/ferror.c +++ b/libio/ferror.c @@ -32,8 +32,6 @@ _IO_ferror (FILE *fp) { int result; CHECK_FILE (fp, EOF); - if (!_IO_need_lock (fp)) - return _IO_ferror_unlocked (fp); _IO_flockfile (fp); result = _IO_ferror_unlocked (fp); _IO_funlockfile (fp); diff --git a/libio/fputc.c b/libio/fputc.c index d29a9d0aee..c3d66500d1 100644 --- a/libio/fputc.c +++ b/libio/fputc.c @@ -32,8 +32,6 @@ fputc (int c, FILE *fp) { int result; CHECK_FILE (fp, EOF); - if (!_IO_need_lock (fp)) - return _IO_putc_unlocked (c, fp); _IO_acquire_lock (fp); result = _IO_putc_unlocked (c, fp); _IO_release_lock (fp); diff --git a/libio/genops.c b/libio/genops.c index ccfbcd149d..eca50bacbc 100644 --- a/libio/genops.c +++ b/libio/genops.c @@ -488,39 +488,11 @@ _IO_init (FILE *fp, int flags) _IO_init_internal (fp, flags); } -static int stdio_needs_locking; - -/* In a single-threaded process most stdio locks can be omitted. After - _IO_enable_locks is called, locks are not optimized away any more. - It must be first called while the process is still single-threaded. - - This lock optimization can be disabled on a per-file basis by setting - _IO_FLAGS2_NEED_LOCK, because a file can have user-defined callbacks - or can be locked with flockfile and then a thread may be created - between a lock and unlock, so omitting the lock is not valid. - - Here we have to make sure that the flag is set on all existing files - and files created later. */ -void -_IO_enable_locks (void) -{ - _IO_ITER i; - - if (stdio_needs_locking) - return; - stdio_needs_locking = 1; - for (i = _IO_iter_begin (); i != _IO_iter_end (); i = _IO_iter_next (i)) - _IO_iter_file (i)->_flags2 |= _IO_FLAGS2_NEED_LOCK; -} -libc_hidden_def (_IO_enable_locks) - void _IO_old_init (FILE *fp, int flags) { fp->_flags = _IO_MAGIC|flags; fp->_flags2 = 0; - if (stdio_needs_locking) - fp->_flags2 |= _IO_FLAGS2_NEED_LOCK; fp->_IO_buf_base = NULL; fp->_IO_buf_end = NULL; fp->_IO_read_base = NULL; diff --git a/libio/getc.c b/libio/getc.c index 8c79cf702f..18647aa3e1 100644 --- a/libio/getc.c +++ b/libio/getc.c @@ -34,8 +34,6 @@ _IO_getc (FILE *fp) { int result; CHECK_FILE (fp, EOF); - if (!_IO_need_lock (fp)) - return _IO_getc_unlocked (fp); _IO_acquire_lock (fp); result = _IO_getc_unlocked (fp); _IO_release_lock (fp); diff --git a/libio/getchar.c b/libio/getchar.c index e0f4f471b3..4ab1e7b065 100644 --- a/libio/getchar.c +++ b/libio/getchar.c @@ -33,10 +33,8 @@ int getchar (void) { int result; - if (!_IO_need_lock (stdin)) - return _IO_getc_unlocked (stdin); _IO_acquire_lock (stdin); result = _IO_getc_unlocked (stdin); _IO_release_lock (stdin); return result; -} \ No newline at end of file +} diff --git a/libio/iofopncook.c b/libio/iofopncook.c index 16bcc7f4a3..5ecca41717 100644 --- a/libio/iofopncook.c +++ b/libio/iofopncook.c @@ -162,8 +162,6 @@ _IO_cookie_init (struct _IO_cookie_file *cfile, int read_write, _IO_mask_flags (&cfile->__fp.file, read_write, _IO_NO_READS+_IO_NO_WRITES+_IO_IS_APPENDING); - cfile->__fp.file._flags2 |= _IO_FLAGS2_NEED_LOCK; - /* We use a negative number different from -1 for _fileno to mark that this special stream is not associated with a real file, but still has to be treated as such. */ diff --git a/libio/ioungetc.c b/libio/ioungetc.c index 92d5572e21..547953dd16 100644 --- a/libio/ioungetc.c +++ b/libio/ioungetc.c @@ -33,8 +33,6 @@ ungetc (int c, FILE *fp) CHECK_FILE (fp, EOF); if (c == EOF) return EOF; - if (!_IO_need_lock (fp)) - return _IO_sputbackc (fp, (unsigned char) c); _IO_acquire_lock (fp); result = _IO_sputbackc (fp, (unsigned char) c); _IO_release_lock (fp); diff --git a/libio/libio.h b/libio/libio.h index 42ff6143af..e1e8ffdc54 100644 --- a/libio/libio.h +++ b/libio/libio.h @@ -88,7 +88,6 @@ typedef struct #define _IO_FLAGS2_USER_WBUF 8 #define _IO_FLAGS2_NOCLOSE 32 #define _IO_FLAGS2_CLOEXEC 64 -#define _IO_FLAGS2_NEED_LOCK 128 /* _IO_pos_BAD is an off64_t value indicating error, unknown, or EOF. */ #define _IO_pos_BAD ((off64_t) -1) @@ -212,9 +211,6 @@ extern int _IO_ftrylockfile (FILE *) __THROW; #define _IO_cleanup_region_end(_Doit) /**/ #endif -#define _IO_need_lock(_fp) \ - (((_fp)->_flags2 & _IO_FLAGS2_NEED_LOCK) != 0) - extern int _IO_vfscanf (FILE * __restrict, const char * __restrict, __gnuc_va_list, int *__restrict); extern __ssize_t _IO_padn (FILE *, int, __ssize_t); diff --git a/libio/memstream.c b/libio/memstream.c index 7ab61876ba..15022b72ee 100644 --- a/libio/memstream.c +++ b/libio/memstream.c @@ -92,9 +92,6 @@ __open_memstream (char **bufloc, size_t *sizeloc) new_f->fp.bufloc = bufloc; new_f->fp.sizeloc = sizeloc; - /* Disable single thread optimization. BZ 21735. */ - new_f->fp._sf._sbf._f._flags2 |= _IO_FLAGS2_NEED_LOCK; - return (FILE *) &new_f->fp._sf._sbf; } libc_hidden_def (__open_memstream) diff --git a/libio/putc.c b/libio/putc.c index 44e30649c9..f4cc2024b6 100644 --- a/libio/putc.c +++ b/libio/putc.c @@ -25,8 +25,6 @@ _IO_putc (int c, FILE *fp) { int result; CHECK_FILE (fp, EOF); - if (!_IO_need_lock (fp)) - return _IO_putc_unlocked (c, fp); _IO_acquire_lock (fp); result = _IO_putc_unlocked (c, fp); _IO_release_lock (fp); diff --git a/libio/wmemstream.c b/libio/wmemstream.c index 9366ef4aad..abaf421069 100644 --- a/libio/wmemstream.c +++ b/libio/wmemstream.c @@ -94,9 +94,6 @@ open_wmemstream (wchar_t **bufloc, size_t *sizeloc) new_f->fp.bufloc = bufloc; new_f->fp.sizeloc = sizeloc; - /* Disable single thread optimization. BZ 21735. */ - new_f->fp._sf._sbf._f._flags2 |= _IO_FLAGS2_NEED_LOCK; - return (FILE *) &new_f->fp._sf._sbf; } diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c index e7a099acb7..4f45ea36bc 100644 --- a/nptl/pthread_create.c +++ b/nptl/pthread_create.c @@ -740,9 +740,6 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr, collect_default_sched (pd); } - if (__glibc_unlikely (__nptl_nthreads == 1)) - _IO_enable_locks (); - /* Pass the descriptor to the caller. */ *newthread = (pthread_t) pd; diff --git a/stdio-common/flockfile.c b/stdio-common/flockfile.c index a5decb450f..49f72c69ab 100644 --- a/stdio-common/flockfile.c +++ b/stdio-common/flockfile.c @@ -22,7 +22,6 @@ void __flockfile (FILE *stream) { - stream->_flags2 |= _IO_FLAGS2_NEED_LOCK; _IO_lock_lock (*stream->_lock); } weak_alias (__flockfile, flockfile); diff --git a/sysdeps/mach/libc-lock.h b/sysdeps/mach/libc-lock.h index 225eb67f5a..ee38948d1e 100644 --- a/sysdeps/mach/libc-lock.h +++ b/sysdeps/mach/libc-lock.h @@ -106,9 +106,9 @@ typedef struct __libc_lock_recursive_opaque__ __libc_lock_recursive_t; __libc_lock_recursive_t *const __lock = &(NAME); \ void *__self = __libc_lock_owner_self (); \ int __r = 0; \ - if (__self == __lock->owner) \ + if (!SINGLE_THREAD_P && __self == __lock->owner) \ ++__lock->cnt; \ - else if ((__r = lll_trylock (__lock->lock)) == 0) \ + else if (!SINGLE_THREAD_P && (__r = lll_trylock (__lock->lock)) == 0) \ __lock->owner = __self, __lock->cnt = 1; \ __r; \ }) @@ -117,7 +117,7 @@ typedef struct __libc_lock_recursive_opaque__ __libc_lock_recursive_t; ({ \ __libc_lock_recursive_t *const __lock = &(NAME); \ void *__self = __libc_lock_owner_self (); \ - if (__self != __lock->owner) \ + if (!SINGLE_THREAD_P && __self != __lock->owner) \ { \ lll_lock (__lock->lock, 0); \ __lock->owner = __self; \ @@ -132,7 +132,8 @@ typedef struct __libc_lock_recursive_opaque__ __libc_lock_recursive_t; if (--__lock->cnt == 0) \ { \ __lock->owner = 0; \ - lll_unlock (__lock->lock, 0); \ + if (!SINGLE_THREAD_P) \ + lll_unlock (__lock->lock, 0); \ } \ }) diff --git a/sysdeps/nptl/libc-lock.h b/sysdeps/nptl/libc-lock.h index 6c2d6acfd1..abd84e71b4 100644 --- a/sysdeps/nptl/libc-lock.h +++ b/sysdeps/nptl/libc-lock.h @@ -86,7 +86,7 @@ typedef struct { int lock; int cnt; void *owner; } __libc_lock_recursive_t; # define __libc_lock_lock_recursive(NAME) \ do { \ void *self = THREAD_SELF; \ - if ((NAME).owner != self) \ + if (!SINGLE_THREAD_P && (NAME).owner != self) \ { \ lll_lock ((NAME).lock, LLL_PRIVATE); \ (NAME).owner = self; \ @@ -104,7 +104,7 @@ typedef struct { int lock; int cnt; void *owner; } __libc_lock_recursive_t; ({ \ int result = 0; \ void *self = THREAD_SELF; \ - if ((NAME).owner != self) \ + if (!SINGLE_THREAD_P && (NAME).owner != self) \ { \ if (lll_trylock ((NAME).lock) == 0) \ { \ @@ -131,7 +131,8 @@ typedef struct { int lock; int cnt; void *owner; } __libc_lock_recursive_t; if (--(NAME).cnt == 0) \ { \ (NAME).owner = NULL; \ - lll_unlock ((NAME).lock, LLL_PRIVATE); \ + if (!SINGLE_THREAD_P) \ + lll_unlock ((NAME).lock, LLL_PRIVATE); \ } \ } while (0) #else -- 2.34.1 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 3/4] Move libio lock single-thread optimization to generic libc-lock (BZ #27842) 2022-04-26 19:15 ` [PATCH v2 3/4] Move libio lock single-thread optimization to generic libc-lock (BZ #27842) Adhemerval Zanella @ 2022-04-27 13:30 ` Florian Weimer 2022-04-27 16:32 ` Adhemerval Zanella 0 siblings, 1 reply; 23+ messages in thread From: Florian Weimer @ 2022-04-27 13:30 UTC (permalink / raw) To: Adhemerval Zanella via Libc-alpha * Adhemerval Zanella via Libc-alpha: > diff --git a/sysdeps/nptl/libc-lock.h b/sysdeps/nptl/libc-lock.h > index 6c2d6acfd1..abd84e71b4 100644 > --- a/sysdeps/nptl/libc-lock.h > +++ b/sysdeps/nptl/libc-lock.h > @@ -86,7 +86,7 @@ typedef struct { int lock; int cnt; void *owner; } __libc_lock_recursive_t; > # define __libc_lock_lock_recursive(NAME) \ > do { \ > void *self = THREAD_SELF; \ > - if ((NAME).owner != self) \ > + if (!SINGLE_THREAD_P && (NAME).owner != self) \ > { \ > lll_lock ((NAME).lock, LLL_PRIVATE); \ > (NAME).owner = self; \ > @@ -104,7 +104,7 @@ typedef struct { int lock; int cnt; void *owner; } __libc_lock_recursive_t; > ({ \ > int result = 0; \ > void *self = THREAD_SELF; \ > - if ((NAME).owner != self) \ > + if (!SINGLE_THREAD_P && (NAME).owner != self) \ > { \ > if (lll_trylock ((NAME).lock) == 0) \ > { \ > @@ -131,7 +131,8 @@ typedef struct { int lock; int cnt; void *owner; } __libc_lock_recursive_t; > if (--(NAME).cnt == 0) \ > { \ > (NAME).owner = NULL; \ > - lll_unlock ((NAME).lock, LLL_PRIVATE); \ > + if (!SINGLE_THREAD_P) \ > + lll_unlock ((NAME).lock, LLL_PRIVATE); \ > } \ > } while (0) > #else I don't think this is correct if threads are created in the lock region. Thanks, Florian ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 3/4] Move libio lock single-thread optimization to generic libc-lock (BZ #27842) 2022-04-27 13:30 ` Florian Weimer @ 2022-04-27 16:32 ` Adhemerval Zanella 2022-04-28 16:39 ` Adhemerval Zanella 0 siblings, 1 reply; 23+ messages in thread From: Adhemerval Zanella @ 2022-04-27 16:32 UTC (permalink / raw) To: Florian Weimer, Adhemerval Zanella via Libc-alpha On 27/04/2022 10:30, Florian Weimer wrote: > * Adhemerval Zanella via Libc-alpha: > >> diff --git a/sysdeps/nptl/libc-lock.h b/sysdeps/nptl/libc-lock.h >> index 6c2d6acfd1..abd84e71b4 100644 >> --- a/sysdeps/nptl/libc-lock.h >> +++ b/sysdeps/nptl/libc-lock.h >> @@ -86,7 +86,7 @@ typedef struct { int lock; int cnt; void *owner; } __libc_lock_recursive_t; >> # define __libc_lock_lock_recursive(NAME) \ >> do { \ >> void *self = THREAD_SELF; \ >> - if ((NAME).owner != self) \ >> + if (!SINGLE_THREAD_P && (NAME).owner != self) \ >> { \ >> lll_lock ((NAME).lock, LLL_PRIVATE); \ >> (NAME).owner = self; \ >> @@ -104,7 +104,7 @@ typedef struct { int lock; int cnt; void *owner; } __libc_lock_recursive_t; >> ({ \ >> int result = 0; \ >> void *self = THREAD_SELF; \ >> - if ((NAME).owner != self) \ >> + if (!SINGLE_THREAD_P && (NAME).owner != self) \ >> { \ >> if (lll_trylock ((NAME).lock) == 0) \ >> { \ >> @@ -131,7 +131,8 @@ typedef struct { int lock; int cnt; void *owner; } __libc_lock_recursive_t; >> if (--(NAME).cnt == 0) \ >> { \ >> (NAME).owner = NULL; \ >> - lll_unlock ((NAME).lock, LLL_PRIVATE); \ >> + if (!SINGLE_THREAD_P) \ >> + lll_unlock ((NAME).lock, LLL_PRIVATE); \ >> } \ >> } while (0) >> #else > > I don't think this is correct if threads are created in the lock region. I was not sure about this one and I think we the main issue in fact there is we can't use the single-thread optimization on unlock. Maybe a better option would to use a different scheme as proposed by https://hal.inria.fr/hal-01236734/document, where we can embedded lock and cnt in only one variable (as the lll_lock already does). ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 3/4] Move libio lock single-thread optimization to generic libc-lock (BZ #27842) 2022-04-27 16:32 ` Adhemerval Zanella @ 2022-04-28 16:39 ` Adhemerval Zanella 2022-04-28 16:56 ` Florian Weimer 0 siblings, 1 reply; 23+ messages in thread From: Adhemerval Zanella @ 2022-04-28 16:39 UTC (permalink / raw) To: Florian Weimer, Adhemerval Zanella via Libc-alpha On 27/04/2022 13:32, Adhemerval Zanella wrote: > > > On 27/04/2022 10:30, Florian Weimer wrote: >> * Adhemerval Zanella via Libc-alpha: >> >>> diff --git a/sysdeps/nptl/libc-lock.h b/sysdeps/nptl/libc-lock.h >>> index 6c2d6acfd1..abd84e71b4 100644 >>> --- a/sysdeps/nptl/libc-lock.h >>> +++ b/sysdeps/nptl/libc-lock.h >>> @@ -86,7 +86,7 @@ typedef struct { int lock; int cnt; void *owner; } __libc_lock_recursive_t; >>> # define __libc_lock_lock_recursive(NAME) \ >>> do { \ >>> void *self = THREAD_SELF; \ >>> - if ((NAME).owner != self) \ >>> + if (!SINGLE_THREAD_P && (NAME).owner != self) \ >>> { \ >>> lll_lock ((NAME).lock, LLL_PRIVATE); \ >>> (NAME).owner = self; \ >>> @@ -104,7 +104,7 @@ typedef struct { int lock; int cnt; void *owner; } __libc_lock_recursive_t; >>> ({ \ >>> int result = 0; \ >>> void *self = THREAD_SELF; \ >>> - if ((NAME).owner != self) \ >>> + if (!SINGLE_THREAD_P && (NAME).owner != self) \ >>> { \ >>> if (lll_trylock ((NAME).lock) == 0) \ >>> { \ >>> @@ -131,7 +131,8 @@ typedef struct { int lock; int cnt; void *owner; } __libc_lock_recursive_t; >>> if (--(NAME).cnt == 0) \ >>> { \ >>> (NAME).owner = NULL; \ >>> - lll_unlock ((NAME).lock, LLL_PRIVATE); \ >>> + if (!SINGLE_THREAD_P) \ >>> + lll_unlock ((NAME).lock, LLL_PRIVATE); \ >>> } \ >>> } while (0) >>> #else >> >> I don't think this is correct if threads are created in the lock region. > > I was not sure about this one and I think we the main issue in fact there is > we can't use the single-thread optimization on unlock. Maybe a better option > would to use a different scheme as proposed by > https://hal.inria.fr/hal-01236734/document, where we can embedded lock and > cnt in only one variable (as the lll_lock already does). Don't 99f841c441feeaa9a3d97fd91bb3d6ec8073c982 have the issue for pthread_mutex_lock ? ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 3/4] Move libio lock single-thread optimization to generic libc-lock (BZ #27842) 2022-04-28 16:39 ` Adhemerval Zanella @ 2022-04-28 16:56 ` Florian Weimer 2022-04-28 17:14 ` Adhemerval Zanella 0 siblings, 1 reply; 23+ messages in thread From: Florian Weimer @ 2022-04-28 16:56 UTC (permalink / raw) To: Adhemerval Zanella; +Cc: Adhemerval Zanella via Libc-alpha * Adhemerval Zanella: >>>> (NAME).owner = NULL; \ >>>> - lll_unlock ((NAME).lock, LLL_PRIVATE); \ >>>> + if (!SINGLE_THREAD_P) \ >>>> + lll_unlock ((NAME).lock, LLL_PRIVATE); \ >>>> } \ >>>> } while (0) >>>> #else >>> >>> I don't think this is correct if threads are created in the lock region. >> >> I was not sure about this one and I think we the main issue in fact there is >> we can't use the single-thread optimization on unlock. Maybe a better option >> would to use a different scheme as proposed by >> https://hal.inria.fr/hal-01236734/document, where we can embedded lock and >> cnt in only one variable (as the lll_lock already does). > > Don't 99f841c441feeaa9a3d97fd91bb3d6ec8073c982 have the issue for pthread_mutex_lock ? No, that optimization follows our documented guidance, namely: | Most applications should perform the same actions whether or not | @code{__libc_single_threaded} is true, except with less | synchronization. If this rule is followed, a process that | subsequently becomes multi-threaded is already in a consistent state. Thanks, Florian ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 3/4] Move libio lock single-thread optimization to generic libc-lock (BZ #27842) 2022-04-28 16:56 ` Florian Weimer @ 2022-04-28 17:14 ` Adhemerval Zanella 0 siblings, 0 replies; 23+ messages in thread From: Adhemerval Zanella @ 2022-04-28 17:14 UTC (permalink / raw) To: Florian Weimer; +Cc: Adhemerval Zanella via Libc-alpha On 28/04/2022 13:56, Florian Weimer wrote: > * Adhemerval Zanella: > >>>>> (NAME).owner = NULL; \ >>>>> - lll_unlock ((NAME).lock, LLL_PRIVATE); \ >>>>> + if (!SINGLE_THREAD_P) \ >>>>> + lll_unlock ((NAME).lock, LLL_PRIVATE); \ >>>>> } \ >>>>> } while (0) >>>>> #else >>>> >>>> I don't think this is correct if threads are created in the lock region. >>> >>> I was not sure about this one and I think we the main issue in fact there is >>> we can't use the single-thread optimization on unlock. Maybe a better option >>> would to use a different scheme as proposed by >>> https://hal.inria.fr/hal-01236734/document, where we can embedded lock and >>> cnt in only one variable (as the lll_lock already does). >> >> Don't 99f841c441feeaa9a3d97fd91bb3d6ec8073c982 have the issue for pthread_mutex_lock ? > > No, that optimization follows our documented guidance, namely: > > | Most applications should perform the same actions whether or not > | @code{__libc_single_threaded} is true, except with less > | synchronization. If this rule is followed, a process that > | subsequently becomes multi-threaded is already in a consistent state. > So I wonder if /* Lock the recursive named lock variable. */ #define __libc_lock_lock_recursive(NAME) \ do { \ void *self = THREAD_SELF; \ if ((NAME).owner != self) \ { \ if (SINGLE_THREAD_P) \ (NAME).lock = 1; \ else \ lll_lock ((NAME).lock, LLL_PRIVATE); \ (NAME).owner = self; \ } \ ++(NAME).cnt; \ } while (0) /* Unlock the recursive named lock variable. */ /* We do no error checking here. */ #define __libc_lock_unlock_recursive(NAME) \ do { \ if (--(NAME).cnt == 0) \ { \ (NAME).owner = NULL; \ if (SINGLE_THREAD_P) \ (NAME).lock = 0; \ else \ lll_unlock ((NAME).lock, LLL_PRIVATE); \ } \ } while (0) Or if we are bounded to keep the current practice to check for single-thread and skip locks internally. It would be good to consolidate all the internal lock usage and have the single-thread lock optimizations on all locks, not only on pthread_mutex_lock. ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 4/4] Assume _LIBC and libc module for libc-lock.h 2022-04-26 19:15 [PATCH v2 0/4] Move libio lock single-thread optimization to generic libc-lock Adhemerval Zanella ` (2 preceding siblings ...) 2022-04-26 19:15 ` [PATCH v2 3/4] Move libio lock single-thread optimization to generic libc-lock (BZ #27842) Adhemerval Zanella @ 2022-04-26 19:15 ` Adhemerval Zanella 3 siblings, 0 replies; 23+ messages in thread From: Adhemerval Zanella @ 2022-04-26 19:15 UTC (permalink / raw) To: libc-alpha, Florian Weimer The libc-lock.h is not used outside glibc nor with modules different than libc or libc_malloc_debug. Checked on x86_64-linux-gnu. --- sysdeps/mach/libc-lock.h | 9 ------ sysdeps/nptl/libc-lock.h | 67 ++++++---------------------------------- 2 files changed, 10 insertions(+), 66 deletions(-) diff --git a/sysdeps/mach/libc-lock.h b/sysdeps/mach/libc-lock.h index ee38948d1e..bb1a492fa9 100644 --- a/sysdeps/mach/libc-lock.h +++ b/sysdeps/mach/libc-lock.h @@ -19,8 +19,6 @@ #ifndef _LIBC_LOCK_H #define _LIBC_LOCK_H 1 -#ifdef _LIBC - #include <tls.h> #include <lowlevellock.h> @@ -38,11 +36,6 @@ extern char __libc_lock_self0[0]; #define __libc_lock_owner_self() \ (__LIBC_NO_TLS () ? (void *)&__libc_lock_self0 : THREAD_SELF) -#else -typedef struct __libc_lock_opaque__ __libc_lock_t; -typedef struct __libc_lock_recursive_opaque__ __libc_lock_recursive_t; -#endif - /* Define a lock variable NAME with storage class CLASS. The lock must be initialized with __libc_lock_init before it can be used (or define it with __libc_lock_define_initialized, below). Use `extern' for CLASS to @@ -215,7 +208,6 @@ struct __libc_once /* Get once control variable. */ #define __libc_once_get(ONCE_CONTROL) ((ONCE_CONTROL).done != 0) -#ifdef _LIBC /* We need portable names for some functions. E.g., when they are used as argument to __libc_cleanup_region_start. */ #define __libc_mutex_unlock __libc_lock_unlock @@ -223,6 +215,5 @@ struct __libc_once /* Hide the definitions which are only supposed to be used inside libc in a separate file. This file is not present in the installation! */ # include <libc-lockP.h> -#endif #endif /* libc-lock.h */ diff --git a/sysdeps/nptl/libc-lock.h b/sysdeps/nptl/libc-lock.h index abd84e71b4..5c85a250d7 100644 --- a/sysdeps/nptl/libc-lock.h +++ b/sysdeps/nptl/libc-lock.h @@ -25,11 +25,7 @@ /* Mutex type. */ -#if !IS_IN (libc) && !IS_IN (libc_malloc_debug) -typedef struct { pthread_mutex_t mutex; } __libc_lock_recursive_t; -#else typedef struct { int lock; int cnt; void *owner; } __libc_lock_recursive_t; -#endif /* Define a lock variable NAME with storage class CLASS. The lock must be initialized with __libc_lock_init before it can be used (or define it @@ -43,68 +39,36 @@ typedef struct { int lock; int cnt; void *owner; } __libc_lock_recursive_t; /* Define an initialized recursive lock variable NAME with storage class CLASS. */ -#if IS_IN (libc) || IS_IN (libc_malloc_debug) -# define __libc_lock_define_initialized_recursive(CLASS, NAME) \ +#define __libc_lock_define_initialized_recursive(CLASS, NAME) \ CLASS __libc_lock_recursive_t NAME = _LIBC_LOCK_RECURSIVE_INITIALIZER; -# define _LIBC_LOCK_RECURSIVE_INITIALIZER \ +#define _LIBC_LOCK_RECURSIVE_INITIALIZER \ { LLL_LOCK_INITIALIZER, 0, NULL } -#else -# define __libc_lock_define_initialized_recursive(CLASS,NAME) \ - CLASS __libc_lock_recursive_t NAME = _LIBC_LOCK_RECURSIVE_INITIALIZER; -# define _LIBC_LOCK_RECURSIVE_INITIALIZER \ - {PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP} -#endif /* Initialize a recursive mutex. */ -#if IS_IN (libc) || IS_IN (libc_malloc_debug) -# define __libc_lock_init_recursive(NAME) \ +#define __libc_lock_init_recursive(NAME) \ ((void) ((NAME) = (__libc_lock_recursive_t) _LIBC_LOCK_RECURSIVE_INITIALIZER)) -#else -# define __libc_lock_init_recursive(NAME) \ - do { \ - if (__pthread_mutex_init != NULL) \ - { \ - pthread_mutexattr_t __attr; \ - __pthread_mutexattr_init (&__attr); \ - __pthread_mutexattr_settype (&__attr, PTHREAD_MUTEX_RECURSIVE_NP); \ - __pthread_mutex_init (&(NAME).mutex, &__attr); \ - __pthread_mutexattr_destroy (&__attr); \ - } \ - } while (0) -#endif /* Finalize recursive named lock. */ -#if IS_IN (libc) || IS_IN (libc_malloc_debug) -# define __libc_lock_fini_recursive(NAME) ((void) 0) -#else -# define __libc_lock_fini_recursive(NAME) \ - __libc_maybe_call (__pthread_mutex_destroy, (&(NAME).mutex), 0) -#endif +#define __libc_lock_fini_recursive(NAME) ((void) 0) /* Lock the recursive named lock variable. */ -#if IS_IN (libc) || IS_IN (libc_malloc_debug) -# define __libc_lock_lock_recursive(NAME) \ +#define __libc_lock_lock_recursive(NAME) \ do { \ void *self = THREAD_SELF; \ - if (!SINGLE_THREAD_P && (NAME).owner != self) \ + if (!SINGLE_THREAD_P && (NAME).owner != self) \ { \ lll_lock ((NAME).lock, LLL_PRIVATE); \ (NAME).owner = self; \ } \ ++(NAME).cnt; \ } while (0) -#else -# define __libc_lock_lock_recursive(NAME) \ - __libc_maybe_call (__pthread_mutex_lock, (&(NAME).mutex), 0) -#endif /* Try to lock the recursive named lock variable. */ -#if IS_IN (libc) || IS_IN (libc_malloc_debug) -# define __libc_lock_trylock_recursive(NAME) \ +#define __libc_lock_trylock_recursive(NAME) \ ({ \ int result = 0; \ void *self = THREAD_SELF; \ - if (!SINGLE_THREAD_P && (NAME).owner != self) \ + if (!SINGLE_THREAD_P && (NAME).owner != self) \ { \ if (lll_trylock ((NAME).lock) == 0) \ { \ @@ -118,15 +82,10 @@ typedef struct { int lock; int cnt; void *owner; } __libc_lock_recursive_t; ++(NAME).cnt; \ result; \ }) -#else -# define __libc_lock_trylock_recursive(NAME) \ - __libc_maybe_call (__pthread_mutex_trylock, (&(NAME).mutex), 0) -#endif /* Unlock the recursive named lock variable. */ -#if IS_IN (libc) || IS_IN (libc_malloc_debug) /* We do no error checking here. */ -# define __libc_lock_unlock_recursive(NAME) \ +#define __libc_lock_unlock_recursive(NAME) \ do { \ if (--(NAME).cnt == 0) \ { \ @@ -135,10 +94,6 @@ typedef struct { int lock; int cnt; void *owner; } __libc_lock_recursive_t; lll_unlock ((NAME).lock, LLL_PRIVATE); \ } \ } while (0) -#else -# define __libc_lock_unlock_recursive(NAME) \ - __libc_maybe_call (__pthread_mutex_unlock, (&(NAME).mutex), 0) -#endif /* Put the unwind buffer BUFFER on the per-thread callback stack. The caller must fill BUFFER->__routine and BUFFER->__arg before calling @@ -178,8 +133,6 @@ libc_hidden_proto (__libc_cleanup_pop_restore) /* Hide the definitions which are only supposed to be used inside libc in a separate file. This file is not present in the installation! */ -#ifdef _LIBC -# include "libc-lockP.h" -#endif +#include "libc-lockP.h" #endif /* libc-lock.h */ -- 2.34.1 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 3/4] Move libio lock single-thread optimization to generic libc-lock (BZ #27842)
@ 2022-04-29 13:04 Wilco Dijkstra
2022-04-29 17:13 ` Adhemerval Zanella
0 siblings, 1 reply; 23+ messages in thread
From: Wilco Dijkstra @ 2022-04-29 13:04 UTC (permalink / raw)
To: Adhemerval Zanella; +Cc: 'GNU C Library', Florian Weimer
Hi Adhemerval,
/* Lock the recursive named lock variable. */
#define __libc_lock_lock_recursive(NAME) \
do { \
void *self = THREAD_SELF; \
if ((NAME).owner != self) \
{ \
if (SINGLE_THREAD_P) \
(NAME).lock = 1; \
else \
lll_lock ((NAME).lock, LLL_PRIVATE); \
(NAME).owner = self; \
} \
++(NAME).cnt; \
} while (0)
The issue is that recursive locks are very expensive. Even with the single thread
optimization there are still 5 memory accesses just to take a free lock! While I'm in
favour of improving locks like above, I think removing the single threaded optimizations
from libio will cause major performance regressions in getc, putc, feof etc.
Basically doing single threaded optimizations at the highest level will always be faster
than doing it at the lock level. That doesn't mean optimizing general locks isn't useful,
however all it does is reduce the need to add single threaded optimizations rather than
make them completely unnecessary.
> Or if we are bounded to keep the current practice to check for single-thread and
> skip locks internally. It would be good to consolidate all the internal lock
> usage and have the single-thread lock optimizations on all locks, not only on
> pthread_mutex_lock.
Yes, we should have single threaded optimization on all locks, including __libc_lock_lock.
Recursive locks can be optimized both for single threaded and multi-threaded case.
I'm wondering whether we could use the lock variable itself to store useful data (since most
ISAs will allow 64 bits). Then all you need to check is the .lock field and only if it is already
taken do the extra checks for ownership and incrementing recursion counter.
Cheers,
Wilco
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v2 3/4] Move libio lock single-thread optimization to generic libc-lock (BZ #27842) 2022-04-29 13:04 [PATCH v2 3/4] Move libio lock single-thread optimization to generic libc-lock (BZ #27842) Wilco Dijkstra @ 2022-04-29 17:13 ` Adhemerval Zanella 2022-05-16 16:17 ` Wilco Dijkstra 0 siblings, 1 reply; 23+ messages in thread From: Adhemerval Zanella @ 2022-04-29 17:13 UTC (permalink / raw) To: Wilco Dijkstra; +Cc: 'GNU C Library', Florian Weimer On 29/04/2022 10:04, Wilco Dijkstra wrote: > Hi Adhemerval, > > /* Lock the recursive named lock variable. */ > #define __libc_lock_lock_recursive(NAME) \ > do { \ > void *self = THREAD_SELF; \ > if ((NAME).owner != self) \ > { \ > if (SINGLE_THREAD_P) \ > (NAME).lock = 1; \ > else \ > lll_lock ((NAME).lock, LLL_PRIVATE); \ > (NAME).owner = self; \ > } \ > ++(NAME).cnt; \ > } while (0) > > > The issue is that recursive locks are very expensive. Even with the single thread > optimization there are still 5 memory accesses just to take a free lock! While I'm in > favour of improving locks like above, I think removing the single threaded optimizations > from libio will cause major performance regressions in getc, putc, feof etc. > > Basically doing single threaded optimizations at the highest level will always be faster > than doing it at the lock level. That doesn't mean optimizing general locks isn't useful, > however all it does is reduce the need to add single threaded optimizations rather than > make them completely unnecessary. The main problem is _IO_enable_locks is a clunky interface because it requires flockfile to set _flags2 outside a lock region leading a possible racy issue (BZ #27842). Moving to lock itself it will pretty much: lock: if (THREAD_SELF->owner != self) THREAD_SELF->lock = 1 unlock: if (THREAD_SELF.cnt == 0) { THREAD_SELF->owner = NULL THREAD_SELF.lock = 0; } Which is still a regression, but at least we don't have a race condition on flockfile anymore. > >> Or if we are bounded to keep the current practice to check for single-thread and >> skip locks internally. It would be good to consolidate all the internal lock >> usage and have the single-thread lock optimizations on all locks, not only on >> pthread_mutex_lock. > > Yes, we should have single threaded optimization on all locks, including __libc_lock_lock. > Recursive locks can be optimized both for single threaded and multi-threaded case. > I'm wondering whether we could use the lock variable itself to store useful data (since most > ISAs will allow 64 bits). Then all you need to check is the .lock field and only if it is already > taken do the extra checks for ownership and incrementing recursion counter. > I think ideally we would like to model all internal locks to a futex-like so we can use the congestion optimization as described by Jens Gustedt paper [1] which would allows us to move the counter and the lock to same word. I don't think we can improve recursive locks without a 64-bit futex operation. Then assuming lll_lock handle the counter, we might further optimize the lock as (without any single-thread optimization): typedef union { uint32_t lck; pid_t owner; } __libc_lock_recursive_t; static inline void __libc_lock_lock_recursive (__libc_lock_recursive_t *l) { pid_t tid = THREAD_SELF->pid; if (l->l.owner != tid) { lll_lock (l->l.lck, LLL_PRIVATE); l->l.owner = tid; } } static inline void __libc_lock_unlock_recursive (__libc_lock_recursive_t *l) { /* not sure if this yield any gain here... */ l->l.owner = 0; lll_unlock (l->l.lck); } And lll_lock and lll_unlock will handle the counter congestion. I think the single-thread optimization could be done on lll_lock/lll_unlock as well. [1] https://hal.inria.fr/hal-01236734/document ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 3/4] Move libio lock single-thread optimization to generic libc-lock (BZ #27842) 2022-04-29 17:13 ` Adhemerval Zanella @ 2022-05-16 16:17 ` Wilco Dijkstra 2022-05-16 16:23 ` Florian Weimer 0 siblings, 1 reply; 23+ messages in thread From: Wilco Dijkstra @ 2022-05-16 16:17 UTC (permalink / raw) To: Adhemerval Zanella; +Cc: 'GNU C Library', Florian Weimer Hi Adhemerval, > The main problem is _IO_enable_locks is a clunky interface because it requires > flockfile to set _flags2 outside a lock region leading a possible racy issue > (BZ #27842). Moving to lock itself it will pretty much: It should be fine if we use a boolean instead of a flag. IIRC the IO structure was externally exposed in Emacs, but if that is no longer the case then we could change the structure safely. A quick check of rand() shows the following results for various locks and optimizations (relative performance compared to unlocked case): 100% - rand() without locking 317% - standard lock (= current code) 108% - add SINGLE_THREAD_P optimization inside rand 112% - single threaded optimization in _libc_lock_lock 373% - standard recursive lock 129% - recursive lock with single thread optimization Locks are expensive and adding a single-thread optimization is important! It looks recursive locks remain expensive (~20% slower) compared to specialized single-thread optimization, but normal locks might be fast enough in most cases. > I think ideally we would like to model all internal locks to a futex-like > so we can use the congestion optimization as described by Jens Gustedt > paper [1] which would allows us to move the counter and the lock to > same word. I don't think we can improve recursive locks without a 64-bit > futex operation. How much lock recursion exists in GLIBC? An 8-bit counter is likely sufficient... With a 64-bit lock you could combine owner and count into a single value - this may not help the multi-threaded case much, but could reduce the overhead of the single-thread case compared to non-recursive locks. Cheers, Wilco ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 3/4] Move libio lock single-thread optimization to generic libc-lock (BZ #27842) 2022-05-16 16:17 ` Wilco Dijkstra @ 2022-05-16 16:23 ` Florian Weimer 0 siblings, 0 replies; 23+ messages in thread From: Florian Weimer @ 2022-05-16 16:23 UTC (permalink / raw) To: Wilco Dijkstra; +Cc: Adhemerval Zanella, 'GNU C Library' * Wilco Dijkstra: > Hi Adhemerval, > >> The main problem is _IO_enable_locks is a clunky interface because it requires >> flockfile to set _flags2 outside a lock region leading a possible racy issue >> (BZ #27842). Moving to lock itself it will pretty much: > > It should be fine if we use a boolean instead of a flag. IIRC the IO > structure was externally exposed in Emacs, but if that is no longer > the case then we could change the structure safely. gnulib has code that directly _flags (not _flags2). gnulib is statically linked, which makes for a long transition time. >> I think ideally we would like to model all internal locks to a futex-like >> so we can use the congestion optimization as described by Jens Gustedt >> paper [1] which would allows us to move the counter and the lock to >> same word. I don't think we can improve recursive locks without a 64-bit >> futex operation. > > How much lock recursion exists in GLIBC? An 8-bit counter is likely > sufficient... One of the loader logs is recursive, and dlopen calls can be nested basically arbitrarily deeply in ELF constructors. Thanks, Florian ^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2022-05-16 16:23 UTC | newest] Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-04-26 19:15 [PATCH v2 0/4] Move libio lock single-thread optimization to generic libc-lock Adhemerval Zanella 2022-04-26 19:15 ` [PATCH v2 1/4] libio: Assume _IO_MTSAFE_IO Adhemerval Zanella 2022-04-27 12:34 ` Florian Weimer 2022-04-27 18:40 ` Adhemerval Zanella 2022-04-27 19:35 ` Adhemerval Zanella 2022-04-26 19:15 ` [PATCH v2 2/4] Consolidate stdio-lock.h Adhemerval Zanella 2022-04-27 13:25 ` Florian Weimer 2022-04-27 16:15 ` Adhemerval Zanella 2022-04-27 18:00 ` Florian Weimer 2022-04-27 18:35 ` Adhemerval Zanella 2022-04-27 18:44 ` Florian Weimer 2022-04-27 18:49 ` Adhemerval Zanella 2022-04-26 19:15 ` [PATCH v2 3/4] Move libio lock single-thread optimization to generic libc-lock (BZ #27842) Adhemerval Zanella 2022-04-27 13:30 ` Florian Weimer 2022-04-27 16:32 ` Adhemerval Zanella 2022-04-28 16:39 ` Adhemerval Zanella 2022-04-28 16:56 ` Florian Weimer 2022-04-28 17:14 ` Adhemerval Zanella 2022-04-26 19:15 ` [PATCH v2 4/4] Assume _LIBC and libc module for libc-lock.h Adhemerval Zanella 2022-04-29 13:04 [PATCH v2 3/4] Move libio lock single-thread optimization to generic libc-lock (BZ #27842) Wilco Dijkstra 2022-04-29 17:13 ` Adhemerval Zanella 2022-05-16 16:17 ` Wilco Dijkstra 2022-05-16 16:23 ` Florian Weimer
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).