Hi Maxime,
> -----Original Message-----
> From: Maxime Coquelin <maxime.coque...@redhat.com>
> Sent: Wednesday, April 14, 2021 9:41 PM
> To: Jiang, Cheng1 <cheng1.ji...@intel.com>; Xia, Chenbo
> <chenbo....@intel.com>
> Cc: dev@dpdk.org; Hu, Jiayu <jiayu...@intel.com>; Yang, YvonneX
> <yvonnex.y...@intel.com>; Wang, Yinan <yinan.w...@intel.com>; Liu,
> Yong <yong....@intel.com>
> Subject: Re: [PATCH v7 2/4] vhost: add support for packed ring in async vhost
>
>
>
> On 4/14/21 8:13 AM, Cheng Jiang wrote:
> > For now async vhost data path only supports split ring. This patch
> > enables packed ring in async vhost data path to make async vhost
> > compatible with virtio 1.1 spec.
> >
> > Signed-off-by: Cheng Jiang <cheng1.ji...@intel.com>
> > ---
> > lib/librte_vhost/rte_vhost_async.h | 1 +
> > lib/librte_vhost/vhost.c | 49 ++--
> > lib/librte_vhost/vhost.h | 15 +-
> > lib/librte_vhost/virtio_net.c | 432 +++++++++++++++++++++++++++--
> > 4 files changed, 456 insertions(+), 41 deletions(-)
> >
> > diff --git a/lib/librte_vhost/rte_vhost_async.h
> > b/lib/librte_vhost/rte_vhost_async.h
> > index c855ff875..6faa31f5a 100644
> > --- a/lib/librte_vhost/rte_vhost_async.h
> > +++ b/lib/librte_vhost/rte_vhost_async.h
> > @@ -89,6 +89,7 @@ struct rte_vhost_async_channel_ops { struct
> > async_inflight_info {
> > struct rte_mbuf *mbuf;
> > uint16_t descs; /* num of descs inflight */
> > + uint16_t nr_buffers; /* num of buffers inflight for packed ring */
> > };
> >
> > /**
> > diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c index
> > a70fe01d8..f509186c6 100644
> > --- a/lib/librte_vhost/vhost.c
> > +++ b/lib/librte_vhost/vhost.c
> > @@ -338,19 +338,22 @@ cleanup_device(struct virtio_net *dev, int
> > destroy) }
> >
> > static void
> > -vhost_free_async_mem(struct vhost_virtqueue *vq)
> > +vhost_free_async_mem(struct virtio_net *dev, struct vhost_virtqueue
> > +*vq)
> > {
> > - if (vq->async_pkts_info)
> > - rte_free(vq->async_pkts_info);
> > - if (vq->async_descs_split)
> > + rte_free(vq->async_pkts_info);
> > +
> > + if (vq_is_packed(dev)) {
> > + rte_free(vq->async_buffers_packed);
> > + vq->async_buffers_packed = NULL;
> > + } else {
>
> Doing this is not necessary:
>
> rte_free(vq->async_buffers_packed);
> vq->async_buffers_packed = NULL;
> rte_free(vq->async_descs_split);
> vq->async_descs_split = NULL;
>
> Above will just work and will avoid adding dev parameter.
async_buffers_packed and async_descs_split are two members of a union.
If I rte_free(vq->async_buffers_packed), then there is no need to
rte_free(vq->async_descs_split).
So I use dev to determine which one I need to free.
Sure, I can free then both with no condition(I can also only free any one of
them), but I think it's a little strange, Right?
>
>
> > rte_free(vq->async_descs_split);
> > - if (vq->it_pool)
> > - rte_free(vq->it_pool);
> > - if (vq->vec_pool)
> > - rte_free(vq->vec_pool);
> > + vq->async_descs_split = NULL;
> > + }
> > +
> > + rte_free(vq->it_pool);
> > + rte_free(vq->vec_pool);
> >
> > vq->async_pkts_info = NULL;
> > - vq->async_descs_split = NULL;
> > vq->it_pool = NULL;
> > vq->vec_pool = NULL;
> > }
> > @@ -360,10 +363,10 @@ free_vq(struct virtio_net *dev, struct
> > vhost_virtqueue *vq) {
> > if (vq_is_packed(dev))
> > rte_free(vq->shadow_used_packed);
> > - else {
> > + else
> > rte_free(vq->shadow_used_split);
> > - vhost_free_async_mem(vq);
> > - }
> > +
> > + vhost_free_async_mem(dev, vq);
> > rte_free(vq->batch_copy_elems);
> > if (vq->iotlb_pool)
> > rte_mempool_free(vq->iotlb_pool);
> > @@ -1626,10 +1629,9 @@ int rte_vhost_async_channel_register(int vid,
> uint16_t queue_id,
> > if (unlikely(vq == NULL || !dev->async_copy))
> > return -1;
> >
> > - /* packed queue is not supported */
> > - if (unlikely(vq_is_packed(dev) || !f.async_inorder)) {
> > + if (unlikely(!f.async_inorder)) {
> > VHOST_LOG_CONFIG(ERR,
> > - "async copy is not supported on packed queue or
> non-inorder mode "
> > + "async copy is not supported on non-inorder mode "
> > "(vid %d, qid: %d)\n", vid, queue_id);
> > return -1;
> > }
> > @@ -1667,12 +1669,19 @@ int rte_vhost_async_channel_register(int vid,
> uint16_t queue_id,
> > vq->vec_pool = rte_malloc_socket(NULL,
> > VHOST_MAX_ASYNC_VEC * sizeof(struct iovec),
> > RTE_CACHE_LINE_SIZE, node);
> > - vq->async_descs_split = rte_malloc_socket(NULL,
> > + if (vq_is_packed(dev)) {
> > + vq->async_buffers_packed = rte_malloc_socket(NULL,
> > + vq->size * sizeof(struct vring_used_elem_packed),
> > + RTE_CACHE_LINE_SIZE, node);
> > + } else {
> > + vq->async_descs_split = rte_malloc_socket(NULL,
> > vq->size * sizeof(struct vring_used_elem),
> > RTE_CACHE_LINE_SIZE, node);
> > - if (!vq->async_descs_split || !vq->async_pkts_info ||
> > - !vq->it_pool || !vq->vec_pool) {
> > - vhost_free_async_mem(vq);
> > + }
> > +
> > + if (!vq->async_buffers_packed || !vq->async_descs_split ||
> > + !vq->async_pkts_info || !vq->it_pool || !vq->vec_pool) {
>
> Not really than of this error handling. Checking after every malloc if it
> suceed
> would be cleaner.
OK, make sense to me. But that means I need to add log(which can be more
specific) after every check, do you thinks it's ok?
If you thinks it's ok, then I'll fix it in the next version.
>
> > + vhost_free_async_mem(dev, vq);
> > VHOST_LOG_CONFIG(ERR,
> > "async register failed: cannot allocate
> memory for vq data "
> > "(vid %d, qid: %d)\n", vid, queue_id); @@ -
> 1728,7 +1737,7 @@ int
> > rte_vhost_async_channel_unregister(int vid, uint16_t queue_id)
> > goto out;
> > }
> >
> > - vhost_free_async_mem(vq);
> > + vhost_free_async_mem(dev, vq);
> >
> > vq->async_ops.transfer_data = NULL;
> > vq->async_ops.check_completed_copies = NULL; diff --git
> > a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h index
> > f628714c2..673335217 100644
> > --- a/lib/librte_vhost/vhost.h
> > +++ b/lib/librte_vhost/vhost.h
> > @@ -201,9 +201,18 @@ struct vhost_virtqueue {
> > uint16_t async_pkts_idx;
> > uint16_t async_pkts_inflight_n;
> > uint16_t async_last_pkts_n;
> > - struct vring_used_elem *async_descs_split;
> > - uint16_t async_desc_idx;
> > - uint16_t last_async_desc_idx;
> > + union {
> > + struct vring_used_elem *async_descs_split;
> > + struct vring_used_elem_packed *async_buffers_packed;
> > + };
> > + union {
> > + uint16_t async_desc_idx;
> > + uint16_t async_packed_buffer_idx;
> > + };
> > + union {
> > + uint16_t last_async_desc_idx;
> > + uint16_t last_async_buffer_idx;
> > + };
>
> Looks almost good to me now, thanks for doing the change.
> Only minor issue is the naming which is not consistent in the different
> fields.
> Sometimes it contains split or packed, sometimes not.
OK, I'll check these names, and fix them.
Thanks a lot.
Cheng
>
> Maxime
