From mboxrd@z Thu Jan  1 00:00:00 1970
From: Yuanhan Liu <yuanhan.liu@linux.intel.com>
Subject: Re: [PATCH] virtio: use zeroed memory for simple TX
	header
Date: Tue, 5 Apr 2016 08:11:46 +0800
Message-ID: <20160405001146.GL3080@yliu-dev.sh.intel.com>
References: <1459454476-6029-1-git-send-email-rlane@bigswitch.com>
 <2344074.HmCM8hzj4i@xps13>
 <20160404200538.GK3080@yliu-dev.sh.intel.com>
 <CAGSMBPO-gDTdBRXTWy6Aq4b8dPoHG1YoUuzNX-ae0E9eNyO6gw@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Transfer-Encoding: quoted-printable
Cc: Thomas Monjalon <thomas.monjalon@6wind.com>,
 "Xie, Huawei" <huawei.xie@intel.com>, dev@dpdk.org,
 stephen@networkplumber.org
To: Rich Lane <rich.lane@bigswitch.com>
Return-path: <dev-bounces@dpdk.org>
Received: from mga11.intel.com (mga11.intel.com [192.55.52.93])
 by dpdk.org (Postfix) with ESMTP id 991672617
 for <dev@dpdk.org>; Tue,  5 Apr 2016 02:10:22 +0200 (CEST)
Content-Disposition: inline
In-Reply-To: <CAGSMBPO-gDTdBRXTWy6Aq4b8dPoHG1YoUuzNX-ae0E9eNyO6gw@mail.gmail.com>
List-Id: patches and discussions about DPDK <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

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.co=
m>
> wrote:
>=20
>     On Mon, Apr 04, 2016 at 03:13:37PM +0200, Thomas Monjalon wrote:
>     > Huawei, Yuanhan, any comment?
>     >
>     > 2016-03-31 13:01, Rich Lane:
>     > >=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0vq->v=
q_ring.desc[i + mid_idx].next =3D i;
>     > >=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0vq->v=
q_ring.desc[i + mid_idx].addr =3D
>     > > -=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0vq->virtio_net_hdr_mem +
>     > > -=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0 =A0 =A0 =A0 =A0i * vq->hw->vtnet_hdr_size;
>     > > +=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0vq->virtio_net_hdr_mem;
>=20
>     I could be wrong, but this looks like a special case when i =3D=3D =
0,
>     which is by no way that zeroed memory is guaranteed? Huawei, do
>     you have time to check this patch?=A0
>=20
>=20
> This bug exists because the type of the objects pointed to by
> virtio_net_hdr_mem changed in 6dc5de3a (virtio: use indirect ring eleme=
nts),
> but because it isn't a C pointer the compiler didn't=A0catch the type m=
ismatch.
> We could also fix it with:
>=20
> =A0 =A0 vq->virtio_net_hdr_mem + i * sizeof(struct virtio_tx_region) + =
offsetof
> (struct virtio_tx_region, tx_hdr)
>=20
> Given that tx_hdr is the first member in struct virtio_tx_region, and u=
sing a
> single header optimizes cache use, that simplifies to the code in my pa=
tch.

It does. However, it hurts readability.

> The
> virtio-net header is never written to by simple TX so it remains zeroed=
.
>=20
> 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 t=
he 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