Re: [PATCH 1/5] vhost: refactor rte_vhost_dequeue_burst

[Yuanhan Liu <yuanhan.liu@linux.intel.com>]

On Fri, Dec 11, 2015 at 10:55:48PM -0800, Rich Lane wrote:
> On Wed, Dec 2, 2015 at 10:06 PM, Yuanhan Liu <yuanhan.liu@linux.intel.com>
> wrote:
> 
>     +static inline struct rte_mbuf *
>     +copy_desc_to_mbuf(struct virtio_net *dev, struct vhost_virtqueue *vq,
>     +                 uint16_t desc_idx, struct rte_mempool *mbuf_pool)
>     +{
> 
> ... 
> 
>     +
>     +       desc = &vq->desc[desc_idx];
>     +       desc_addr = gpa_to_vva(dev, desc->addr);
>     +       rte_prefetch0((void *)(uintptr_t)desc_addr);
>     +
>     +       /* Discard virtio header */
>     +       desc_avail  = desc->len - vq->vhost_hlen;
> 
> 
> If desc->len is zero (happens all the time when restarting DPDK apps in the
> guest) then desc_avail will be huge.

I'm aware of it; I have already noted that in the cover letter. This
patch set is just a code refactor. Things like that will do a in a
latter patch set. (The thing is that Huawei is very cagy about making
any changes to vhost rxtx code, as it's the hot data path. So, I will
not make any future changes base on this refactor, unless it's applied).
>  
> 
>     +       desc_offset = vq->vhost_hlen;
>     +
>     +       mbuf_avail  = 0;
>     +       mbuf_offset = 0;
>     +       while (desc_avail || (desc->flags & VRING_DESC_F_NEXT) != 0) { 
> 
>     +               /* This desc reachs to its end, get the next one */
>     +               if (desc_avail == 0) {
>     +                       desc = &vq->desc[desc->next];
> 
> 
> Need to check desc->next against vq->size.

Thanks for the remind.

> 
> There should be a limit on the number of descriptors in a chain to prevent an
> infinite loop.
> 
> 
>      uint16_t
>      rte_vhost_dequeue_burst(struct virtio_net *dev, uint16_t queue_id,
>             struct rte_mempool *mbuf_pool, struct rte_mbuf **pkts, uint16_t
>     count)
>      {
>     ...
>             avail_idx =  *((volatile uint16_t *)&vq->avail->idx);
>     -
>     -       /* If there are no available buffers then return. */
>     -       if (vq->last_used_idx == avail_idx)
>     +       free_entries = avail_idx - vq->last_used_idx;
>     +       if (free_entries == 0)
>                     return 0;
> 
> 
> A common problem is that avail_idx goes backwards when the guest zeroes its
> virtqueues. This function could check for free_entries > vq->size and reset
> vq->last_used_idx.

Thanks, but ditto, will consider it in another patch set.
> 
> 
>     +       LOG_DEBUG(VHOST_DATA, "(%"PRIu64") about to dequene %u buffers\n",
>     +                       dev->device_fh, count);
> 
> 
> Typo at "dequene".

Oops; thanks.

	--yliu