Re: [RFC PATCH for 8.0 10/13] virtio-net: Migrate vhost inflight descriptors

[Jason Wang]

On Tue, Dec 6, 2022 at 1:05 AM Eugenio Pérez <eperezma@redhat.com> wrote:
>
> There is currently no data to be migrated, since nothing populates or
> read the fields on virtio-net.
>
> The migration of in-flight descriptors is modelled after the migration
> of requests in virtio-blk. With some differences:
> * virtio-blk migrates queue number on each request. Here we only add a
>   vq if it has descriptors to migrate, and then we make all descriptors
>   in an array.
> * Use of QTAILQ since it works similar to signal the end of the inflight
>   descriptors: 1 for more data, 0 if end. But do it for each vq instead
>   of for each descriptor.
> * Usage of VMState macros.
>
> The fields of descriptors would be way more complicated if we use the
> VirtQueueElements directly, since there would be a few levels of
> indirections. Using VirtQueueElementOld for the moment, and migrate to
> VirtQueueElement for the final patch.
>
> TODO: Proper migration versioning
> TODO: Do not embed vhost-vdpa structs
> TODO: Migrate the VirtQueueElement, not VirtQueueElementOld.
>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>  include/hw/virtio/virtio-net.h |   2 +
>  include/migration/vmstate.h    |  11 +++
>  hw/net/virtio-net.c            | 129 +++++++++++++++++++++++++++++++++
>  3 files changed, 142 insertions(+)
>
> diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
> index ef234ffe7e..ae7c017ef0 100644
> --- a/include/hw/virtio/virtio-net.h
> +++ b/include/hw/virtio/virtio-net.h
> @@ -151,9 +151,11 @@ typedef struct VirtIONetQueue {
>      QEMUTimer *tx_timer;
>      QEMUBH *tx_bh;
>      uint32_t tx_waiting;
> +    uint32_t tx_inflight_num, rx_inflight_num;
>      struct {
>          VirtQueueElement *elem;
>      } async_tx;
> +    VirtQueueElement **tx_inflight, **rx_inflight;
>      struct VirtIONet *n;
>  } VirtIONetQueue;
>
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 9726d2d09e..9e0dfef9ee 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -626,6 +626,17 @@ extern const VMStateInfo vmstate_info_qlist;
>      .offset     = vmstate_offset_varray(_state, _field, _type),      \
>  }
>
> +#define VMSTATE_STRUCT_VARRAY_ALLOC_UINT16(_field, _state, _field_num,       
>  \
> +                                           _version, _vmsd, _type) {         
>  \
> +    .name       = (stringify(_field)),                                       
>  \
> +    .version_id = (_version),                                                
>  \
> +    .vmsd       = &(_vmsd),                                                  
>  \
> +    .num_offset = vmstate_offset_value(_state, _field_num, uint16_t),        
>  \
> +    .size       = sizeof(_type),                                             
>  \
> +    .flags      = VMS_STRUCT | VMS_VARRAY_UINT16 | VMS_ALLOC | VMS_POINTER,  
>  \
> +    .offset     = vmstate_offset_pointer(_state, _field, _type),             
>  \
> +}
> +
>  #define VMSTATE_STRUCT_VARRAY_ALLOC(_field, _state, _field_num, _version, 
> _vmsd, _type) {\
>      .name       = (stringify(_field)),                               \
>      .version_id = (_version),                                        \
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index aba12759d5..ffd7bf1fc7 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -3077,6 +3077,13 @@ static bool mac_table_doesnt_fit(void *opaque, int 
> version_id)
>      return !mac_table_fits(opaque, version_id);
>  }
>
> +typedef struct VirtIONetInflightQueue {
> +    uint16_t idx;
> +    uint16_t num;
> +    QTAILQ_ENTRY(VirtIONetInflightQueue) entry;
> +    VirtQueueElementOld *elems;
> +} VirtIONetInflightQueue;
> +
>  /* This temporary type is shared by all the WITH_TMP methods
>   * although only some fields are used by each.
>   */
> @@ -3086,6 +3093,7 @@ struct VirtIONetMigTmp {
>      uint16_t        curr_queue_pairs_1;
>      uint8_t         has_ufo;
>      uint32_t        has_vnet_hdr;
> +    QTAILQ_HEAD(, VirtIONetInflightQueue) queues_inflight;
>  };
>
>  /* The 2nd and subsequent tx_waiting flags are loaded later than
> @@ -3231,6 +3239,124 @@ static const VMStateDescription 
> vmstate_virtio_net_rss = {
>      },
>  };
>
> +static const VMStateDescription vmstate_virtio_net_inflight_queue = {
> +    .name      = "virtio-net-device/inflight/queue",
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT16(idx, VirtIONetInflightQueue),
> +        VMSTATE_UINT16(num, VirtIONetInflightQueue),
> +
> +        VMSTATE_STRUCT_VARRAY_ALLOC_UINT16(elems, VirtIONetInflightQueue, 
> num,
> +                                           0, vmstate_virtqueue_element_old,
> +                                           VirtQueueElementOld),
> +        VMSTATE_END_OF_LIST()
> +    },
> +};

A dumb question, any reason we need bother with virtio-net? It looks
to me it's not a must and would complicate migration compatibility.

I guess virtio-blk is the better place.

Thanks

> +
> +static int virtio_net_inflight_init(void *opaque)
> +{
> +    struct VirtIONetMigTmp *tmp = opaque;
> +
> +    QTAILQ_INIT(&tmp->queues_inflight);
> +    return 0;
> +}
> +
> +static int virtio_net_inflight_pre_save(void *opaque)
> +{
> +    struct VirtIONetMigTmp *tmp = opaque;
> +    VirtIONet *net = tmp->parent;
> +    uint16_t curr_queue_pairs = net->multiqueue ? net->curr_queue_pairs : 1;
> +    VirtIONetInflightQueue *qi = g_new0(VirtIONetInflightQueue,
> +                                        curr_queue_pairs * 2);
> +
> +    virtio_net_inflight_init(opaque);
> +    for (uint16_t i = 0; i < curr_queue_pairs * 2; ++i) {
> +        VirtIONetQueue *q = &net->vqs[vq2q(i)];
> +        size_t n = i % 2 ? q->tx_inflight_num : q->rx_inflight_num;
> +        VirtQueueElement **inflight = i % 2 ? q->tx_inflight : 
> q->rx_inflight;
> +
> +        if (n == 0) {
> +            continue;
> +        }
> +
> +        qi[i].idx = i;
> +        qi[i].num = n;
> +        qi[i].elems = g_new0(VirtQueueElementOld, n);
> +        for (uint16_t j = 0; j < n; ++j) {
> +            qemu_put_virtqueue_element_old(inflight[j], &qi[i].elems[j]);
> +        }
> +        QTAILQ_INSERT_TAIL(&tmp->queues_inflight, &qi[i], entry);
> +    }
> +
> +    return 0;
> +}
> +
> +static int virtio_net_inflight_post_save(void *opaque)
> +{
> +    struct VirtIONetMigTmp *tmp = opaque;
> +    VirtIONetInflightQueue *qi;
> +
> +    while ((qi = QTAILQ_FIRST(&tmp->queues_inflight))) {
> +        QTAILQ_REMOVE(&tmp->queues_inflight, qi, entry);
> +        g_free(qi->elems);
> +        g_free(qi);
> +    }
> +
> +    return 0;
> +}
> +
> +static int virtio_net_inflight_post_load(void *opaque, int version_id)
> +{
> +    struct VirtIONetMigTmp *tmp = opaque;
> +    VirtIONet *net = tmp->parent;
> +    uint16_t curr_queue_pairs = net->multiqueue ? net->curr_queue_pairs : 1;
> +    VirtIONetInflightQueue *qi;
> +
> +    while ((qi = QTAILQ_FIRST(&tmp->queues_inflight))) {
> +        VirtIONetQueue *q = &net->vqs[vq2q(qi->idx)];
> +        uint32_t *n = qi->idx % 2 ? &q->tx_inflight_num : 
> &q->rx_inflight_num;
> +        VirtQueueElement ***inflight = qi->idx % 2 ?
> +                                       &q->tx_inflight : &q->rx_inflight;
> +        if (unlikely(qi->num == 0)) {
> +            /* TODO: error message */
> +            return -1;
> +        }
> +
> +        if (unlikely(qi->idx > curr_queue_pairs * 2)) {
> +            /* TODO: error message */
> +            return -1;
> +        }
> +
> +        *n = qi->num;
> +        *inflight = g_new(VirtQueueElement *, *n);
> +        for (uint16_t j = 0; j < *n; ++j) {
> +            (*inflight)[j] = qemu_get_virtqueue_element_from_old(
> +                &net->parent_obj, &qi->elems[j],
> +                sizeof(VirtQueueElement));
> +        }
> +
> +        QTAILQ_REMOVE(&tmp->queues_inflight, qi, entry);
> +        g_free(qi->elems);
> +        g_free(qi);
> +    }
> +
> +    return 0;
> +}
> +
> +/* TODO: Allocate a temporal per queue / queue element, not all of them! */
> +static const VMStateDescription vmstate_virtio_net_inflight = {
> +    .name      = "virtio-net-device/inflight",
> +    .pre_save = virtio_net_inflight_pre_save,
> +    .post_save = virtio_net_inflight_post_save,
> +    .pre_load = virtio_net_inflight_init,
> +    .post_load = virtio_net_inflight_post_load,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_QTAILQ_V(queues_inflight, struct VirtIONetMigTmp, 0,
> +                         vmstate_virtio_net_inflight_queue,
> +                         VirtIONetInflightQueue, entry),
> +        VMSTATE_END_OF_LIST()
> +    },
> +};
> +
>  static const VMStateDescription vmstate_virtio_net_device = {
>      .name = "virtio-net-device",
>      .version_id = VIRTIO_NET_VM_VERSION,
> @@ -3279,6 +3405,9 @@ static const VMStateDescription 
> vmstate_virtio_net_device = {
>                           vmstate_virtio_net_tx_waiting),
>          VMSTATE_UINT64_TEST(curr_guest_offloads, VirtIONet,
>                              has_ctrl_guest_offloads),
> +        /* TODO: Move to subsection */
> +        VMSTATE_WITH_TMP(VirtIONet, struct VirtIONetMigTmp,
> +                         vmstate_virtio_net_inflight),
>          VMSTATE_END_OF_LIST()
>     },
>      .subsections = (const VMStateDescription * []) {
> --
> 2.31.1
>