From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id 352841B51E for ; Wed, 10 Oct 2018 13:43:50 +0200 (CEST) Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 62AE7308A96D; Wed, 10 Oct 2018 11:43:49 +0000 (UTC) Received: from localhost (dhcp-192-209.str.redhat.com [10.33.192.209]) by smtp.corp.redhat.com (Postfix) with ESMTPS id F094D69601; Wed, 10 Oct 2018 11:43:45 +0000 (UTC) Date: Wed, 10 Oct 2018 13:43:44 +0200 From: Jens Freimann To: Maxime Coquelin Cc: dev@dpdk.org, tiwei.bie@intel.com, Gavin.Hu@arm.com Message-ID: <20181010114344.iubzam4zjvcpqht5@jenstp.localdomain> References: <20181003131118.21491-1-jfreimann@redhat.com> <20181003131118.21491-6-jfreimann@redhat.com> <8586e66a-4901-baef-935e-709844244b28@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline In-Reply-To: <8586e66a-4901-baef-935e-709844244b28@redhat.com> User-Agent: NeoMutt/20180716 X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.41]); Wed, 10 Oct 2018 11:43:49 +0000 (UTC) Subject: Re: [dpdk-dev] [PATCH v7 5/8] net/virtio: implement transmit path for packed queues 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: Wed, 10 Oct 2018 11:43:51 -0000 On Wed, Oct 10, 2018 at 09:27:31AM +0200, Maxime Coquelin wrote: > > >On 10/03/2018 03:11 PM, Jens Freimann wrote: >>This implements the transmit path for devices with >>support for packed virtqueues. >> >>Signed-off-by: Jens Freiman >>--- >> drivers/net/virtio/virtio_ethdev.c | 32 ++-- >> drivers/net/virtio/virtio_ethdev.h | 2 + >> drivers/net/virtio/virtio_ring.h | 15 +- >> drivers/net/virtio/virtio_rxtx.c | 276 +++++++++++++++++++++++++++++ >> drivers/net/virtio/virtqueue.h | 18 +- >> 5 files changed, 329 insertions(+), 14 deletions(-) >> >>diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c >>index d6a1613dd..c65ac365c 100644 >>--- a/drivers/net/virtio/virtio_ethdev.c >>+++ b/drivers/net/virtio/virtio_ethdev.c >>@@ -390,6 +390,8 @@ virtio_init_queue(struct rte_eth_dev *dev, uint16_t vtpci_queue_idx) >> vq->hw = hw; >> vq->vq_queue_index = vtpci_queue_idx; >> vq->vq_nentries = vq_size; >>+ if (vtpci_packed_queue(hw)) >>+ vq->vq_ring.avail_wrap_counter = 1; >> /* >> * Reserve a memzone for vring elements >>@@ -496,16 +498,22 @@ virtio_init_queue(struct rte_eth_dev *dev, uint16_t vtpci_queue_idx) >> memset(txr, 0, vq_size * sizeof(*txr)); >> for (i = 0; i < vq_size; i++) { >> struct vring_desc *start_dp = txr[i].tx_indir; >>- >>- vring_desc_init_split(start_dp, RTE_DIM(txr[i].tx_indir)); >>- >>+ struct vring_desc_packed*start_dp_packed = txr[i].tx_indir_pq; >>+ >> /* first indirect descriptor is always the tx header */ >>- start_dp->addr = txvq->virtio_net_hdr_mem >>- + i * sizeof(*txr) >>- + offsetof(struct virtio_tx_region, tx_hdr); >>- >>- start_dp->len = hw->vtnet_hdr_size; >>- start_dp->flags = VRING_DESC_F_NEXT; >>+ if (vtpci_packed_queue(hw)) { >No need to init desc here? No, for split rings this is only done to chain descriptors (set the next field). For packed rings we don't need this. > >>+ start_dp_packed->addr = txvq->virtio_net_hdr_mem >>+ + i * sizeof(*txr) >>+ + offsetof(struct virtio_tx_region, tx_hdr); >>+ start_dp_packed->len = hw->vtnet_hdr_size; >>+ } else { >>+ vring_desc_init_split(start_dp, RTE_DIM(txr[i].tx_indir)); >>+ start_dp->addr = txvq->virtio_net_hdr_mem >>+ + i * sizeof(*txr) >>+ + offsetof(struct virtio_tx_region, tx_hdr); >>+ start_dp->len = hw->vtnet_hdr_size; >>+ start_dp->flags = VRING_DESC_F_NEXT; >>+ } >> } >> } >>@@ -1344,7 +1352,11 @@ set_rxtx_funcs(struct rte_eth_dev *eth_dev) >> eth_dev->rx_pkt_burst = &virtio_recv_pkts; >> } >>- if (hw->use_inorder_tx) { >>+ if (vtpci_packed_queue(hw)) { >>+ PMD_INIT_LOG(INFO, "virtio: using virtio 1.1 Tx path on port %u", >>+ eth_dev->data->port_id); >>+ eth_dev->tx_pkt_burst = virtio_xmit_pkts_packed; >>+ } else if (hw->use_inorder_tx) { >> PMD_INIT_LOG(INFO, "virtio: using inorder Tx path on port %u", >> eth_dev->data->port_id); >> eth_dev->tx_pkt_burst = virtio_xmit_pkts_inorder; >>diff --git a/drivers/net/virtio/virtio_ethdev.h b/drivers/net/virtio/virtio_ethdev.h >>index e0f80e5a4..05d355180 100644 >>--- a/drivers/net/virtio/virtio_ethdev.h >>+++ b/drivers/net/virtio/virtio_ethdev.h >>@@ -82,6 +82,8 @@ uint16_t virtio_recv_mergeable_pkts_inorder(void *rx_queue, >> uint16_t virtio_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, >> uint16_t nb_pkts); >>+uint16_t virtio_xmit_pkts_packed(void *tx_queue, struct rte_mbuf **tx_pkts, >>+ uint16_t nb_pkts); >> uint16_t virtio_xmit_pkts_inorder(void *tx_queue, struct rte_mbuf **tx_pkts, >> uint16_t nb_pkts); >>diff --git a/drivers/net/virtio/virtio_ring.h b/drivers/net/virtio/virtio_ring.h >>index b9e63d4d4..dbffd4dcd 100644 >>--- a/drivers/net/virtio/virtio_ring.h >>+++ b/drivers/net/virtio/virtio_ring.h >>@@ -108,14 +108,25 @@ set_desc_avail(struct vring *vr, struct vring_desc_packed *desc) >> } >> static inline int >>-desc_is_used(struct vring_desc_packed *desc, struct vring *vr) >>+_desc_is_used(struct vring_desc_packed *desc) >> { >> uint16_t used, avail; >> used = !!(desc->flags & VRING_DESC_F_USED(1)); >> avail = !!(desc->flags & VRING_DESC_F_AVAIL(1)); >>- return used == avail && used == vr->used_wrap_counter; >>+ return used == avail; >>+ >>+} >>+ >>+static inline int >>+desc_is_used(struct vring_desc_packed *desc, struct vring *vr) >>+{ >>+ uint16_t used; >>+ >>+ used = !!(desc->flags & VRING_DESC_F_USED(1)); >>+ >>+ return _desc_is_used(desc) && used == vr->used_wrap_counter; >> } > >This is not in the right patch. yes, fixed. >> /* The standard layout for the ring is a continuous chunk of memory which >>diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c >>index eb891433e..4078fba8e 100644 >>--- a/drivers/net/virtio/virtio_rxtx.c >>+++ b/drivers/net/virtio/virtio_rxtx.c >>@@ -38,6 +38,7 @@ >> #define VIRTIO_DUMP_PACKET(m, len) do { } while (0) >> #endif >>+ > >Remove trailing line. > >I need to review the remaining, but you can work on above comments in >the mean time. done. Thanks for the review! regards, Jens