Re: [Qemu-devel] [PATCH 27/38] ivshmem: Simplify how we cope with short reads from server

[Marc-André Lureau]

Hi

On Mon, Feb 29, 2016 at 7:40 PM, Markus Armbruster <address@hidden> wrote:
> Short reads from a UNIX domain sockets are exceedingly unlikely when
> the other side always sends eight bytes and we always read eight
> bytes.  We cope with them anyway.  However, the code doing that is
> rather convoluted.  Dumb it down radically.
>
> Replace the convoluted code

agreed

>
> Signed-off-by: Markus Armbruster <address@hidden>
> ---
>  hw/misc/ivshmem.c | 76 
> ++++++++++++-------------------------------------------
>  1 file changed, 16 insertions(+), 60 deletions(-)
>
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index e578b8a..fb8a4f7 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -26,7 +26,6 @@
>  #include "migration/migration.h"
>  #include "qemu/error-report.h"
>  #include "qemu/event_notifier.h"
> -#include "qemu/fifo8.h"
>  #include "sysemu/char.h"
>  #include "sysemu/hostmem.h"
>  #include "qapi/visitor.h"
> @@ -80,7 +79,6 @@ typedef struct IVShmemState {
>      uint32_t intrstatus;
>
>      CharDriverState *server_chr;
> -    Fifo8 incoming_fifo;
>      MemoryRegion ivshmem_mmio;
>
>      /* We might need to register the BAR before we actually have the memory.
> @@ -99,6 +97,8 @@ typedef struct IVShmemState {
>      uint32_t vectors;
>      uint32_t features;
>      MSIVector *msi_vectors;
> +    uint64_t msg_buf;           /* buffer for receiving server messages */
> +    int msg_buffered_bytes;     /* #bytes in @msg_buf */
>
>      Error *migration_blocker;
>
> @@ -255,11 +255,6 @@ static const MemoryRegionOps ivshmem_mmio_ops = {
>      },
>  };
>
> -static int ivshmem_can_receive(void * opaque)
> -{
> -    return sizeof(int64_t);
> -}
> -
>  static void ivshmem_vector_notify(void *opaque)
>  {
>      MSIVector *entry = opaque;
> @@ -459,53 +454,6 @@ static void resize_peers(IVShmemState *s, int nb_peers)
>      }
>  }
>
> -static bool fifo_update_and_get(IVShmemState *s, const uint8_t *buf, int 
> size,
> -                                void *data, size_t len)
> -{
> -    const uint8_t *p;
> -    uint32_t num;
> -
> -    assert(len <= sizeof(int64_t)); /* limitation of the fifo */
> -    if (fifo8_is_empty(&s->incoming_fifo) && size == len) {
> -        memcpy(data, buf, size);
> -        return true;
> -    }
> -
> -    IVSHMEM_DPRINTF("short read of %d bytes\n", size);
> -
> -    num = MIN(size, sizeof(int64_t) - fifo8_num_used(&s->incoming_fifo));
> -    fifo8_push_all(&s->incoming_fifo, buf, num);
> -
> -    if (fifo8_num_used(&s->incoming_fifo) < len) {
> -        assert(num == 0);
> -        return false;
> -    }
> -
> -    size -= num;
> -    buf += num;
> -    p = fifo8_pop_buf(&s->incoming_fifo, len, &num);
> -    assert(num == len);
> -
> -    memcpy(data, p, len);
> -
> -    if (size > 0) {
> -        fifo8_push_all(&s->incoming_fifo, buf, size);
> -    }
> -
> -    return true;
> -}
> -
> -static bool fifo_update_and_get_i64(IVShmemState *s,
> -                                    const uint8_t *buf, int size, int64_t 
> *i64)
> -{
> -    if (fifo_update_and_get(s, buf, size, i64, sizeof(*i64))) {
> -        *i64 = GINT64_FROM_LE(*i64);
> -        return true;
> -    }
> -
> -    return false;
> -}
> -
>  static void ivshmem_add_kvm_msi_virq(IVShmemState *s, int vector,
>                                       Error **errp)
>  {
> @@ -658,6 +606,14 @@ static void process_msg(IVShmemState *s, int64_t msg, 
> int fd, Error **errp)
>      }
>  }
>
> +static int ivshmem_can_receive(void *opaque)
> +{
> +    IVShmemState *s = opaque;
> +
> +    assert(s->msg_buffered_bytes < sizeof(s->msg_buf));
> +    return sizeof(s->msg_buf) - s->msg_buffered_bytes;
> +}
> +
>  static void ivshmem_read(void *opaque, const uint8_t *buf, int size)
>  {
>      IVShmemState *s = opaque;
> @@ -665,8 +621,12 @@ static void ivshmem_read(void *opaque, const uint8_t 
> *buf, int size)
>      int incoming_fd;
>      int64_t incoming_posn;
>
> -    if (!fifo_update_and_get_i64(s, buf, size, &incoming_posn)) {
> -        return;
> +    assert(size >= 0 && s->msg_buffered_bytes + size <= sizeof(s->msg_buf));
> +    memcpy((unsigned char *)&s->msg_buf + s->msg_buffered_bytes, buf, size);
> +    s->msg_buffered_bytes += size;
> +    if (s->msg_buffered_bytes == sizeof(s->msg_buf)) {
> +        incoming_posn = le64_to_cpu(s->msg_buf);
> +        s->msg_buffered_bytes = 0;
>      }
>

missing "else return" though.

>      incoming_fd = qemu_chr_fe_get_msgfd(s->server_chr);
> @@ -1019,8 +979,6 @@ static void pci_ivshmem_realize(PCIDevice *dev, Error 
> **errp)
>          }
>      }
>
> -    fifo8_create(&s->incoming_fifo, sizeof(int64_t));
> -
>      if (s->role_val == IVSHMEM_PEER) {
>          error_setg(&s->migration_blocker,
>                     "Migration is disabled when using feature 'peer mode' in 
> device 'ivshmem'");
> @@ -1033,8 +991,6 @@ static void pci_ivshmem_exit(PCIDevice *dev)
>      IVShmemState *s = IVSHMEM(dev);
>      int i;
>
> -    fifo8_destroy(&s->incoming_fifo);
> -
>      if (s->migration_blocker) {
>          migrate_del_blocker(s->migration_blocker);
>          error_free(s->migration_blocker);
> --
> 2.4.3
>
>



-- 
Marc-André Lureau