From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <olivier.matz@6wind.com>
Received: from mail.droids-corp.org (zoll.droids-corp.org [94.23.50.67])
by dpdk.org (Postfix) with ESMTP id 2A3CE56A1
for <dev@dpdk.org>; Fri, 7 Oct 2016 18:36:50 +0200 (CEST)
Received: from lfbn-1-5996-232.w90-110.abo.wanadoo.fr ([90.110.195.232]
helo=[192.168.1.13])
by mail.droids-corp.org with esmtpsa (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128)
(Exim 4.84_2) (envelope-from <olivier.matz@6wind.com>)
id 1bsYBc-0005md-G4; Fri, 07 Oct 2016 18:39:52 +0200
To: Maxime Coquelin <maxime.coquelin@redhat.com>, dev@dpdk.org,
yuanhan.liu@linux.intel.com
References: <1469088510-7552-1-git-send-email-olivier.matz@6wind.com>
<1475485223-30566-1-git-send-email-olivier.matz@6wind.com>
<1475485223-30566-11-git-send-email-olivier.matz@6wind.com>
<e35e4b43-5052-fafe-febb-a2bf2e92819b@redhat.com>
Cc: konstantin.ananyev@intel.com, sugesh.chandran@intel.com,
bruce.richardson@intel.com, jianfeng.tan@intel.com, helin.zhang@intel.com,
adrien.mazarguil@6wind.com, stephen@networkplumber.org, dprovan@bivio.net,
xiao.w.wang@intel.com
From: Olivier Matz <olivier.matz@6wind.com>
Message-ID: <e413a854-0359-b22a-3f1e-9028ea0c5d15@6wind.com>
Date: Fri, 7 Oct 2016 18:36:35 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
Icedove/45.2.0
MIME-Version: 1.0
In-Reply-To: <e35e4b43-5052-fafe-febb-a2bf2e92819b@redhat.com>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 7bit
Subject: Re: [dpdk-dev] [PATCH v2 10/12] virtio: add Tx checksum offload
support
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
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>
X-List-Received-Date: Fri, 07 Oct 2016 16:36:50 -0000
Hi Maxime,
On 10/07/2016 09:25 AM, Maxime Coquelin wrote:
> Hi Olivier,
>
> On 10/03/2016 11:00 AM, Olivier Matz wrote:
>> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
>> ---
>> drivers/net/virtio/virtio_ethdev.c | 7 +++++
>> drivers/net/virtio/virtio_ethdev.h | 1 +
>> drivers/net/virtio/virtio_rxtx.c | 57
>> +++++++++++++++++++++++++-------------
>> 3 files changed, 45 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/net/virtio/virtio_ethdev.c
>> b/drivers/net/virtio/virtio_ethdev.c
>> index 43cb096..55024cd 100644
>> --- a/drivers/net/virtio/virtio_ethdev.c
>> +++ b/drivers/net/virtio/virtio_ethdev.c
>> @@ -1578,6 +1578,13 @@ virtio_dev_info_get(struct rte_eth_dev *dev,
>> struct rte_eth_dev_info *dev_info)
>> dev_info->rx_offload_capa =
>> DEV_RX_OFFLOAD_TCP_CKSUM |
>> DEV_RX_OFFLOAD_UDP_CKSUM;
>> + dev_info->tx_offload_capa = 0;
>> +
>> + if (hw->guest_features & (1ULL << VIRTIO_NET_F_CSUM)) {
>> + dev_info->tx_offload_capa |=
>> + DEV_TX_OFFLOAD_UDP_CKSUM |
>> + DEV_TX_OFFLOAD_TCP_CKSUM;
>> + }
>> }
>>
>> /*
>> diff --git a/drivers/net/virtio/virtio_ethdev.h
>> b/drivers/net/virtio/virtio_ethdev.h
>> index 2fc9218..202aa2e 100644
>> --- a/drivers/net/virtio/virtio_ethdev.h
>> +++ b/drivers/net/virtio/virtio_ethdev.h
>> @@ -62,6 +62,7 @@
>> 1u << VIRTIO_NET_F_CTRL_VQ | \
>> 1u << VIRTIO_NET_F_CTRL_RX | \
>> 1u << VIRTIO_NET_F_CTRL_VLAN | \
>> + 1u << VIRTIO_NET_F_CSUM | \
>> 1u << VIRTIO_NET_F_MRG_RXBUF | \
>> 1ULL << VIRTIO_F_VERSION_1)
>>
>> diff --git a/drivers/net/virtio/virtio_rxtx.c
>> b/drivers/net/virtio/virtio_rxtx.c
>> index eda678a..4ae11e7 100644
>> --- a/drivers/net/virtio/virtio_rxtx.c
>> +++ b/drivers/net/virtio/virtio_rxtx.c
>> @@ -213,13 +213,14 @@ static inline void
>> virtqueue_enqueue_xmit(struct virtnet_tx *txvq, struct rte_mbuf *cookie,
>> uint16_t needed, int use_indirect, int can_push)
>> {
>> + struct virtio_tx_region *txr = txvq->virtio_net_hdr_mz->addr;
>> struct vq_desc_extra *dxp;
>> struct virtqueue *vq = txvq->vq;
>> struct vring_desc *start_dp;
>> uint16_t seg_num = cookie->nb_segs;
>> uint16_t head_idx, idx;
>> uint16_t head_size = vq->hw->vtnet_hdr_size;
>> - unsigned long offs;
>> + struct virtio_net_hdr *hdr;
>>
>> head_idx = vq->vq_desc_head_idx;
>> idx = head_idx;
>> @@ -230,10 +231,9 @@ virtqueue_enqueue_xmit(struct virtnet_tx *txvq,
>> struct rte_mbuf *cookie,
>> start_dp = vq->vq_ring.desc;
>>
>> if (can_push) {
>> - /* put on zero'd transmit header (no offloads) */
>> - void *hdr = rte_pktmbuf_prepend(cookie, head_size);
>> -
>> - memset(hdr, 0, head_size);
>> + /* prepend cannot fail, checked by caller */
>> + hdr = (struct virtio_net_hdr *)
>> + rte_pktmbuf_prepend(cookie, head_size);
>> } else if (use_indirect) {
>> /* setup tx ring slot to point to indirect
>> * descriptor list stored in reserved region.
>> @@ -241,14 +241,11 @@ virtqueue_enqueue_xmit(struct virtnet_tx *txvq,
>> struct rte_mbuf *cookie,
>> * the first slot in indirect ring is already preset
>> * to point to the header in reserved region
>> */
>> - struct virtio_tx_region *txr = txvq->virtio_net_hdr_mz->addr;
>> -
>> - offs = idx * sizeof(struct virtio_tx_region)
>> - + offsetof(struct virtio_tx_region, tx_indir);
>> -
>> - start_dp[idx].addr = txvq->virtio_net_hdr_mem + offs;
>> + start_dp[idx].addr = txvq->virtio_net_hdr_mem +
>> + RTE_PTR_DIFF(&txr[idx].tx_indir, txr);
>> start_dp[idx].len = (seg_num + 1) * sizeof(struct vring_desc);
>> start_dp[idx].flags = VRING_DESC_F_INDIRECT;
>> + hdr = (struct virtio_net_hdr *)&txr[idx].tx_hdr;
>>
>> /* loop below will fill in rest of the indirect elements */
>> start_dp = txr[idx].tx_indir;
>> @@ -257,15 +254,40 @@ virtqueue_enqueue_xmit(struct virtnet_tx *txvq,
>> struct rte_mbuf *cookie,
>> /* setup first tx ring slot to point to header
>> * stored in reserved region.
>> */
>> - offs = idx * sizeof(struct virtio_tx_region)
>> - + offsetof(struct virtio_tx_region, tx_hdr);
>> -
>> - start_dp[idx].addr = txvq->virtio_net_hdr_mem + offs;
>> + start_dp[idx].addr = txvq->virtio_net_hdr_mem +
>> + RTE_PTR_DIFF(&txr[idx].tx_hdr, txr);
>> start_dp[idx].len = vq->hw->vtnet_hdr_size;
>> start_dp[idx].flags = VRING_DESC_F_NEXT;
>> + hdr = (struct virtio_net_hdr *)&txr[idx].tx_hdr;
>> +
>> idx = start_dp[idx].next;
>> }
>>
>> + /* Checksum Offload */
>> + switch (cookie->ol_flags & PKT_TX_L4_MASK) {
>> + case PKT_TX_UDP_CKSUM:
>> + hdr->csum_start = cookie->l2_len + cookie->l3_len;
>> + hdr->csum_offset = 6;
>> + hdr->flags = VIRTIO_NET_HDR_F_NEEDS_CSUM;
>> + break;
>> +
>> + case PKT_TX_TCP_CKSUM:
>> + hdr->csum_start = cookie->l2_len + cookie->l3_len;
>> + hdr->csum_offset = 16;
>> + hdr->flags = VIRTIO_NET_HDR_F_NEEDS_CSUM;
>> + break;
>> +
>> + default:
>> + hdr->csum_start = 0;
>> + hdr->csum_offset = 0;
>> + hdr->flags = 0;
>> + break;
>> + }
>> +
>> + hdr->gso_type = 0;
>> + hdr->gso_size = 0;
>> + hdr->hdr_len = 0;
>
> In he case we don't use any offload, have you measured the performance
> regression
> with current code when using a dedicated descriptor for the header?
> I haven't tested you series, but I would think it is more than 15% for
> 64 bytes packets based on my trials to use a single descriptor.
>
> Indeed, without your series, when using a dedicated desc for the header,
> the header is not accessed in the virtio transmit path. It is zeroed at
> init time.
>
> Could we keep the same behaviour when offloading features aren't
> negotiated?
You're right, it could have a performance impact. I'll try to restore
the initial behavior when offload is disabled.
Thanks,
Olivier