[dpdk-dev] virtio: use zeroed memory for simple TX header

  For simple TX the virtio-net header must be zeroed, but it was using memory
that had been initialized with indirect descriptor tables. This resulted in
"unsupported gso type" errors from librte_vhost.

We can use the same memory for every descriptor to save cachelines in the
vswitch.

Signed-off-by: Rich Lane <rlane@bigswitch.com>
---
 drivers/net/virtio/virtio_rxtx.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
  

Huawei, Yuanhan, any comment?

2016-03-31 13:01, Rich Lane:
>  				vq->vq_ring.desc[i + mid_idx].next = i;
>  				vq->vq_ring.desc[i + mid_idx].addr =
> -					vq->virtio_net_hdr_mem +
> -						i * vq->hw->vtnet_hdr_size;
> +					vq->virtio_net_hdr_mem;
>  				vq->vq_ring.desc[i + mid_idx].len =
>  					vq->hw->vtnet_hdr_size;
  

On Mon, Apr 04, 2016 at 03:13:37PM +0200, Thomas Monjalon wrote:
> Huawei, Yuanhan, any comment?
> 
> 2016-03-31 13:01, Rich Lane:
> >  				vq->vq_ring.desc[i + mid_idx].next = i;
> >  				vq->vq_ring.desc[i + mid_idx].addr =
> > -					vq->virtio_net_hdr_mem +
> > -						i * vq->hw->vtnet_hdr_size;
> > +					vq->virtio_net_hdr_mem;

I could be wrong, but this looks like a special case when i == 0,
which is by no way that zeroed memory is guaranteed? Huawei, do
you have time to check this patch?

Thanks.
  

On Mon, Apr 4, 2016 at 1:05 PM, Yuanhan Liu <yuanhan.liu@linux.intel.com>
wrote:

> On Mon, Apr 04, 2016 at 03:13:37PM +0200, Thomas Monjalon wrote:
> > Huawei, Yuanhan, any comment?
> >
> > 2016-03-31 13:01, Rich Lane:
> > >                             vq->vq_ring.desc[i + mid_idx].next = i;
> > >                             vq->vq_ring.desc[i + mid_idx].addr =
> > > -                                   vq->virtio_net_hdr_mem +
> > > -                                           i * vq->hw->vtnet_hdr_size;
> > > +                                   vq->virtio_net_hdr_mem;
>
> I could be wrong, but this looks like a special case when i == 0,
> which is by no way that zeroed memory is guaranteed? Huawei, do
> you have time to check this patch?


This bug exists because the type of the objects pointed to by
virtio_net_hdr_mem changed in 6dc5de3a (virtio: use indirect ring
elements), but because it isn't a C pointer the compiler didn't catch the
type mismatch. We could also fix it with:

    vq->virtio_net_hdr_mem + i * sizeof(struct virtio_tx_region) +
offsetof(struct virtio_tx_region, tx_hdr)

Given that tx_hdr is the first member in struct virtio_tx_region, and using
a single header optimizes cache use, that simplifies to the code in my
patch. The virtio-net header is never written to by simple TX so it remains
zeroed.

I can respin the patch using offsetof if that's preferred.

Note that right now virtio simple TX is broken with DPDK vhost due to the
flood of error messages.
  

On Mon, Apr 04, 2016 at 03:57:11PM -0700, Rich Lane wrote:
> On Mon, Apr 4, 2016 at 1:05 PM, Yuanhan Liu <yuanhan.liu@linux.intel.com>
> wrote:
> 
>     On Mon, Apr 04, 2016 at 03:13:37PM +0200, Thomas Monjalon wrote:
>     > Huawei, Yuanhan, any comment?
>     >
>     > 2016-03-31 13:01, Rich Lane:
>     > >                             vq->vq_ring.desc[i + mid_idx].next = i;
>     > >                             vq->vq_ring.desc[i + mid_idx].addr =
>     > > -                                   vq->virtio_net_hdr_mem +
>     > > -                                           i * vq->hw->vtnet_hdr_size;
>     > > +                                   vq->virtio_net_hdr_mem;
> 
>     I could be wrong, but this looks like a special case when i == 0,
>     which is by no way that zeroed memory is guaranteed? Huawei, do
>     you have time to check this patch? 
> 
> 
> This bug exists because the type of the objects pointed to by
> virtio_net_hdr_mem changed in 6dc5de3a (virtio: use indirect ring elements),
> but because it isn't a C pointer the compiler didn't catch the type mismatch.
> We could also fix it with:
> 
>     vq->virtio_net_hdr_mem + i * sizeof(struct virtio_tx_region) + offsetof
> (struct virtio_tx_region, tx_hdr)
> 
> Given that tx_hdr is the first member in struct virtio_tx_region, and using a
> single header optimizes cache use, that simplifies to the code in my patch.

It does. However, it hurts readability.

> The
> virtio-net header is never written to by simple TX so it remains zeroed.
> 
> I can respin the patch using offsetof if that's preferred.

Yes, please. In such way, we could also align with the setting up code
at virtio_dev_queue_setup().

BTW, I have one question: will simple Tx work with indirect buf
enabled?

> Note that right now virtio simple TX is broken with DPDK vhost due to the flood
> of error messages.

Yes, we need the fix, and thanks for the catching. BTW, it's a
regression fix, you'd better add a Fixline into your commit log.

	--yliu