Re: [PATCH gnumach] x86_64: avoid iterating over the message twice in copyoutmsg/copyinmsg for faster RPCs.

[Samuel Thibault]

Applied, thanks!

Flavio Cruz, le lun. 19 févr. 2024 23:58:00 -0500, a ecrit:
> This is a follow up to
> https://git.savannah.gnu.org/cgit/hurd/gnumach.git/commit/?id=69620634858b2992e1a362e33c95d9a8ee57bce7
> where we made inlined ports 8 bytes long to avoid resizing.
> 
> The last thing that copy{in,out}msg were doing was just updating
> msgt_size field since that's required for kernel stub code and implicitly
> assumed by IPC code. This was moved into ipc_kmsg_copy{in,out}_body.
> 
> For a 32 bit userland, the code also stops updating
> msgt_size for out of line ports, same as the 64 bit userland.
> ---
>  i386/i386/locore.h |   6 +++
>  ipc/ipc_kmsg.c     |  52 +++++++++++++------
>  x86_64/copy_user.c | 123 +++++++++------------------------------------
>  3 files changed, 66 insertions(+), 115 deletions(-)
> 
> diff --git a/i386/i386/locore.h b/i386/i386/locore.h
> index 374c8cf..217e6de 100644
> --- a/i386/i386/locore.h
> +++ b/i386/i386/locore.h
> @@ -50,7 +50,13 @@ extern int discover_x86_cpu_type (void);
>  extern int copyin (const void *userbuf, void *kernelbuf, size_t cn);
>  extern int copyinmsg (const void *userbuf, void *kernelbuf, size_t cn, 
> size_t kn);
>  extern int copyout (const void *kernelbuf, void *userbuf, size_t cn);
> +#ifdef USER32
>  extern int copyoutmsg (const void *kernelbuf, void *userbuf, size_t cn);
> +#else
> +static inline int copyoutmsg (const void *kernelbuf, void *userbuf, size_t 
> cn) {
> +     return copyout (kernelbuf, userbuf, cn);
> +}
> +#endif
>  
>  extern int inst_fetch (int eip, int cs);
>  
> diff --git a/ipc/ipc_kmsg.c b/ipc/ipc_kmsg.c
> index bd84380..6a7ad3c 100644
> --- a/ipc/ipc_kmsg.c
> +++ b/ipc/ipc_kmsg.c
> @@ -1321,7 +1321,7 @@ ipc_kmsg_copyin_body(
>               mach_msg_type_number_t number;
>               boolean_t is_inline, longform, dealloc, is_port;
>               vm_offset_t data;
> -             uint64_t length;
> +             vm_size_t length;
>               kern_return_t kr;
>  
>               type = (mach_msg_type_long_t *) saddr;
> @@ -1355,7 +1355,8 @@ ipc_kmsg_copyin_body(
>  
>               is_port = MACH_MSG_TYPE_PORT_ANY(name);
>  
> -             if ((is_port && (size != PORT_T_SIZE_IN_BITS)) ||
> +             if ((is_port && !is_inline && (size != 
> PORT_NAME_T_SIZE_IN_BITS)) ||
> +                 (is_port && is_inline && (size != PORT_T_SIZE_IN_BITS)) ||
>  #ifndef __x86_64__
>                   (longform && ((type->msgtl_header.msgt_name != 0) ||
>                                 (type->msgtl_header.msgt_size != 0) ||
> @@ -1396,11 +1397,22 @@ ipc_kmsg_copyin_body(
>                       if (length == 0)
>                               data = 0;
>                       else if (is_port) {
> +                             const vm_size_t user_length = length;
> +                             /*
> +                              * In 64 bit architectures, out of line port 
> names are
> +                              * represented as an array of mach_port_name_t 
> which are
> +                              * smaller than mach_port_t.
> +                              */
> +                             if (sizeof(mach_port_name_t) != 
> sizeof(mach_port_t)) {
> +                                     length = sizeof(mach_port_t) * number;
> +                                     type->msgtl_size = sizeof(mach_port_t) 
> * 8;
> +                             }
> +
>                               data = kalloc(length);
>                               if (data == 0)
>                                       goto invalid_memory;
>  
> -                             if (sizeof(mach_port_name_t) != 
> sizeof(mach_port_t))
> +                             if (user_length != length)
>                               {
>                                       mach_port_name_t *src = 
> (mach_port_name_t*)addr;
>                                       mach_port_t *dst = (mach_port_t*)data;
> @@ -1416,7 +1428,7 @@ ipc_kmsg_copyin_body(
>                                       goto invalid_memory;
>                               }
>                               if (dealloc &&
> -                                 (vm_deallocate(map, addr, length) != 
> KERN_SUCCESS)) {
> +                                 (vm_deallocate(map, addr, user_length) != 
> KERN_SUCCESS)) {
>                                       kfree(data, length);
>                                       goto invalid_memory;
>                               }
> @@ -2372,7 +2384,7 @@ ipc_kmsg_copyout_body(
>               mach_msg_type_size_t size;
>               mach_msg_type_number_t number;
>               boolean_t is_inline, longform, is_port;
> -             uint64_t length;
> +             vm_size_t length;
>               vm_offset_t addr;
>  
>               type = (mach_msg_type_long_t *) saddr;
> @@ -2406,18 +2418,28 @@ ipc_kmsg_copyout_body(
>                       ipc_object_t *objects;
>                       mach_msg_type_number_t i;
>  
> -                     if (!is_inline && (length != 0)) {
> -                             /* first allocate memory in the map */
> -                             uint64_t allocated = length;
> +                     if (!is_inline) {
> +                             if (length != 0) {
> +                                     vm_size_t user_length = length;
>  
> -                             _Static_assert(sizeof(mach_port_name_t) <= 
> sizeof(mach_port_t),
> -                                             "Size of mach_port_t should be 
> equal or larger than mach_port_name_t.");
> -                             allocated -= (sizeof(mach_port_t) - 
> sizeof(mach_port_name_t)) * number;
> +                                     if (sizeof(mach_port_name_t) != 
> sizeof(mach_port_t)) {
> +                                             user_length = 
> sizeof(mach_port_name_t) * number;
> +                                     }
>  
> -                             kr = vm_allocate(map, &addr, allocated, TRUE);
> -                             if (kr != KERN_SUCCESS) {
> -                                     ipc_kmsg_clean_body(taddr, saddr);
> -                                     goto vm_copyout_failure;
> +                                     /* first allocate memory in the map */
> +                                     kr = vm_allocate(map, &addr, 
> user_length, TRUE);
> +                                     if (kr != KERN_SUCCESS) {
> +                                             ipc_kmsg_clean_body(taddr, 
> saddr);
> +                                             goto vm_copyout_failure;
> +                                     }
> +                             }
> +
> +                             if (sizeof(mach_port_name_t) != 
> sizeof(mach_port_t)) {
> +                                     /* Out of line ports are always 
> returned as mach_port_name_t.
> +                                      * Note: we have to do this after 
> ipc_kmsg_clean_body, otherwise
> +                                      * the cleanup function will not work 
> correctly.
> +                                      */
> +                                     type->msgtl_size = 
> sizeof(mach_port_name_t) * 8;
>                               }
>                       }
>  
> diff --git a/x86_64/copy_user.c b/x86_64/copy_user.c
> index c6e125d..89acd81 100644
> --- a/x86_64/copy_user.c
> +++ b/x86_64/copy_user.c
> @@ -49,10 +49,6 @@ typedef struct {
>    natural_t msgtl_number;
>  } mach_msg_user_type_long_t;
>  _Static_assert(sizeof(mach_msg_user_type_long_t) == 12);
> -#else
> -typedef mach_msg_type_t mach_msg_user_type_t;
> -typedef mach_msg_type_long_t mach_msg_user_type_long_t;
> -#endif  /* USER32 */
>  
>  /*
>  * Helper to unpack the relevant fields of a msg type; the fields are 
> different
> @@ -87,7 +83,6 @@ static inline void unpack_msg_type(vm_offset_t addr,
>      }
>  }
>  
> -#ifdef USER32
>  static inline void mach_msg_user_type_to_kernel(const mach_msg_user_type_t 
> *u,
>      mach_msg_type_t* k) {
>    k->msgt_name = u->msgt_name;
> @@ -144,53 +139,33 @@ static inline void 
> mach_msg_kernel_type_to_user_long(const mach_msg_type_long_t
>    };
>    *u = user;
>  }
> -#endif
>  
>  static inline int copyin_mach_msg_type(const rpc_vm_offset_t *uaddr, 
> mach_msg_type_t *kaddr) {
> -#ifdef USER32
>    mach_msg_user_type_t user;
> -  int ret = copyin(uaddr, &user, sizeof(mach_msg_user_type_t));
> -  if (ret) {
> -    return ret;
> -  }
> +  if (copyin(uaddr, &user, sizeof(mach_msg_user_type_t)))
> +    return 1;
>    mach_msg_user_type_to_kernel(&user, kaddr);
>    return 0;
> -#else
> -  return copyin(uaddr, kaddr, sizeof(mach_msg_type_t));
> -#endif
>  }
>  
>  static inline int copyout_mach_msg_type(const mach_msg_type_t *kaddr, 
> rpc_vm_offset_t  *uaddr) {
> -#ifdef USER32
>    mach_msg_user_type_t user;
>    mach_msg_kernel_type_to_user(kaddr, &user);
>    return copyout(&user, uaddr, sizeof(mach_msg_user_type_t));
> -#else
> -  return copyout(kaddr, uaddr, sizeof(mach_msg_type_t));
> -#endif
>  }
>  
>  static inline int copyin_mach_msg_type_long(const rpc_vm_offset_t *uaddr, 
> mach_msg_type_long_t *kaddr) {
> -#ifdef USER32
>    mach_msg_user_type_long_t user;
> -  int ret = copyin(uaddr, &user, sizeof(mach_msg_user_type_long_t));
> -  if (ret)
> -    return ret;
> +  if (copyin(uaddr, &user, sizeof(mach_msg_user_type_long_t)))
> +    return 1;
>    mach_msg_user_type_to_kernel_long(&user, kaddr);
>    return 0;
> -#else
> -  return copyin(uaddr, kaddr, sizeof(mach_msg_type_long_t));
> -#endif
>  }
>  
>  static inline int copyout_mach_msg_type_long(const mach_msg_type_long_t 
> *kaddr, rpc_vm_offset_t *uaddr) {
> -#ifdef USER32
>    mach_msg_user_type_long_t user;
>    mach_msg_kernel_type_to_user_long(kaddr, &user);
>    return copyout(&user, uaddr, sizeof(mach_msg_user_type_long_t));
> -#else
> -  return copyout(kaddr, uaddr, sizeof(mach_msg_type_long_t));
> -#endif
>  }
>  
>  /* Optimized version of unpack_msg_type(), including proper copyin() */
> @@ -265,15 +240,8 @@ static inline int copyout_unpack_msg_type(vm_offset_t 
> kaddr,
>        mach_msg_type_size_t orig_size = kmtl->msgtl_size;
>        int ret;
>  
> -      if (MACH_MSG_TYPE_PORT_ANY(kmtl->msgtl_name)) {
> -#ifdef USER32
> -        kmtl->msgtl_size = bytes_to_descsize(sizeof(mach_port_name_t));
> -#else
> -        /* 64 bit ABI uses mach_port_name_inlined_t for inlined ports. */
> -        if (!kmt->msgt_inline)
> +      if (MACH_MSG_TYPE_PORT_ANY(kmtl->msgtl_name) && kmt->msgt_inline)
>            kmtl->msgtl_size = bytes_to_descsize(sizeof(mach_port_name_t));
> -#endif
> -      }
>        ret = copyout_mach_msg_type_long(kmtl, (void*)uaddr);
>        kmtl->msgtl_size = orig_size;
>        if (ret)
> @@ -289,21 +257,12 @@ static inline int copyout_unpack_msg_type(vm_offset_t 
> kaddr,
>      {
>        mach_msg_type_size_t orig_size = kmt->msgt_size;
>        int ret;
> -
> -      if (MACH_MSG_TYPE_PORT_ANY(kmt->msgt_name)) {
> -#ifdef USER32
> +      if (MACH_MSG_TYPE_PORT_ANY(kmt->msgt_name) && kmt->msgt_inline)
>          kmt->msgt_size = bytes_to_descsize(sizeof(mach_port_name_t));
> -#else
> -        /* 64 bit ABI uses mach_port_name_inlined_t for inlined ports. */
> -        if (!kmt->msgt_inline)
> -          kmt->msgt_size = bytes_to_descsize(sizeof(mach_port_name_t));
> -#endif
> -      }
>        ret = copyout_mach_msg_type(kmt, (void *)uaddr);
>        kmt->msgt_size = orig_size;
>        if (ret)
>          return 1;
> -
>        *name = kmt->msgt_name;
>        *size = kmt->msgt_size;
>        *number = kmt->msgt_number;
> @@ -313,7 +272,6 @@ static inline int copyout_unpack_msg_type(vm_offset_t 
> kaddr,
>    return 0;
>  }
>  
> -#ifdef USER32
>  /*
>   * Compute the user-space size of a message still in the kernel when 
> processing
>   * messages from 32bit userland.
> @@ -372,7 +330,7 @@ size_t msg_usize(const mach_msg_header_t *kmsg)
>      }
>    return usize;
>  }
> -#endif /* USER32 */
> +#endif  /* USER32 */
>  
>  /*
>   * Expand the msg header and, if required, the msg body (ports, pointers)
> @@ -386,6 +344,9 @@ int copyinmsg (const void *userbuf, void *kernelbuf, 
> const size_t usize, const s
>    const mach_msg_user_header_t *umsg = userbuf;
>    mach_msg_header_t *kmsg = kernelbuf;
>  
> +  
> _Static_assert(!mach_msg_user_is_misaligned(sizeof(mach_msg_user_header_t)),
> +                 "mach_msg_user_header_t needs to be MACH_MSG_USER_ALIGNMENT 
> aligned.");
> +
>  #ifdef USER32
>    if (copyin(&umsg->msgh_bits, &kmsg->msgh_bits, sizeof(kmsg->msgh_bits)))
>      return 1;
> @@ -397,23 +358,12 @@ int copyinmsg (const void *userbuf, void *kernelbuf, 
> const size_t usize, const s
>    if (copyin(&umsg->msgh_seqno, &kmsg->msgh_seqno,
>               sizeof(kmsg->msgh_seqno) + sizeof(kmsg->msgh_id)))
>      return 1;
> -#else
> -  /* The 64 bit interface ensures the header is the same size, so it does 
> not need any resizing. */
> -  _Static_assert(sizeof(mach_msg_header_t) == sizeof(mach_msg_user_header_t),
> -              "mach_msg_header_t and mach_msg_user_header_t expected to be 
> of the same size");
> -  if (copyin(umsg, kmsg, sizeof(mach_msg_header_t)))
> -    return 1;
> -  kmsg->msgh_remote_port &= 0xFFFFFFFF; // FIXME: still have port names here
> -  kmsg->msgh_local_port &= 0xFFFFFFFF;  // also, this assumes little-endian
> -#endif
>  
>    vm_offset_t usaddr, ueaddr, ksaddr;
>    ksaddr = (vm_offset_t)(kmsg + 1);
>    usaddr = (vm_offset_t)(umsg + 1);
>    ueaddr = (vm_offset_t)umsg + usize;
>  
> -  
> _Static_assert(!mach_msg_user_is_misaligned(sizeof(mach_msg_user_header_t)),
> -                 "mach_msg_user_header_t needs to be MACH_MSG_USER_ALIGNMENT 
> aligned.");
>  
>    if (usize > sizeof(mach_msg_user_header_t))
>      {
> @@ -441,7 +391,6 @@ int copyinmsg (const void *userbuf, void *kernelbuf, 
> const size_t usize, const s
>              {
>                if (MACH_MSG_TYPE_PORT_ANY(name))
>                  {
> -#ifdef USER32
>                    if (size != bytes_to_descsize(sizeof(mach_port_name_t)))
>                      return 1;
>                    if ((usaddr + sizeof(mach_port_name_t)*number) > ueaddr)
> @@ -454,17 +403,6 @@ int copyinmsg (const void *userbuf, void *kernelbuf, 
> const size_t usize, const s
>                        ksaddr += sizeof(mach_port_t);
>                        usaddr += sizeof(mach_port_name_t);
>                      }
> -#else
> -                  if (size != 
> bytes_to_descsize(sizeof(mach_port_name_inlined_t)))
> -                    return 1;
> -                  const vm_size_t length = number * 
> sizeof(mach_port_name_inlined_t);
> -                  if ((usaddr + length) > ueaddr)
> -                    return 1;
> -                  if (copyin((void*)usaddr, (void*)ksaddr, length))
> -                    return 1;
> -                  usaddr += length;
> -                  ksaddr += length;
> -#endif
>                  }
>                else
>                  {
> @@ -485,11 +423,9 @@ int copyinmsg (const void *userbuf, void *kernelbuf, 
> const size_t usize, const s
>  
>                /* out-of-line port arrays are always arrays of 
> mach_port_name_t (4 bytes)
>                 * and are expanded in ipc_kmsg_copyin_body() */
> -              if (MACH_MSG_TYPE_PORT_ANY(name)) {
> -                if (size != bytes_to_descsize(sizeof(mach_port_name_t)))
> -                  return 1;
> -                adjust_msg_type_size(ktaddr, sizeof(mach_port_t) - 
> sizeof(mach_port_name_t));
> -              }
> +              if (MACH_MSG_TYPE_PORT_ANY(name) &&
> +                  (size != bytes_to_descsize(sizeof(mach_port_name_t))))
> +                return 1;
>  
>                if (copyin_address((rpc_vm_offset_t*)usaddr, 
> (vm_offset_t*)ksaddr))
>                  return 1;
> @@ -506,18 +442,24 @@ int copyinmsg (const void *userbuf, void *kernelbuf, 
> const size_t usize, const s
>  
>    kmsg->msgh_size = sizeof(mach_msg_header_t) + ksaddr - (vm_offset_t)(kmsg 
> + 1);
>    assert(kmsg->msgh_size <= ksize);
> -#ifndef USER32
> -  if (kmsg->msgh_size != usize)
> +#else
> +  /* The 64 bit interface ensures the header is the same size, so it does 
> not need any resizing. */
> +  _Static_assert(sizeof(mach_msg_header_t) == sizeof(mach_msg_user_header_t),
> +              "mach_msg_header_t and mach_msg_user_header_t expected to be 
> of the same size");
> +  if (copyin(umsg, kmsg, usize))
>      return 1;
> +  kmsg->msgh_remote_port &= 0xFFFFFFFF; // FIXME: still have port names here
> +  kmsg->msgh_local_port &= 0xFFFFFFFF;  // also, this assumes little-endian
>  #endif
>    return 0;
>  }
>  
> +#ifdef USER32
> +/* This is defined simply as copyout for !USER32.  */
>  int copyoutmsg (const void *kernelbuf, void *userbuf, const size_t ksize)
>  {
>    const mach_msg_header_t *kmsg = kernelbuf;
>    mach_msg_user_header_t *umsg = userbuf;
> -#ifdef USER32
>    if (copyout(&kmsg->msgh_bits, &umsg->msgh_bits, sizeof(kmsg->msgh_bits)))
>      return 1;
>    /* umsg->msgh_size is filled in later */
> @@ -528,10 +470,6 @@ int copyoutmsg (const void *kernelbuf, void *userbuf, 
> const size_t ksize)
>    if (copyout(&kmsg->msgh_seqno, &umsg->msgh_seqno,
>               sizeof(kmsg->msgh_seqno) + sizeof(kmsg->msgh_id)))
>      return 1;
> -#else
> -  if (copyout(kmsg, umsg, sizeof(mach_msg_header_t)))
> -    return 1;
> -#endif  /* USER32 */
>  
>    vm_offset_t ksaddr, keaddr, usaddr;
>    ksaddr = (vm_offset_t)(kmsg + 1);
> @@ -559,7 +497,6 @@ int copyoutmsg (const void *kernelbuf, void *userbuf, 
> const size_t ksize)
>              {
>                if (MACH_MSG_TYPE_PORT_ANY(name))
>                  {
> -#ifdef USER32
>                    for (int i=0; i<number; i++)
>                      {
>                        if (copyout_port((mach_port_t*)ksaddr, 
> (mach_port_name_t*)usaddr))
> @@ -567,15 +504,6 @@ int copyoutmsg (const void *kernelbuf, void *userbuf, 
> const size_t ksize)
>                        ksaddr += sizeof(mach_port_t);
>                        usaddr += sizeof(mach_port_name_t);
>                      }
> -#else
> -                  if (size != 
> bytes_to_descsize(sizeof(mach_port_name_inlined_t)))
> -                    return 1;
> -                  const vm_size_t length = number * 
> sizeof(mach_port_name_inlined_t);
> -                  if (copyout((void*)ksaddr, (void*)usaddr, length))
> -                    return 1;
> -                  ksaddr += length;
> -                  usaddr += length;
> -#endif
>                  }
>                else
>                  {
> @@ -603,11 +531,6 @@ int copyoutmsg (const void *kernelbuf, void *userbuf, 
> const size_t ksize)
>    usize = sizeof(mach_msg_user_header_t) + usaddr - (vm_offset_t)(umsg + 1);
>    if (copyout(&usize, &umsg->msgh_size, sizeof(umsg->msgh_size)))
>      return 1;
> -#ifndef USER32
> -  if (usize != ksize)
> -    return 1;
> -#endif
> -
>    return 0;
> -
>  }
> +#endif  /* USER32 */
> -- 
> 2.39.2
> 
> 

-- 
Samuel
---
Pour une évaluation indépendante, transparente et rigoureuse !
Je soutiens la Commission d'Évaluation de l'Inria.