From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <yuanhan.liu@linux.intel.com>
Received: from mga06.intel.com (mga06.intel.com [134.134.136.31])
 by dpdk.org (Postfix) with ESMTP id 89246201
 for <dev@dpdk.org>; Wed, 14 Dec 2016 08:26:02 +0100 (CET)
Received: from fmsmga001.fm.intel.com ([10.253.24.23])
 by orsmga104.jf.intel.com with ESMTP; 13 Dec 2016 23:26:01 -0800
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.33,345,1477983600"; d="scan'208";a="1081603463"
Received: from yliu-dev.sh.intel.com (HELO yliu-dev) ([10.239.67.162])
 by fmsmga001.fm.intel.com with ESMTP; 13 Dec 2016 23:26:00 -0800
Date: Wed, 14 Dec 2016 15:27:50 +0800
From: Yuanhan Liu <yuanhan.liu@linux.intel.com>
To: Olivier Matz <olivier.matz@6wind.com>
Cc: dev@dpdk.org, maxime.coquelin@redhat.com, huawei.xie@intel.com,
 stephen@networkplumber.org
Message-ID: <20161214072750.GK18991@yliu-dev.sh.intel.com>
References: <1479977798-13417-1-git-send-email-olivier.matz@6wind.com>
 <1479977798-13417-6-git-send-email-olivier.matz@6wind.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1479977798-13417-6-git-send-email-olivier.matz@6wind.com>
User-Agent: Mutt/1.5.23 (2014-03-12)
Subject: Re: [dpdk-dev] [PATCH 5/5] net/virtio: fix Tso when mbuf is shared
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: DPDK patches and discussions <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>
X-List-Received-Date: Wed, 14 Dec 2016 07:26:03 -0000

Hi Olivier,

Firstly sorry for late response!

On Thu, Nov 24, 2016 at 09:56:38AM +0100, Olivier Matz wrote:
> With virtio, doing tso requires to modify the network
> packet data:

I thought more about it this time, and I'm wondering why it's needed.

> - the dpdk API requires to set the l4 checksum to an
>   Intel-Nic-like pseudo header checksum that does
>   not include the ip length

If the packet is for a NIC pmd driver in the end, then the NIC driver
(or application) would handle the checksum correctly.  You could check
the tx_prep patchset for example.

> - the virtio peer expects that the l4 checksum is
>   a standard pseudo header checksum.

For this case, the checksum is then not needed: we could assume the data
between virtio to virtio transmission on the same host is always valid,
that checksum validation is unnecessary.

So, in either case, it doesn't seem to me we have to generate the checksum
here. Or am I miss something?

OTOH, even if it does, I still see some issues (see below).

>  		/* TCP Segmentation Offload */
>  		if (cookie->ol_flags & PKT_TX_TCP_SEG) {
> -			virtio_tso_fix_cksum(cookie);
> +			offset = virtio_tso_fix_cksum(cookie,
> +				RTE_PTR_ADD(hdr, start_dp[hdr_idx].len),
> +				VIRTIO_MAX_HDR_SZ);
> +			if (offset > 0) {
> +				RTE_ASSERT(can_push != 0);

I think it's (can_push == 0) ?

> +				start_dp[hdr_idx].len += offset;

Actually, there is an assumption if you do this, that the backend driver
must have to support ANY_LAYOUT. Otherwise, it won't work: the driver
would expect the header and packet data is totally separated into two
desc buffers.

Though the assumption is most likely true in nowadays, I don't think
it's a guarantee.

	--yliu