From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.droids-corp.org (zoll.droids-corp.org [94.23.50.67]) by dpdk.org (Postfix) with ESMTP id 2A3CE56A1 for ; 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 ) id 1bsYBc-0005md-G4; Fri, 07 Oct 2016 18:39:52 +0200 To: Maxime Coquelin , 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> 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 Message-ID: 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: 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-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 >> --- >> 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