[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH RESEND v6 15/36] multi-process: setup memory manager for remo
From: |
Stefan Hajnoczi |
Subject: |
Re: [PATCH RESEND v6 15/36] multi-process: setup memory manager for remote device |
Date: |
Tue, 12 May 2020 13:11:37 +0100 |
On Wed, Apr 22, 2020 at 09:13:50PM -0700, address@hidden wrote:
> diff --git a/exec.c b/exec.c
> index 5b1e414099..1e02e00f00 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2371,6 +2371,23 @@ RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size,
> MemoryRegion *mr,
>
> return block;
> }
> +
> +void qemu_ram_init_from_fd(MemoryRegion *mr, int fd, uint64_t size,
This looks like a memory_region_init_*() function, not a qemu_ram_*()
function. Why is it being added to exec.c instead of memory.c?
> + ram_addr_t offset, Error **errp)
qemu_ram_alloc_from_fd()'s offset argument has the off_t type. Why is
ram_addr_t used here?
> +typedef struct {
> + hwaddr gpas[REMOTE_MAX_FDS];
> + uint64_t sizes[REMOTE_MAX_FDS];
> + ram_addr_t offsets[REMOTE_MAX_FDS];
Should this be off_t because it's the file offset, not a RAMBlock
address?
> +} sync_sysmem_msg_t;
QEMU coding style would name this struct SyncSysMemMsg.
> +void remote_sysmem_reconfig(MPQemuMsg *msg, Error **errp)
> +{
> + sync_sysmem_msg_t *sysmem_info = &msg->data1.sync_sysmem;
> + MemoryRegion *sysmem, *subregion, *next;
> + Error *local_err = NULL;
> + int region;
> +
> + sysmem = get_system_memory();
> +
> + qemu_mutex_lock_iothread();
> +
> + memory_region_transaction_begin();
> +
> + QTAILQ_FOREACH_SAFE(subregion, &sysmem->subregions, subregions_link,
> next) {
> + if (subregion->ram) {
> + memory_region_del_subregion(sysmem, subregion);
> + qemu_ram_free(subregion->ram_block);
subregion is leaked. qemu_ram_free() shouldn't be called directly. It's
normally called from the MemoryRegion->destructor function but that
wasn't set up by qemu_ram_init_from_fd().
Please check how MemoryRegion lifecycles should work, update
qemu_ram_init_from_fd(), and update this function to avoid leaks.
> + }
> + }
> +
> + for (region = 0; region < msg->num_fds; region++) {
> + subregion = g_new(MemoryRegion, 1);
> + qemu_ram_init_from_fd(subregion, msg->fds[region],
> + sysmem_info->sizes[region],
> + sysmem_info->offsets[region], &local_err);
Where are is msg->fds[region] closed?
signature.asc
Description: PGP signature
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [PATCH RESEND v6 15/36] multi-process: setup memory manager for remote device,
Stefan Hajnoczi <=