[dpdk-dev,1/5] vhost: refactor rte_vhost_dequeue_burst

The current rte_vhost_dequeue_burst() implementation is a bit messy
and logic twisted. And you could see repeat code here and there: it
invokes rte_pktmbuf_alloc() 3 at three different places!

However, rte_vhost_dequeue_burst() acutally does a simple job: copy
the packet data from vring desc to mbuf. What's tricky here is:

- desc buff could be chained (by desc->next field), so that you need
  fetch next one if current is wholly drained.

- One mbuf could not be big enough to hold all desc buff, hence you
  need to chain the mbuf as well, by the mbuf->next field.

Even though, the logic could be simple. Here is the pseudo code.

	while (this_desc_is_not_drained_totally || has_next_desc) {
		if (this_desc_has_drained_totally) {
			this_desc = next_desc();
		}

		if (mbuf_has_no_room) {
			mbuf = allocate_a_new_mbuf();
		}

		COPY(mbuf, desc);
	}

And this is how I refactored this patch.

Note that the old patch does a special handling for skipping virtio
header. However, that could be simply done by adjusting desc_avail
and desc_offset var:

	/* Discard virtio header */
	desc_avail  = desc->len - vq->vhost_hlen;
	desc_offset = vq->vhost_hlen;

This refactor makes the code much more readable (IMO), yet it reduces
code size (nearly 2K):

    # without this patch
    $ size /path/to/vhost_rxtx.o
       text    data     bss     dec     hex filename
      39348       0       0   39348    99b4 /path/to/vhost_rxtx.o

    # with this patch
    $ size /path/to/vhost_rxtx.o
       text    data     bss     dec     hex filename
      37435       0       0   37435    923b /path/to/vhost_rxtx.o

Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
 lib/librte_vhost/vhost_rxtx.c | 287 +++++++++++++++++-------------------------
 1 file changed, 113 insertions(+), 174 deletions(-)
  

On Thu,  3 Dec 2015 14:06:09 +0800
Yuanhan Liu <yuanhan.liu@linux.intel.com> wrote:

> +#define COPY(dst, src) do {						\
> +	cpy_len = RTE_MIN(desc_avail, mbuf_avail);			\
> +	rte_memcpy((void *)(uintptr_t)(dst),				\
> +		   (const void *)(uintptr_t)(src), cpy_len);		\
> +									\
> +	mbuf_avail  -= cpy_len;						\
> +	mbuf_offset += cpy_len;						\
> +	desc_avail  -= cpy_len;						\
> +	desc_offset += cpy_len;						\
> +} while(0)
> +

I see lots of issues here.

All those void * casts are unnecessary, C casts arguements already.
rte_memcpy is slower for constant size values than memcpy()

This macro violates the rule that ther should be no hidden variables
in a macro. I.e you are assuming cpy_len, desc_avail, and mbuf_avail
are defined in all code using the macro.

Why use an un-typed macro when an inline function would be just
as fast and give type safety?
  

On Thu,  3 Dec 2015 14:06:09 +0800
Yuanhan Liu <yuanhan.liu@linux.intel.com> wrote:

> +	rte_prefetch0((void *)(uintptr_t)desc_addr);

Another unnecessary set of casts.
  

On Wed, Dec 02, 2015 at 11:02:44PM -0800, Stephen Hemminger wrote:
> On Thu,  3 Dec 2015 14:06:09 +0800
> Yuanhan Liu <yuanhan.liu@linux.intel.com> wrote:
> 
> > +#define COPY(dst, src) do {						\
> > +	cpy_len = RTE_MIN(desc_avail, mbuf_avail);			\
> > +	rte_memcpy((void *)(uintptr_t)(dst),				\
> > +		   (const void *)(uintptr_t)(src), cpy_len);		\
> > +									\
> > +	mbuf_avail  -= cpy_len;						\
> > +	mbuf_offset += cpy_len;						\
> > +	desc_avail  -= cpy_len;						\
> > +	desc_offset += cpy_len;						\
> > +} while(0)
> > +
> 
> I see lots of issues here.
> 
> All those void * casts are unnecessary, C casts arguements already.

Hi Stephen,

Without the cast, the compile will not work, as dst is actully with
uint64_t type.

> rte_memcpy is slower for constant size values than memcpy()

Sorry, what does it have something to do with this patch?

> This macro violates the rule that ther should be no hidden variables
> in a macro. I.e you are assuming cpy_len, desc_avail, and mbuf_avail
> are defined in all code using the macro.

Yes, I'm aware of that. And I agree that it's not a good usage
_in general_.

But I'm thinking that it's okay to use it here, for the three
major functions has quite similar logic.  And if my memory
servers me right, there are also quite many codes like that
in Linux kernel.

> Why use an un-typed macro when an inline function would be just
> as fast and give type safety?

It references too many variables, upto 6. And there are 4 vars
needed to be updated. Therefore, making it to be a function
would make the caller long and ugly. That's why I was thinking
to use a macro to remove few lines of repeat code.

So, if hidden var macro is forbidden here, I guess I would go
with just unfolding those lines of code, but not introducing
a helper function.

	--yliu
  

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.


> +       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.

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.

+       LOG_DEBUG(VHOST_DATA, "(%"PRIu64") about to dequene %u buffers\n",
> +                       dev->device_fh, count);
>

Typo at "dequene".
  

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
  

On 12/3/2015 2:03 PM, Yuanhan Liu wrote:
> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> ---
>  lib/librte_vhost/vhost_rxtx.c | 287 +++++++++++++++++-------------------------
>  1 file changed, 113 insertions(+), 174 deletions(-)

Prefer to unroll copy_mbuf_to_desc and your COPY macro. It prevents us
processing descriptors in a burst way in future.
  

On Tue, Jan 26, 2016 at 10:30:12AM +0000, Xie, Huawei wrote:
> On 12/3/2015 2:03 PM, Yuanhan Liu wrote:
> > Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> > ---
> >  lib/librte_vhost/vhost_rxtx.c | 287 +++++++++++++++++-------------------------
> >  1 file changed, 113 insertions(+), 174 deletions(-)
> 
> Prefer to unroll copy_mbuf_to_desc and your COPY macro. It prevents us

I'm okay to unroll COPY macro. But for copy_mbuf_to_desc, I prefer not
to do that, unless it has a good reason.

> processing descriptors in a burst way in future.

So, do you have a plan?

	--yliu
  

On 1/27/2016 11:26 AM, Yuanhan Liu wrote:
> On Tue, Jan 26, 2016 at 10:30:12AM +0000, Xie, Huawei wrote:
>> On 12/3/2015 2:03 PM, Yuanhan Liu wrote:
>>> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
>>> ---
>>>  lib/librte_vhost/vhost_rxtx.c | 287 +++++++++++++++++-------------------------
>>>  1 file changed, 113 insertions(+), 174 deletions(-)
>> Prefer to unroll copy_mbuf_to_desc and your COPY macro. It prevents us
> I'm okay to unroll COPY macro. But for copy_mbuf_to_desc, I prefer not
> to do that, unless it has a good reason.
>
>> processing descriptors in a burst way in future.
> So, do you have a plan?

I think it is OK. If we need unroll in future, we could do that then. I
am open to this. Just my preference. I understand that wrapping makes
code more readable.

>
> 	--yliu
>
  

On Wed, Jan 27, 2016 at 06:12:22AM +0000, Xie, Huawei wrote:
> On 1/27/2016 11:26 AM, Yuanhan Liu wrote:
> > On Tue, Jan 26, 2016 at 10:30:12AM +0000, Xie, Huawei wrote:
> >> On 12/3/2015 2:03 PM, Yuanhan Liu wrote:
> >>> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> >>> ---
> >>>  lib/librte_vhost/vhost_rxtx.c | 287 +++++++++++++++++-------------------------
> >>>  1 file changed, 113 insertions(+), 174 deletions(-)
> >> Prefer to unroll copy_mbuf_to_desc and your COPY macro. It prevents us
> > I'm okay to unroll COPY macro. But for copy_mbuf_to_desc, I prefer not
> > to do that, unless it has a good reason.
> >
> >> processing descriptors in a burst way in future.
> > So, do you have a plan?
> 
> I think it is OK. If we need unroll in future, we could do that then. I
> am open to this. Just my preference. I understand that wrapping makes
> code more readable.

Okay, let's consider it then: unroll would be easy after all.

	--yliu