Re: [Qemu-devel] [PATCH v2 35/41] postcopy: introduce helper functions for postcopy

[Juan Quintela <quintela@redhat.com>]

Isaku Yamahata <yamahata@valinux.co.jp> wrote:
> +//#define DEBUG_UMEM
> +#ifdef DEBUG_UMEM
> +#include <sys/syscall.h>
> +#define DPRINTF(format, ...)                                            \
> +    do {                                                                \
> +        printf("%d:%ld %s:%d "format, getpid(), syscall(SYS_gettid),    \
> +               __func__, __LINE__, ## __VA_ARGS__);                     \
> +    } while (0)

This should be in a header file that is linux specific?  And (at least
on my systems) gettid is already defined on glibc.


> +#else
> +#define DPRINTF(format, ...)    do { } while (0)
> +#endif


> +
> +#define DEV_UMEM        "/dev/umem"
> +
> +UMem *umem_new(void *hostp, size_t size)
> +{
> +    struct umem_init uinit = {
> +        .size = size,
> +    };
> +    UMem *umem;
> +
> +    assert((size % getpagesize()) == 0);
> +    umem = g_new(UMem, 1);
> +    umem->fd = open(DEV_UMEM, O_RDWR);
> +    if (umem->fd < 0) {
> +        perror("can't open "DEV_UMEM);
> +        abort();

Can we return one error insntead of abort?  the same for the rest of the
file aborts.


> +size_t umem_pages_size(uint64_t nr)
> +{
> +    return sizeof(struct umem_pages) + nr * sizeof(uint64_t);

Can we make sure that the pgoffs field is aligned?  I know that as it is
now it is aligned, but better to be sure?

> +}
> +
> +static void umem_write_cmd(int fd, uint8_t cmd)
> +{
> +    DPRINTF("write cmd %c\n", cmd);
> +
> +    for (;;) {
> +        ssize_t ret = write(fd, &cmd, 1);
> +        if (ret == -1) {
> +            if (errno == EINTR) {
> +                continue;
> +            } else if (errno == EPIPE) {
> +                perror("pipe");
> +                DPRINTF("write cmd %c %zd %d: pipe is closed\n",
> +                        cmd, ret, errno);
> +                break;
> +            }


Grr, we don't have a function that writes does a "safe_write".  The most
similar thing in qemu looks to be send_all().

> +
> +            perror("pipe");

Can we make a different perror() message than previous error?

> +            DPRINTF("write cmd %c %zd %d\n", cmd, ret, errno);
> +            abort();
> +        }
> +
> +        break;
> +    }
> +}
> +
> +static void umem_read_cmd(int fd, uint8_t expect)
> +{
> +    uint8_t cmd;
> +    for (;;) {
> +        ssize_t ret = read(fd, &cmd, 1);
> +        if (ret == -1) {
> +            if (errno == EINTR) {
> +                continue;
> +            }
> +            perror("pipe");
> +            DPRINTF("read error cmd %c %zd %d\n", cmd, ret, errno);
> +            abort();
> +        }
> +
> +        if (ret == 0) {
> +            DPRINTF("read cmd %c %zd: pipe is closed\n", cmd, ret);
> +            abort();
> +        }
> +
> +        break;
> +    }
> +
> +    DPRINTF("read cmd %c\n", cmd);
> +    if (cmd != expect) {
> +        DPRINTF("cmd %c expect %d\n", cmd, expect);
> +        abort();

Ouch.  If we receive garbage, we just exit?

I really think that we should implement error handling.

> +    }
> +}
> +
> +struct umem_pages *umem_recv_pages(QEMUFile *f, int *offset)
> +{
> +    int ret;
> +    uint64_t nr;
> +    size_t size;
> +    struct umem_pages *pages;
> +
> +    ret = qemu_peek_buffer(f, (uint8_t*)&nr, sizeof(nr), *offset);
> +    *offset += sizeof(nr);
> +    DPRINTF("ret %d nr %ld\n", ret, nr);
> +    if (ret != sizeof(nr) || nr == 0) {
> +        return NULL;
> +    }
> +
> +    size = umem_pages_size(nr);
> +    pages = g_malloc(size);

Just thinking about this.  Couldn't we just decide on a "big enough"
buffer, and never send anything bigger than that?  That would remove the
need to have to malloc()/free() a buffer for each reception?



> +/* qemu side handler */
> +struct umem_pages *umem_qemu_trigger_page_fault(QEMUFile *from_umemd,
> +                                                int *offset)
> +{
> +    uint64_t i;
> +    int page_shift = ffs(getpagesize()) - 1;
> +    struct umem_pages *pages = umem_recv_pages(from_umemd, offset);
> +    if (pages == NULL) {
> +        return NULL;
> +    }
> +
> +    for (i = 0; i < pages->nr; i++) {
> +        ram_addr_t addr = pages->pgoffs[i] << page_shift;
> +
> +        /* make pages present by forcibly triggering page fault. */
> +        volatile uint8_t *ram = qemu_get_ram_ptr(addr);
> +        uint8_t dummy_read = ram[0];
> +        (void)dummy_read;   /* suppress unused variable warning */
> +    }
> +
> +    /*
> +     * Very Linux implementation specific.
> +     * Make it sure that other thread doesn't fault on the above virtual
> +     * address. (More exactly other thread doesn't call fault handler with
> +     * the offset.)
> +     * the fault handler is called with mmap_sem read locked.
> +     * madvise() does down/up_write(mmap_sem)
> +     */
> +    qemu_madvise(NULL, 0, MADV_NORMAL);

If it is linux specific, should be inside CONFIG_LINUX ifdef, or a
function hided on some header.

Talking about looking, what protects that no other thread enters this
function before this one calls madvise?   Or I am losing something obvious?

> +
> +struct umem_pages {
> +    uint64_t nr;
> +    uint64_t pgoffs[0];
> +};
> +

QEMU really likes typedefs for structs.

Later, Juan.