From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by dpdk.org (Postfix) with ESMTP id BE31A5F2A for ; Mon, 4 Jun 2018 16:29:59 +0200 (CEST) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 3A2A540BC042; Mon, 4 Jun 2018 14:29:59 +0000 (UTC) Received: from [10.36.112.47] (ovpn-112-47.ams2.redhat.com [10.36.112.47]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 58058210C6D2; Mon, 4 Jun 2018 14:29:58 +0000 (UTC) To: Tiwei Bie Cc: zhihong.wang@intel.com, dev@dpdk.org References: <20180601124758.22652-1-maxime.coquelin@redhat.com> <20180601124758.22652-5-maxime.coquelin@redhat.com> <20180604115507.GA21406@debian> From: Maxime Coquelin Message-ID: <5b8d8122-5521-2f65-51e8-224f49f8fb57@redhat.com> Date: Mon, 4 Jun 2018 16:29:56 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <20180604115507.GA21406@debian> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Scanned-By: MIMEDefang 2.78 on 10.11.54.4 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.7]); Mon, 04 Jun 2018 14:29:59 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.7]); Mon, 04 Jun 2018 14:29:59 +0000 (UTC) for IP:'10.11.54.4' DOMAIN:'int-mx04.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'maxime.coquelin@redhat.com' RCPT:'' 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 14:30:00 -0000 On 06/04/2018 01:55 PM, Tiwei Bie wrote: > 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. Agree, I'll do that. I just moved code from virtio_rxtx.c without change, but it is indeed 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; Acked. > > 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. Isn't what the code is doing? has_tx_offload will be false if none of the Tx offload features have been negotiated, so ol_flags won't be checked in that case. >> + >> 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 >