diff mbox series

[v2,8/8] linux-user: Simplify host <-> target errno conversion using macros

Message ID 20210708141121.1731691-9-f4bug@amsat.org
State New
Headers show
Series linux-user: target <-> host errno conversion code refactor | expand

Commit Message

Philippe Mathieu-Daudé July 8, 2021, 2:11 p.m. UTC 
Convert the host_to_target_errno_table[] array to a switch case
to allow compiler optimizations. Extract the errnos list as to
a new includible unit, using a generic macro. Remove the code
related to target_to_host_errno_table[] initialization.

Suggested-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 linux-user/syscall.c    | 169 +++++-----------------------------------
 linux-user/errnos.c.inc | 140 +++++++++++++++++++++++++++++++++
 2 files changed, 161 insertions(+), 148 deletions(-)
 create mode 100644 linux-user/errnos.c.inc

Comments

Laurent Vivier July 8, 2021, 3:30 p.m. UTC | #1 
Le 08/07/2021 à 16:11, Philippe Mathieu-Daudé a écrit :
> Convert the host_to_target_errno_table[] array to a switch case
> to allow compiler optimizations. Extract the errnos list as to
> a new includible unit, using a generic macro. Remove the code
> related to target_to_host_errno_table[] initialization.
> 

Is there some performance penalties by using a switch() rather than an array[] ?

> Suggested-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  linux-user/syscall.c    | 169 +++++-----------------------------------
>  linux-user/errnos.c.inc | 140 +++++++++++++++++++++++++++++++++
>  2 files changed, 161 insertions(+), 148 deletions(-)
>  create mode 100644 linux-user/errnos.c.inc
> 
...
> diff --git a/linux-user/errnos.c.inc b/linux-user/errnos.c.inc
> new file mode 100644
> index 00000000000..807c97ca25e
> --- /dev/null
> +++ b/linux-user/errnos.c.inc
> @@ -0,0 +1,140 @@
...
> +#ifdef ERKFILL

You fix it in patch 1 but forgot to report it in your patch :)

Thanks,
Laurent
Richard Henderson July 8, 2021, 3:44 p.m. UTC | #2 
On 7/8/21 7:11 AM, Philippe Mathieu-Daudé wrote:
> Convert the host_to_target_errno_table[] array to a switch case
> to allow compiler optimizations. Extract the errnos list as to
> a new includible unit, using a generic macro. Remove the code
> related to target_to_host_errno_table[] initialization.
> 
> Suggested-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   linux-user/syscall.c    | 169 +++++-----------------------------------
>   linux-user/errnos.c.inc | 140 +++++++++++++++++++++++++++++++++
>   2 files changed, 161 insertions(+), 148 deletions(-)
>   create mode 100644 linux-user/errnos.c.inc
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 56682b06cbd..8bb528d2cf7 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -507,157 +507,37 @@ static inline int next_free_host_timer(void)
>   }
>   #endif
>   
> -#define ERRNO_TABLE_SIZE 1200
> -
>   static inline bool errno_exists(int err)
>   {
> -    return err >= 0 && err < ERRNO_TABLE_SIZE;
> +    switch (err) {
> +#define E(X)  case X: return true;
> +#include "errnos.c.inc"
> +#undef E
> +    default:
> +        return false;
> +    }
>   }

Not true.  As documented, errnos.c.inc only contains those errno values which are 
overridden, not all errno values which are valid.

r~
Richard Henderson July 8, 2021, 3:48 p.m. UTC | #3 
On 7/8/21 8:30 AM, Laurent Vivier wrote:
> Le 08/07/2021 à 16:11, Philippe Mathieu-Daudé a écrit :
>> Convert the host_to_target_errno_table[] array to a switch case
>> to allow compiler optimizations. Extract the errnos list as to
>> a new includible unit, using a generic macro. Remove the code
>> related to target_to_host_errno_table[] initialization.
>>
> 
> Is there some performance penalties by using a switch() rather than an array[] ?

In many cases, definitely not.  The compiler does notice the identity function when host 
and guest errnos match.

In the other case, I would doubt (without evidence) there's much penalty.  The switch 
becomes a table lookup + indirect branch + immediate load + return.  Or a really good 
compiler transforms to a different array lookup.


r~
Philippe Mathieu-Daudé July 8, 2021, 4:09 p.m. UTC | #4 
On 7/8/21 5:30 PM, Laurent Vivier wrote:
> Le 08/07/2021 à 16:11, Philippe Mathieu-Daudé a écrit :
>> Convert the host_to_target_errno_table[] array to a switch case
>> to allow compiler optimizations. Extract the errnos list as to
>> a new includible unit, using a generic macro. Remove the code
>> related to target_to_host_errno_table[] initialization.
>>
> 
> Is there some performance penalties by using a switch() rather than an array[] ?

Per Richard suggestion
https://www.mail-archive.com/qemu-devel@nongnu.org/msg821488.html
rather the opposite, it might be now easier for the compiler to
optimize. And we free unused entries in the array (.rodata).

>> Suggested-by: Richard Henderson <richard.henderson@linaro.org>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  linux-user/syscall.c    | 169 +++++-----------------------------------
>>  linux-user/errnos.c.inc | 140 +++++++++++++++++++++++++++++++++
>>  2 files changed, 161 insertions(+), 148 deletions(-)
>>  create mode 100644 linux-user/errnos.c.inc
>>
> ...
>> diff --git a/linux-user/errnos.c.inc b/linux-user/errnos.c.inc
>> new file mode 100644
>> index 00000000000..807c97ca25e
>> --- /dev/null
>> +++ b/linux-user/errnos.c.inc
>> @@ -0,0 +1,140 @@
> ...
>> +#ifdef ERKFILL
> 
> You fix it in patch 1 but forgot to report it in your patch :)

Grrr :(
diff mbox series

Patch

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 56682b06cbd..8bb528d2cf7 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -507,157 +507,37 @@  static inline int next_free_host_timer(void)
 }
 #endif
 
-#define ERRNO_TABLE_SIZE 1200
-
 static inline bool errno_exists(int err)
 {
-    return err >= 0 && err < ERRNO_TABLE_SIZE;
+    switch (err) {
+#define E(X)  case X: return true;
+#include "errnos.c.inc"
+#undef E
+    default:
+        return false;
+    }
 }
 
-/* target_to_host_errno_table[] is initialized from
- * host_to_target_errno_table[] in syscall_init(). */
-static uint16_t target_to_host_errno_table[ERRNO_TABLE_SIZE] = {
-};
-
-/*
- * This list is the union of errno values overridden in asm-<arch>/errno.h
- * minus the errnos that are not actually generic to all archs.
- */
-static uint16_t host_to_target_errno_table[ERRNO_TABLE_SIZE] = {
-    [EAGAIN]            = TARGET_EAGAIN,
-    [EIDRM]             = TARGET_EIDRM,
-    [ECHRNG]            = TARGET_ECHRNG,
-    [EL2NSYNC]          = TARGET_EL2NSYNC,
-    [EL3HLT]            = TARGET_EL3HLT,
-    [EL3RST]            = TARGET_EL3RST,
-    [ELNRNG]            = TARGET_ELNRNG,
-    [EUNATCH]           = TARGET_EUNATCH,
-    [ENOCSI]            = TARGET_ENOCSI,
-    [EL2HLT]            = TARGET_EL2HLT,
-    [EDEADLK]           = TARGET_EDEADLK,
-    [ENOLCK]            = TARGET_ENOLCK,
-    [EBADE]             = TARGET_EBADE,
-    [EBADR]             = TARGET_EBADR,
-    [EXFULL]            = TARGET_EXFULL,
-    [ENOANO]            = TARGET_ENOANO,
-    [EBADRQC]           = TARGET_EBADRQC,
-    [EBADSLT]           = TARGET_EBADSLT,
-    [EBFONT]            = TARGET_EBFONT,
-    [ENOSTR]            = TARGET_ENOSTR,
-    [ENODATA]           = TARGET_ENODATA,
-    [ETIME]             = TARGET_ETIME,
-    [ENOSR]             = TARGET_ENOSR,
-    [ENONET]            = TARGET_ENONET,
-    [ENOPKG]            = TARGET_ENOPKG,
-    [EREMOTE]           = TARGET_EREMOTE,
-    [ENOLINK]           = TARGET_ENOLINK,
-    [EADV]              = TARGET_EADV,
-    [ESRMNT]            = TARGET_ESRMNT,
-    [ECOMM]             = TARGET_ECOMM,
-    [EPROTO]            = TARGET_EPROTO,
-    [EDOTDOT]           = TARGET_EDOTDOT,
-    [EMULTIHOP]         = TARGET_EMULTIHOP,
-    [EBADMSG]           = TARGET_EBADMSG,
-    [ENAMETOOLONG]      = TARGET_ENAMETOOLONG,
-    [EOVERFLOW]         = TARGET_EOVERFLOW,
-    [ENOTUNIQ]          = TARGET_ENOTUNIQ,
-    [EBADFD]            = TARGET_EBADFD,
-    [EREMCHG]           = TARGET_EREMCHG,
-    [ELIBACC]           = TARGET_ELIBACC,
-    [ELIBBAD]           = TARGET_ELIBBAD,
-    [ELIBSCN]           = TARGET_ELIBSCN,
-    [ELIBMAX]           = TARGET_ELIBMAX,
-    [ELIBEXEC]          = TARGET_ELIBEXEC,
-    [EILSEQ]            = TARGET_EILSEQ,
-    [ENOSYS]            = TARGET_ENOSYS,
-    [ELOOP]             = TARGET_ELOOP,
-    [ERESTART]          = TARGET_ERESTART,
-    [ESTRPIPE]          = TARGET_ESTRPIPE,
-    [ENOTEMPTY]         = TARGET_ENOTEMPTY,
-    [EUSERS]            = TARGET_EUSERS,
-    [ENOTSOCK]          = TARGET_ENOTSOCK,
-    [EDESTADDRREQ]      = TARGET_EDESTADDRREQ,
-    [EMSGSIZE]          = TARGET_EMSGSIZE,
-    [EPROTOTYPE]        = TARGET_EPROTOTYPE,
-    [ENOPROTOOPT]       = TARGET_ENOPROTOOPT,
-    [EPROTONOSUPPORT]   = TARGET_EPROTONOSUPPORT,
-    [ESOCKTNOSUPPORT]   = TARGET_ESOCKTNOSUPPORT,
-    [EOPNOTSUPP]        = TARGET_EOPNOTSUPP,
-    [EPFNOSUPPORT]      = TARGET_EPFNOSUPPORT,
-    [EAFNOSUPPORT]      = TARGET_EAFNOSUPPORT,
-    [EADDRINUSE]        = TARGET_EADDRINUSE,
-    [EADDRNOTAVAIL]     = TARGET_EADDRNOTAVAIL,
-    [ENETDOWN]          = TARGET_ENETDOWN,
-    [ENETUNREACH]       = TARGET_ENETUNREACH,
-    [ENETRESET]         = TARGET_ENETRESET,
-    [ECONNABORTED]      = TARGET_ECONNABORTED,
-    [ECONNRESET]        = TARGET_ECONNRESET,
-    [ENOBUFS]           = TARGET_ENOBUFS,
-    [EISCONN]           = TARGET_EISCONN,
-    [ENOTCONN]          = TARGET_ENOTCONN,
-    [EUCLEAN]           = TARGET_EUCLEAN,
-    [ENOTNAM]           = TARGET_ENOTNAM,
-    [ENAVAIL]           = TARGET_ENAVAIL,
-    [EISNAM]            = TARGET_EISNAM,
-    [EREMOTEIO]         = TARGET_EREMOTEIO,
-    [EDQUOT]            = TARGET_EDQUOT,
-    [ESHUTDOWN]         = TARGET_ESHUTDOWN,
-    [ETOOMANYREFS]      = TARGET_ETOOMANYREFS,
-    [ETIMEDOUT]         = TARGET_ETIMEDOUT,
-    [ECONNREFUSED]      = TARGET_ECONNREFUSED,
-    [EHOSTDOWN]         = TARGET_EHOSTDOWN,
-    [EHOSTUNREACH]      = TARGET_EHOSTUNREACH,
-    [EALREADY]          = TARGET_EALREADY,
-    [EINPROGRESS]       = TARGET_EINPROGRESS,
-    [ESTALE]            = TARGET_ESTALE,
-    [ECANCELED]         = TARGET_ECANCELED,
-    [ENOMEDIUM]         = TARGET_ENOMEDIUM,
-    [EMEDIUMTYPE]       = TARGET_EMEDIUMTYPE,
-#ifdef ENOKEY
-    [ENOKEY]            = TARGET_ENOKEY,
-#endif
-#ifdef EKEYEXPIRED
-    [EKEYEXPIRED]       = TARGET_EKEYEXPIRED,
-#endif
-#ifdef EKEYREVOKED
-    [EKEYREVOKED]       = TARGET_EKEYREVOKED,
-#endif
-#ifdef EKEYREJECTED
-    [EKEYREJECTED]      = TARGET_EKEYREJECTED,
-#endif
-#ifdef EOWNERDEAD
-    [EOWNERDEAD]        = TARGET_EOWNERDEAD,
-#endif
-#ifdef ENOTRECOVERABLE
-    [ENOTRECOVERABLE]   = TARGET_ENOTRECOVERABLE,
-#endif
-#ifdef ENOMSG
-    [ENOMSG]            = TARGET_ENOMSG,
-#endif
-#ifdef ERFKILL
-    [ERFKILL]           = TARGET_ERFKILL,
-#endif
-#ifdef EHWPOISON
-    [EHWPOISON]         = TARGET_EHWPOISON,
-#endif
-};
-
-static inline int host_to_target_errno(int err)
+static inline int host_to_target_errno(int host_errno)
 {
-    if (err >= 0 && err < ERRNO_TABLE_SIZE &&
-        host_to_target_errno_table[err]) {
-        return host_to_target_errno_table[err];
+    switch (host_errno) {
+#define E(X)  case X: return TARGET_##X;
+#include "errnos.c.inc"
+#undef E
+    default:
+        return host_errno;
     }
-    return err;
 }
 
-static inline int target_to_host_errno(int err)
+static inline int target_to_host_errno(int target_errno)
 {
-    if (err >= 0 && err < ERRNO_TABLE_SIZE &&
-        target_to_host_errno_table[err]) {
-        return target_to_host_errno_table[err];
+    switch (target_errno) {
+#define E(X)  case TARGET_##X: return X;
+#include "errnos.c.inc"
+#undef E
+    default:
+        return target_errno;
     }
-    return err;
 }
 
 static inline abi_long get_errno(abi_long ret)
@@ -7107,7 +6987,6 @@  void syscall_init(void)
     IOCTLEntry *ie;
     const argtype *arg_type;
     int size;
-    int i;
 
     thunk_init(STRUCT_MAX);
 
@@ -7117,12 +6996,6 @@  void syscall_init(void)
 #undef STRUCT
 #undef STRUCT_SPECIAL
 
-    /* Build target_to_host_errno_table[] table from
-     * host_to_target_errno_table[]. */
-    for (i = 0; i < ERRNO_TABLE_SIZE; i++) {
-        target_to_host_errno_table[host_to_target_errno_table[i]] = i;
-    }
-
     /* we patch the ioctl size if necessary. We rely on the fact that
        no ioctl has all the bits at '1' in the size field */
     ie = ioctl_entries;
diff --git a/linux-user/errnos.c.inc b/linux-user/errnos.c.inc
new file mode 100644
index 00000000000..807c97ca25e
--- /dev/null
+++ b/linux-user/errnos.c.inc
@@ -0,0 +1,140 @@ 
+/*
+ * This list is the union of errno values overridden in asm-<arch>/errno.h
+ * minus the errnos that are not actually generic to all archs.
+ *
+ * Please keep this list sorted alphabetically.
+ *
+ *  Copyright (c) 2003 Fabrice Bellard
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program 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 General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, see <http://www.gnu.org/licenses/>.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+E(EADDRINUSE)
+E(EADDRNOTAVAIL)
+E(EADV)
+E(EAFNOSUPPORT)
+E(EAGAIN)
+E(EALREADY)
+E(EBADE)
+E(EBADFD)
+E(EBADMSG)
+E(EBADR)
+E(EBADRQC)
+E(EBADSLT)
+E(EBFONT)
+E(ECANCELED)
+E(ECHRNG)
+E(ECOMM)
+E(ECONNABORTED)
+E(ECONNREFUSED)
+E(ECONNRESET)
+E(EDEADLK)
+E(EDESTADDRREQ)
+E(EDOTDOT)
+E(EDQUOT)
+E(EHOSTDOWN)
+E(EHOSTUNREACH)
+#ifdef EHWPOISON
+E(EHWPOISON)
+#endif
+E(EIDRM)
+E(EILSEQ)
+E(EINPROGRESS)
+E(EISCONN)
+E(EISNAM)
+#ifdef EKEYEXPIRED
+E(EKEYEXPIRED)
+#endif
+#ifdef EKEYREJECTED
+E(EKEYREJECTED)
+#endif
+#ifdef EKEYREVOKED
+E(EKEYREVOKED)
+#endif
+E(EL2HLT)
+E(EL2NSYNC)
+E(EL3HLT)
+E(EL3RST)
+E(ELIBACC)
+E(ELIBBAD)
+E(ELIBEXEC)
+E(ELIBMAX)
+E(ELIBSCN)
+E(ELNRNG)
+E(ELOOP)
+E(EMEDIUMTYPE)
+E(EMSGSIZE)
+E(EMULTIHOP)
+E(ENAMETOOLONG)
+E(ENAVAIL)
+E(ENETDOWN)
+E(ENETRESET)
+E(ENETUNREACH)
+E(ENOANO)
+E(ENOBUFS)
+E(ENOCSI)
+E(ENODATA)
+#ifdef ENOKEY
+E(ENOKEY)
+#endif
+E(ENOLCK)
+E(ENOLINK)
+E(ENOMEDIUM)
+#ifdef ENOMSG
+E(ENOMSG)
+#endif
+E(ENONET)
+E(ENOPKG)
+E(ENOPROTOOPT)
+E(ENOSR)
+E(ENOSTR)
+E(ENOSYS)
+E(ENOTCONN)
+E(ENOTEMPTY)
+E(ENOTNAM)
+#ifdef ENOTRECOVERABLE
+E(ENOTRECOVERABLE)
+#endif
+E(ENOTSOCK)
+E(ENOTUNIQ)
+E(EOPNOTSUPP)
+E(EOVERFLOW)
+#ifdef EOWNERDEAD
+E(EOWNERDEAD)
+#endif
+E(EPFNOSUPPORT)
+E(EPROTO)
+E(EPROTONOSUPPORT)
+E(EPROTOTYPE)
+E(EREMCHG)
+E(EREMOTE)
+E(EREMOTEIO)
+E(ERESTART)
+#ifdef ERKFILL
+E(ERFKILL)
+#endif
+E(ESHUTDOWN)
+E(ESOCKTNOSUPPORT)
+E(ESRMNT)
+E(ESTALE)
+E(ESTRPIPE)
+E(ETIME)
+E(ETIMEDOUT)
+E(ETOOMANYREFS)
+E(EUCLEAN)
+E(EUNATCH)
+E(EUSERS)
+E(EXFULL)