DPDK patches and discussions
 help / color / mirror / Atom feed
From: Tiwei Bie <tiwei.bie@intel.com>
To: Andrew Rybchenko <arybchenko@solarflare.com>
Cc: Marvin Liu <yong.liu@intel.com>,
	maxime.coquelin@redhat.com, zhihong.wang@intel.com,
	stephen@networkplumber.org, dev@dpdk.org, stable@dpdk.org,
	Kevin Traynor <ktraynor@redhat.com>,
	Luca Boccassi <bluca@debian.org>
Subject: Re: [dpdk-dev] [PATCH] net/virtio: fix mbuf data and pkt length mismatch
Date: Tue, 15 Oct 2019 13:33:10 +0800	[thread overview]
Message-ID: <20191015053309.GA18809@___> (raw)
In-Reply-To: <ea94171a-ef64-6909-a976-388b8fadc7e2@solarflare.com>

On Mon, Oct 14, 2019 at 06:15:42PM +0300, Andrew Rybchenko wrote:
> > diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
> > index 27ead19fb..822cce06d 100644
> > --- a/drivers/net/virtio/virtio_rxtx.c
> > +++ b/drivers/net/virtio/virtio_rxtx.c
> > @@ -597,9 +597,8 @@ virtqueue_enqueue_xmit_inorder(struct virtnet_tx *txvq,
> >   		dxp->cookie = (void *)cookies[i];
> >   		dxp->ndescs = 1;
> > -		hdr = (struct virtio_net_hdr *)
> > -			rte_pktmbuf_prepend(cookies[i], head_size);
> > -		cookies[i]->pkt_len -= head_size;
> > +		hdr = (struct virtio_net_hdr *)(char *)cookies[i]->buf_addr +
> > +			cookies[i]->data_off - head_size;
> >   		/* if offload disabled, hdr is not zeroed yet, do it now */
> >   		if (!vq->hw->has_tx_offload)
> > @@ -608,9 +607,10 @@ virtqueue_enqueue_xmit_inorder(struct virtnet_tx *txvq,
> >   			virtqueue_xmit_offload(hdr, cookies[i], true);
> >   		start_dp[idx].addr  = VIRTIO_MBUF_DATA_DMA_ADDR(cookies[i], vq);
> 
> As I understand the problem is here. It points to start of the packet
> (Ethernet header) since data_off is not changed above now, but
> should point to virtio_net_hdr before the packet.
> 
> I think the patch fixes the bug in a wrong direction. It looks better
> to simply remove
> 
>     cookies[i]->pkt_len -= head_size;
> 
> above and care about real packet length difference in
> virtio_update_packet_stats() or when it is called from Tx path.
> 
> If it is OK for maintainers I'm ready to send patches to rollback back
> this one and fix it as described above.

Hi Andrew,

Thanks for looking into this! Feel free to send your fix.
PS. Another thing also needs to be noticed is that, after
prepending the net hdr with rte_pktmbuf_prepend(), below
code in virtio_update_packet_stats() won't be able to
access the ether header as expected:

https://github.com/DPDK/dpdk/blob/31b798a6f08e9b333b94b8bb26910209aa810b73/drivers/net/virtio/virtio_rxtx.c#L134-L140

Thanks,
Tiwei

  parent reply	other threads:[~2019-10-15  5:36 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-23 14:05 Marvin Liu
2019-09-23 15:22 ` Stephen Hemminger
2019-09-24  4:53   ` Liu, Yong
2019-09-27  9:30 ` Maxime Coquelin
2019-09-27  9:50 ` Maxime Coquelin
2019-10-07  7:17   ` Andrew Rybchenko
2019-10-14 15:15 ` Andrew Rybchenko
2019-10-14 15:28   ` Kevin Traynor
2019-10-15  5:33   ` Tiwei Bie [this message]
2019-10-15  8:14     ` Andrew Rybchenko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191015053309.GA18809@___ \
    --to=tiwei.bie@intel.com \
    --cc=arybchenko@solarflare.com \
    --cc=bluca@debian.org \
    --cc=dev@dpdk.org \
    --cc=ktraynor@redhat.com \
    --cc=maxime.coquelin@redhat.com \
    --cc=stable@dpdk.org \
    --cc=stephen@networkplumber.org \
    --cc=yong.liu@intel.com \
    --cc=zhihong.wang@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).