From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id 552002C38 for ; Mon, 4 Jun 2018 13:54:55 +0200 (CEST) X-Amp-Result: UNSCANNABLE X-Amp-File-Uploaded: False Received: from orsmga006.jf.intel.com ([10.7.209.51]) by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 04 Jun 2018 04:54:54 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.49,476,1520924400"; d="scan'208";a="47121712" Received: from debian.sh.intel.com (HELO debian) ([10.67.104.203]) by orsmga006.jf.intel.com with ESMTP; 04 Jun 2018 04:54:53 -0700 Date: Mon, 4 Jun 2018 19:55:07 +0800 From: Tiwei Bie To: Maxime Coquelin Cc: zhihong.wang@intel.com, dev@dpdk.org Message-ID: <20180604115507.GA21406@debian> References: <20180601124758.22652-1-maxime.coquelin@redhat.com> <20180601124758.22652-5-maxime.coquelin@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20180601124758.22652-5-maxime.coquelin@redhat.com> User-Agent: Mutt/1.9.5 (2018-04-13) Subject: Re: [dpdk-dev] [PATCH 4/4] net/virtio: improve offload check performance X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 04 Jun 2018 11:54:56 -0000 On Fri, Jun 01, 2018 at 02:47:58PM +0200, Maxime Coquelin wrote: > Instead of checking the multiple Virtio features bits for > every packet, let's do the check once at configure time and > store it in virtio_hw struct. > > Signed-off-by: Maxime Coquelin > --- > drivers/net/virtio/virtio_ethdev.c | 19 +++++++++++++++++++ > drivers/net/virtio/virtio_pci.h | 2 ++ > drivers/net/virtio/virtio_rxtx.c | 29 ++++++----------------------- > 3 files changed, 27 insertions(+), 23 deletions(-) > > diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c > index d481b282e..981e0994a 100644 > --- a/drivers/net/virtio/virtio_ethdev.c > +++ b/drivers/net/virtio/virtio_ethdev.c > @@ -1790,6 +1790,22 @@ rte_virtio_pmd_init(void) > rte_pci_register(&rte_virtio_pmd); > } > > +static inline int Maybe it's better to return bool. > +rx_offload_enabled(struct virtio_hw *hw) > +{ > + return vtpci_with_feature(hw, VIRTIO_NET_F_GUEST_CSUM) || > + vtpci_with_feature(hw, VIRTIO_NET_F_GUEST_TSO4) || > + vtpci_with_feature(hw, VIRTIO_NET_F_GUEST_TSO6); > +} > + > +static inline int Ditto. > +tx_offload_enabled(struct virtio_hw *hw) > +{ > + return vtpci_with_feature(hw, VIRTIO_NET_F_CSUM) || > + vtpci_with_feature(hw, VIRTIO_NET_F_HOST_TSO4) || > + vtpci_with_feature(hw, VIRTIO_NET_F_HOST_TSO6); > +} > + > /* > * Configure virtio device > * It returns 0 on success. > @@ -1869,6 +1885,9 @@ virtio_dev_configure(struct rte_eth_dev *dev) > return -ENOTSUP; > } > > + hw->has_tx_offload = !!tx_offload_enabled(hw); > + hw->has_rx_offload = !!rx_offload_enabled(hw); > + > if (dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC) > /* Enable vector (0) for Link State Intrerrupt */ > if (VTPCI_OPS(hw)->set_config_irq(hw, 0) == > diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h > index a28ba8339..e0bb871f2 100644 > --- a/drivers/net/virtio/virtio_pci.h > +++ b/drivers/net/virtio/virtio_pci.h > @@ -233,6 +233,8 @@ struct virtio_hw { > uint8_t modern; > uint8_t use_simple_rx; > uint8_t use_simple_tx; > + uint8_t has_tx_offload; > + uint8_t has_rx_offload; I think it's better to use spaces, instead of 4 spaces width tabs after uint8_t. > uint16_t port_id; > uint8_t mac_addr[ETHER_ADDR_LEN]; > uint32_t notify_off_multiplier; > diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c > index 92fab2174..3f113a118 100644 > --- a/drivers/net/virtio/virtio_rxtx.c > +++ b/drivers/net/virtio/virtio_rxtx.c > @@ -225,13 +225,6 @@ virtio_tso_fix_cksum(struct rte_mbuf *m) > } > } > > -static inline int > -tx_offload_enabled(struct virtio_hw *hw) > -{ > - return vtpci_with_feature(hw, VIRTIO_NET_F_CSUM) || > - vtpci_with_feature(hw, VIRTIO_NET_F_HOST_TSO4) || > - vtpci_with_feature(hw, VIRTIO_NET_F_HOST_TSO6); > -} > > /* avoid write operation when necessary, to lessen cache issues */ > #define ASSIGN_UNLESS_EQUAL(var, val) do { \ > @@ -253,13 +246,15 @@ virtqueue_enqueue_xmit(struct virtnet_tx *txvq, struct rte_mbuf *cookie, > struct virtio_net_hdr *hdr; > int offload; > > - offload = tx_offload_enabled(vq->hw); > head_idx = vq->vq_desc_head_idx; > idx = head_idx; > dxp = &vq->vq_descx[idx]; > dxp->cookie = (void *)cookie; > dxp->ndescs = needed; > > + offload = vq->hw->has_tx_offload && > + (cookie->ol_flags & PKT_TX_OFFLOAD_MASK); If features aren't negotiated, I think there is no need to check ol_flags and update the net header. > + > start_dp = vq->vq_ring.desc; > > if (can_push) { > @@ -270,7 +265,6 @@ virtqueue_enqueue_xmit(struct virtnet_tx *txvq, struct rte_mbuf *cookie, > * which is wrong. Below subtract restores correct pkt size. > */ > cookie->pkt_len -= head_size; > - /* if offload disabled, it is not zeroed below, do it now */ > if (offload == 0) { > ASSIGN_UNLESS_EQUAL(hdr->csum_start, 0); > ASSIGN_UNLESS_EQUAL(hdr->csum_offset, 0); > @@ -686,14 +680,6 @@ virtio_rx_offload(struct rte_mbuf *m, struct virtio_net_hdr *hdr) > return 0; > } [...] Best regards, Tiwei Bie