On Tue, 12 Nov 2024 10:37:27 +0200
Yishai Hadas <yishaih@xxxxxxxxxx> wrote:
> diff --git a/drivers/vfio/pci/virtio/migrate.c b/drivers/vfio/pci/virtio/migrate.c
> new file mode 100644
> index 000000000000..3d5eaa1cbcdb
> --- /dev/null
> +++ b/drivers/vfio/pci/virtio/migrate.c
...
> +static int virtiovf_add_migration_pages(struct virtiovf_data_buffer *buf,
> + unsigned int npages)
> +{
> + unsigned int to_alloc = npages;
> + struct page **page_list;
> + unsigned long filled;
> + unsigned int to_fill;
> + int ret;
> +
> + to_fill = min_t(unsigned int, npages, PAGE_SIZE / sizeof(*page_list));
> + page_list = kvzalloc(to_fill * sizeof(*page_list), GFP_KERNEL_ACCOUNT);
> + if (!page_list)
> + return -ENOMEM;
> +
> + do {
> + filled = alloc_pages_bulk_array(GFP_KERNEL_ACCOUNT, to_fill,
> + page_list);
> + if (!filled) {
> + ret = -ENOMEM;
> + goto err;
> + }
> + to_alloc -= filled;
> + ret = sg_alloc_append_table_from_pages(&buf->table, page_list,
> + filled, 0, filled << PAGE_SHIFT, UINT_MAX,
> + SG_MAX_SINGLE_ALLOC, GFP_KERNEL_ACCOUNT);
> +
> + if (ret)
> + goto err;
Cleanup here has me a bit concerned. I see this pattern in
mlx5vf_add_migration_pages() as well. IIUC, if we hit the previous
ENOMEM condition we simply free page_list and return error because any
pages we've already added to the sg table will be freed in the next
function below. But what happens here? It looks like we've allocated
a bunch of pages, failed to added them to the sg table and we drop them
on the floor. Am I wrong?
> + buf->allocated_length += filled * PAGE_SIZE;
> + /* clean input for another bulk allocation */
> + memset(page_list, 0, filled * sizeof(*page_list));
> + to_fill = min_t(unsigned int, to_alloc,
> + PAGE_SIZE / sizeof(*page_list));
> + } while (to_alloc > 0);
> +
> + kvfree(page_list);
> + return 0;
> +
> +err:
> + kvfree(page_list);
> + return ret;
If ret were initialized to zero it looks like we wouldn't need
duplicate code for the success path, but that might change if we need
more unwind for the above. Thanks,
Alex
> +}
> +
> +static void virtiovf_free_data_buffer(struct virtiovf_data_buffer *buf)
> +{
> + struct sg_page_iter sg_iter;
> +
> + /* Undo alloc_pages_bulk_array() */
> + for_each_sgtable_page(&buf->table.sgt, &sg_iter, 0)
> + __free_page(sg_page_iter_page(&sg_iter));
> + sg_free_append_table(&buf->table);
> + kfree(buf);
> +}
