From: Lukasz Majewski <lukma@denx.de>
To: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Cc: Joseph Myers <joseph@codesourcery.com>,
Paul Eggert <eggert@cs.ucla.edu>,
Alistair Francis <alistair23@gmail.com>,
Arnd Bergmann <arnd@arndb.de>,
Alistair Francis <alistair.francis@wdc.com>,
GNU C Library <libc-alpha@sourceware.org>,
Florian Weimer <fweimer@redhat.com>,
Carlos O'Donell <carlos@redhat.com>,
Stepan Golosunov <stepan@golosunov.pp.ru>,
Andreas Schwab <schwab@suse.de>, Zack Weinberg <zackw@panix.com>
Subject: Re: [PATCH v2 1/2] aio: Provide Linux specific copy of aio_suspend.c
Date: Tue, 17 Nov 2020 16:58:15 +0100 [thread overview]
Message-ID: <20201117165815.2514d919@jawa> (raw)
In-Reply-To: <3b8dc4d3-5e12-7d03-735d-b1a717063dd2@linaro.org>
[-- Attachment #1: Type: text/plain, Size: 11817 bytes --]
Hi Adhemerval,
> On 14/11/2020 18:45, Lukasz Majewski wrote:
> > This patch is a preparatory work, to provide 64 bit time support
> > to aio_suspend.c, which would avoid build breaks on the HURD
> > (i686-gnu) port, as it is not supporting 64 bit version of
> > __pthread_cond_timedwait64.
> >
> > The file sysdeps/unix/sysv/linux/aio_suspend.c is the exact copy of
> > ./sysdeps/pthread/aio_suspend.c
> >
> > ---
> > Changes for v2:
> > - Rephase the commit message
> > ---
>
> I am not very found of this code duplication and the Linux version
> does not need !DONT_NEED_AIO_MISC_COND, since nptl/aio_misc.h will
> always define to 1.
>
> I think it would be better to move the sysdeps/pthread/aio_suspend.c
> to a parametrized implementation
> (sysdeps/pthread/aio_suspend_skeleton.c or something) where the
> internal types and function should be defined (similar to the
> dynamic_array.h) something like:
>
> 94 #ifdef DONT_NEED_AIO_MISC_COND
> 95 static int
> 96 __attribute__ ((noinline))
> 97 do_aio_misc_wait (unsigned int *cntr, const TIMESPEC_STRUCT
> *timeout) 98 {
> 99 int result = 0;
> 100
> 101 AIO_MISC_WAIT_MACRO (result, *cntr, timeout, 1);
> 102
> 103 return result;
> 104 }
> 105 #endif
> 106
> 107 int
> 108 AIO_SUSPEND (const struct aiocb *const list[], int nent,
> 109 const TIMESPEC_STRUCT *timeout)
> 110 {
> 111 if (__glibc_unlikely (nent < 0))
> 112 {
> 113 __set_errno (EINVAL);
> 114 return -1;
> 115 }
>
> And then generic sysdeps/pthread/aio_suspend.c will define the types
> and include the parametrized implementation:
>
> #define TIMESPEC_STRUCT struct timespec
> #define AIO_SUSPEND aio_suspend
> #include <sysdeps/pthread/aio_suspend_skeleton.c>
>
>
> The Linux implementation sysdeps/unix/sysv/linux/aio_suspend.c then
> can do:
>
>
> #if __TIMESIZE != 64
> # define TIMESPEC_STRUCT struct __timespec64
> # define AIO_SUSPEND __aio_suspend64_time64
> # define AIO_MISC_WAIT_MACRO AIO_MISC_WAIT64
> #else
> # define TIMESPEC_STRUCT struct timespec
> # define AIO_SUSPEND aio_suspend
> # define AIO_MISC_WAIT_MACRO AIO_MISC_WAIT
> #endif
> #include <sysdeps/pthread/aio_suspend_skeleton.c>
>
> #if __TIMESIZE != 64
> librt_hidden_def (__aio_suspend_time64)
>
> int
> __aio_suspend (const struct aiocb *const list[], int nent,
> const struct timespec *timeout)
> {
> struct __timespec64 ts64;
> if (timeout != NULL)
> ts64 = valid_timespec_to_timespec64 (*timeout);
> return __aio_suspend_time64 (list, nent, timeout != NULL ? &ts64
> : NULL); }
> #endif
>
Ok. This shall help with avoiding code duplication.
>
> > sysdeps/unix/sysv/linux/aio_suspend.c | 253
> > ++++++++++++++++++++++++++ 1 file changed, 253 insertions(+)
> > create mode 100644 sysdeps/unix/sysv/linux/aio_suspend.c
> >
> > diff --git a/sysdeps/unix/sysv/linux/aio_suspend.c
> > b/sysdeps/unix/sysv/linux/aio_suspend.c new file mode 100644
> > index 0000000000..ad03f13558
> > --- /dev/null
> > +++ b/sysdeps/unix/sysv/linux/aio_suspend.c
> > @@ -0,0 +1,253 @@
> > +/* Suspend until termination of a requests.
> > + Copyright (C) 1997-2020 Free Software Foundation, Inc.
> > + This file is part of the GNU C Library.
> > + Contributed by Ulrich Drepper <drepper@cygnus.com>, 1997.
> > +
> > + 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/>. */
> > +
> > +
> > +/* We use an UGLY hack to prevent gcc from finding us cheating.
> > The
> > + implementations of aio_suspend and aio_suspend64 are identical
> > and so
> > + we want to avoid code duplication by using aliases. But gcc
> > sees
> > + the different parameter lists and prints a warning. We define
> > here
> > + a function so that aio_suspend64 has no prototype. */
> > +#define aio_suspend64 XXX
> > +#include <aio.h>
> > +/* And undo the hack. */
> > +#undef aio_suspend64
> > +
> > +#include <assert.h>
> > +#include <errno.h>
> > +#include <stdbool.h>
> > +#include <stdlib.h>
> > +#include <sys/time.h>
> > +
> > +#include <libc-lock.h>
> > +#include <aio_misc.h>
> > +
> > +
> > +struct clparam
> > +{
> > + const struct aiocb *const *list;
> > + struct waitlist *waitlist;
> > + struct requestlist **requestlist;
> > +#ifndef DONT_NEED_AIO_MISC_COND
> > + pthread_cond_t *cond;
> > +#endif
> > + int nent;
> > +};
> > +
> > +
> > +static void
> > +cleanup (void *arg)
> > +{
> > +#ifdef DONT_NEED_AIO_MISC_COND
> > + /* Acquire the mutex. If pthread_cond_*wait is used this would
> > + happen implicitly. */
> > + pthread_mutex_lock (&__aio_requests_mutex);
> > +#endif
> > +
> > + const struct clparam *param = (const struct clparam *) arg;
> > +
> > + /* Now remove the entry in the waiting list for all requests
> > + which didn't terminate. */
> > + int cnt = param->nent;
> > + while (cnt-- > 0)
> > + if (param->list[cnt] != NULL
> > + && param->list[cnt]->__error_code == EINPROGRESS)
> > + {
> > + struct waitlist **listp;
> > +
> > + assert (param->requestlist[cnt] != NULL);
> > +
> > + /* There is the chance that we cannot find our entry
> > anymore. This
> > + could happen if the request terminated and restarted
> > again. */
> > + listp = ¶m->requestlist[cnt]->waiting;
> > + while (*listp != NULL && *listp != ¶m->waitlist[cnt])
> > + listp = &(*listp)->next;
> > +
> > + if (*listp != NULL)
> > + *listp = (*listp)->next;
> > + }
> > +
> > +#ifndef DONT_NEED_AIO_MISC_COND
> > + /* Release the conditional variable. */
> > + (void) pthread_cond_destroy (param->cond);
> > +#endif
> > +
> > + /* Release the mutex. */
> > + pthread_mutex_unlock (&__aio_requests_mutex);
> > +}
> > +
> > +#ifdef DONT_NEED_AIO_MISC_COND
> > +static int
> > +__attribute__ ((noinline))
> > +do_aio_misc_wait (unsigned int *cntr, const struct timespec
> > *timeout) +{
> > + int result = 0;
> > +
> > + AIO_MISC_WAIT (result, *cntr, timeout, 1);
> > +
> > + return result;
> > +}
> > +#endif
> > +
> > +int
> > +aio_suspend (const struct aiocb *const list[], int nent,
> > + const struct timespec *timeout)
> > +{
> > + if (__glibc_unlikely (nent < 0))
> > + {
> > + __set_errno (EINVAL);
> > + return -1;
> > + }
> > +
> > + struct waitlist waitlist[nent];
> > + struct requestlist *requestlist[nent];
> > +#ifndef DONT_NEED_AIO_MISC_COND
> > + pthread_cond_t cond = PTHREAD_COND_INITIALIZER;
> > +#endif
> > + int cnt;
> > + bool any = false;
> > + int result = 0;
> > + unsigned int cntr = 1;
> > +
> > + /* Request the mutex. */
> > + pthread_mutex_lock (&__aio_requests_mutex);
> > +
> > + /* There is not yet a finished request. Signal the request that
> > + we are working for it. */
> > + for (cnt = 0; cnt < nent; ++cnt)
> > + if (list[cnt] != NULL)
> > + {
> > + if (list[cnt]->__error_code == EINPROGRESS)
> > + {
> > + requestlist[cnt] = __aio_find_req ((aiocb_union *)
> > list[cnt]); +
> > + if (requestlist[cnt] != NULL)
> > + {
> > +#ifndef DONT_NEED_AIO_MISC_COND
> > + waitlist[cnt].cond = &cond;
> > +#endif
> > + waitlist[cnt].result = NULL;
> > + waitlist[cnt].next = requestlist[cnt]->waiting;
> > + waitlist[cnt].counterp = &cntr;
> > + waitlist[cnt].sigevp = NULL;
> > + requestlist[cnt]->waiting = &waitlist[cnt];
> > + any = true;
> > + }
> > + else
> > + /* We will never suspend. */
> > + break;
> > + }
> > + else
> > + /* We will never suspend. */
> > + break;
> > + }
> > +
> > +
> > + /* Only if none of the entries is NULL or finished to be wait.
> > */
> > + if (cnt == nent && any)
> > + {
> > + struct clparam clparam =
> > + {
> > + .list = list,
> > + .waitlist = waitlist,
> > + .requestlist = requestlist,
> > +#ifndef DONT_NEED_AIO_MISC_COND
> > + .cond = &cond,
> > +#endif
> > + .nent = nent
> > + };
> > +
> > + pthread_cleanup_push (cleanup, &clparam);
> > +
> > +#ifdef DONT_NEED_AIO_MISC_COND
> > + result = do_aio_misc_wait (&cntr, timeout);
> > +#else
> > + if (timeout == NULL)
> > + result = pthread_cond_wait (&cond, &__aio_requests_mutex);
> > + else
> > + {
> > + /* We have to convert the relative timeout value into an
> > + absolute time value with pthread_cond_timedwait
> > expects. */
> > + struct timespec now;
> > + struct timespec abstime;
> > +
> > + __clock_gettime (CLOCK_REALTIME, &now);
> > + abstime.tv_nsec = timeout->tv_nsec + now.tv_nsec;
> > + abstime.tv_sec = timeout->tv_sec + now.tv_sec;
> > + if (abstime.tv_nsec >= 1000000000)
> > + {
> > + abstime.tv_nsec -= 1000000000;
> > + abstime.tv_sec += 1;
> > + }
> > +
> > + result = pthread_cond_timedwait (&cond,
> > &__aio_requests_mutex,
> > + &abstime);
> > + }
> > +#endif
> > +
> > + pthread_cleanup_pop (0);
> > + }
> > +
> > + /* Now remove the entry in the waiting list for all requests
> > + which didn't terminate. */
> > + while (cnt-- > 0)
> > + if (list[cnt] != NULL && list[cnt]->__error_code ==
> > EINPROGRESS)
> > + {
> > + struct waitlist **listp;
> > +
> > + assert (requestlist[cnt] != NULL);
> > +
> > + /* There is the chance that we cannot find our entry
> > anymore. This
> > + could happen if the request terminated and restarted
> > again. */
> > + listp = &requestlist[cnt]->waiting;
> > + while (*listp != NULL && *listp != &waitlist[cnt])
> > + listp = &(*listp)->next;
> > +
> > + if (*listp != NULL)
> > + *listp = (*listp)->next;
> > + }
> > +
> > +#ifndef DONT_NEED_AIO_MISC_COND
> > + /* Release the conditional variable. */
> > + if (__glibc_unlikely (pthread_cond_destroy (&cond) != 0))
> > + /* This must never happen. */
> > + abort ();
> > +#endif
> > +
> > + if (result != 0)
> > + {
> > +#ifndef DONT_NEED_AIO_MISC_COND
> > + /* An error occurred. Possibly it's ETIMEDOUT. We have to
> > translate
> > + the timeout error report of `pthread_cond_timedwait' to
> > the
> > + form expected from `aio_suspend'. */
> > + if (result == ETIMEDOUT)
> > + __set_errno (EAGAIN);
> > + else
> > +#endif
> > + __set_errno (result);
> > +
> > + result = -1;
> > + }
> > +
> > + /* Release the mutex. */
> > + pthread_mutex_unlock (&__aio_requests_mutex);
> > +
> > + return result;
> > +}
> > +
> > +weak_alias (aio_suspend, aio_suspend64)
> >
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
prev parent reply other threads:[~2020-11-17 15:58 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-14 21:45 Lukasz Majewski
2020-11-14 21:45 ` [PATCH v2 2/2] y2038: Convert aio_suspend to support 64 bit time Lukasz Majewski
2020-11-17 14:41 ` Adhemerval Zanella
2020-11-17 23:01 ` Lukasz Majewski
2020-11-17 14:27 ` [PATCH v2 1/2] aio: Provide Linux specific copy of aio_suspend.c Adhemerval Zanella
2020-11-17 15:58 ` Lukasz Majewski [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20201117165815.2514d919@jawa \
--to=lukma@denx.de \
--cc=adhemerval.zanella@linaro.org \
--cc=alistair.francis@wdc.com \
--cc=alistair23@gmail.com \
--cc=arnd@arndb.de \
--cc=carlos@redhat.com \
--cc=eggert@cs.ucla.edu \
--cc=fweimer@redhat.com \
--cc=joseph@codesourcery.com \
--cc=libc-alpha@sourceware.org \
--cc=schwab@suse.de \
--cc=stepan@golosunov.pp.ru \
--cc=zackw@panix.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).