[dpdk-dev,v3,07/21] net/virtio: implement transmit path for packed queues
Checks
Commit Message
This implements the transmit path for devices with
support for Virtio 1.1.
Add the feature bit for Virtio 1.1 and enable code to
add buffers to vring and mark descriptors as available.
This is based on a patch by Yuanhan Liu.
Signed-off-by: Jens Freiman <jfreimann@redhat.com>
---
drivers/net/virtio/virtio_ethdev.c | 8 ++-
drivers/net/virtio/virtio_ethdev.h | 3 ++
drivers/net/virtio/virtio_rxtx.c | 102 ++++++++++++++++++++++++++++++++++++-
3 files changed, 111 insertions(+), 2 deletions(-)
Comments
On 04/05/2018 12:10 PM, Jens Freimann wrote:
> This implements the transmit path for devices with
> support for Virtio 1.1.
>
> Add the feature bit for Virtio 1.1 and enable code to
> add buffers to vring and mark descriptors as available.
>
> This is based on a patch by Yuanhan Liu.
>
> Signed-off-by: Jens Freiman <jfreimann@redhat.com>
> ---
> drivers/net/virtio/virtio_ethdev.c | 8 ++-
> drivers/net/virtio/virtio_ethdev.h | 3 ++
> drivers/net/virtio/virtio_rxtx.c | 102 ++++++++++++++++++++++++++++++++++++-
> 3 files changed, 111 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index cccefafe9..089a161ac 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -383,6 +383,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
> @@ -1328,7 +1330,11 @@ set_rxtx_funcs(struct rte_eth_dev *eth_dev)
> eth_dev->rx_pkt_burst = &virtio_recv_pkts;
> }
>
> - if (hw->use_simple_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_simple_tx) {
> PMD_INIT_LOG(INFO, "virtio: using simple Tx path on port %u",
> eth_dev->data->port_id);
> eth_dev->tx_pkt_burst = virtio_xmit_pkts_simple;
> diff --git a/drivers/net/virtio/virtio_ethdev.h b/drivers/net/virtio/virtio_ethdev.h
> index bb40064ea..d457013cb 100644
> --- a/drivers/net/virtio/virtio_ethdev.h
> +++ b/drivers/net/virtio/virtio_ethdev.h
> @@ -36,6 +36,7 @@
> 1ULL << VIRTIO_NET_F_GUEST_ANNOUNCE | \
> 1u << VIRTIO_RING_F_INDIRECT_DESC | \
> 1ULL << VIRTIO_F_VERSION_1 | \
> + 1ULL << VIRTIO_F_RING_PACKED | \
Should it really advertise VIRTIO_F_RING_PACKED unconditionally, as it
is not yet fully supported? (non-pow2, indirect descs, etc...)
> 1ULL << VIRTIO_F_IOMMU_PLATFORM)
>
> #define VIRTIO_PMD_SUPPORTED_GUEST_FEATURES \
On Fri, Apr 06, 2018 at 09:56:06AM +0200, Maxime Coquelin wrote:
>
>
>On 04/05/2018 12:10 PM, Jens Freimann wrote:
>>This implements the transmit path for devices with
>>support for Virtio 1.1.
>>
>>Add the feature bit for Virtio 1.1 and enable code to
>>add buffers to vring and mark descriptors as available.
>>
>>This is based on a patch by Yuanhan Liu.
>>
>>Signed-off-by: Jens Freiman <jfreimann@redhat.com>
>>---
>> drivers/net/virtio/virtio_ethdev.c | 8 ++-
>> drivers/net/virtio/virtio_ethdev.h | 3 ++
>> drivers/net/virtio/virtio_rxtx.c | 102 ++++++++++++++++++++++++++++++++++++-
>> 3 files changed, 111 insertions(+), 2 deletions(-)
>>
>>diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
>>index cccefafe9..089a161ac 100644
>>--- a/drivers/net/virtio/virtio_ethdev.c
>>+++ b/drivers/net/virtio/virtio_ethdev.c
>>@@ -383,6 +383,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
>>@@ -1328,7 +1330,11 @@ set_rxtx_funcs(struct rte_eth_dev *eth_dev)
>> eth_dev->rx_pkt_burst = &virtio_recv_pkts;
>> }
>>- if (hw->use_simple_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_simple_tx) {
>> PMD_INIT_LOG(INFO, "virtio: using simple Tx path on port %u",
>> eth_dev->data->port_id);
>> eth_dev->tx_pkt_burst = virtio_xmit_pkts_simple;
>>diff --git a/drivers/net/virtio/virtio_ethdev.h b/drivers/net/virtio/virtio_ethdev.h
>>index bb40064ea..d457013cb 100644
>>--- a/drivers/net/virtio/virtio_ethdev.h
>>+++ b/drivers/net/virtio/virtio_ethdev.h
>>@@ -36,6 +36,7 @@
>> 1ULL << VIRTIO_NET_F_GUEST_ANNOUNCE | \
>> 1u << VIRTIO_RING_F_INDIRECT_DESC | \
>> 1ULL << VIRTIO_F_VERSION_1 | \
>>+ 1ULL << VIRTIO_F_RING_PACKED | \
>
>Should it really advertise VIRTIO_F_RING_PACKED unconditionally, as it
>is not yet fully supported? (non-pow2, indirect descs, etc...)
We can advertise packed ring but have VIRTIO_F_INDIRECT_DESC disabled.
non-pow2 needs to be integrated thoug and will be in v4.
regards,
Jens
>
>> 1ULL << VIRTIO_F_IOMMU_PLATFORM)
>> #define VIRTIO_PMD_SUPPORTED_GUEST_FEATURES \
On Thu, Apr 05, 2018 at 12:10:17PM +0200, Jens Freimann wrote:
[...]
> diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
> index a8aa87b32..9f9b5a8f8 100644
> --- a/drivers/net/virtio/virtio_rxtx.c
> +++ b/drivers/net/virtio/virtio_rxtx.c
> @@ -38,6 +38,101 @@
> #define VIRTIO_DUMP_PACKET(m, len) do { } while (0)
> #endif
>
> +#define VIRTIO_SIMPLE_FLAGS ((uint32_t)ETH_TXQ_FLAGS_NOMULTSEGS | \
> + ETH_TXQ_FLAGS_NOOFFLOADS)
Why add this?
> +
> +/* Cleanup from completed transmits. */
> +static void
> +virtio_xmit_cleanup_packed(struct virtqueue *vq)
> +{
> + uint16_t idx;
> + uint16_t size = vq->vq_nentries;
> + struct vring_desc_packed *desc = vq->vq_ring.desc_packed;
> +
> + idx = vq->vq_used_cons_idx & (size - 1);
> + while (desc_is_used(&desc[idx]) &&
> + vq->vq_free_cnt < size) {
> + vq->vq_free_cnt++;
> + idx = ++vq->vq_used_cons_idx & (size - 1);
Driver needs to keep track of the number of descriptors
to be skipped for each used descriptor.
> + }
> +}
> +
> +uint16_t
> +virtio_xmit_pkts_packed(void *tx_queue, struct rte_mbuf **tx_pkts,
> + uint16_t nb_pkts)
> +{
[...]
> +}
> int
> virtio_dev_rx_queue_done(void *rxq, uint16_t offset)
> {
Please add an empty line between the functions.
> @@ -547,6 +642,10 @@ virtio_dev_tx_queue_setup_finish(struct rte_eth_dev *dev,
>
> PMD_INIT_FUNC_TRACE();
>
> + if (vtpci_packed_queue(hw)) {
> + vq->vq_ring.avail_wrap_counter = 1;
virtio_dev_tx_queue_setup_finish() will be called during a
dev_stop()/dev_start(). The problem is that, the dev_stop()
doesn't really stop the device. So we can't reset the wrap
counter to 1 in dev_start().
> + }
> +
> if (hw->use_simple_tx) {
> for (desc_idx = 0; desc_idx < mid_idx; desc_idx++) {
> vq->vq_ring.avail->ring[desc_idx] =
> @@ -567,7 +666,8 @@ virtio_dev_tx_queue_setup_finish(struct rte_eth_dev *dev,
> vq->vq_ring.avail->ring[desc_idx] = desc_idx;
> }
>
> - VIRTQUEUE_DUMP(vq);
> + if (!vtpci_packed_queue(hw))
The check isn't needed.
> + VIRTQUEUE_DUMP(vq);
>
> return 0;
> }
> --
> 2.14.3
>
On Sun, Apr 08, 2018 at 12:51:32PM +0800, Tiwei Bie wrote:
>On Thu, Apr 05, 2018 at 12:10:17PM +0200, Jens Freimann wrote:
>[...]
>> diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
>> index a8aa87b32..9f9b5a8f8 100644
>> --- a/drivers/net/virtio/virtio_rxtx.c
>> +++ b/drivers/net/virtio/virtio_rxtx.c
>> @@ -38,6 +38,101 @@
>> #define VIRTIO_DUMP_PACKET(m, len) do { } while (0)
>> #endif
>>
>> +#define VIRTIO_SIMPLE_FLAGS ((uint32_t)ETH_TXQ_FLAGS_NOMULTSEGS | \
>> + ETH_TXQ_FLAGS_NOOFFLOADS)
>
>Why add this?
mistake during rebase, will fix.
>
>> +
>> +/* Cleanup from completed transmits. */
>> +static void
>> +virtio_xmit_cleanup_packed(struct virtqueue *vq)
>> +{
>> + uint16_t idx;
>> + uint16_t size = vq->vq_nentries;
>> + struct vring_desc_packed *desc = vq->vq_ring.desc_packed;
>> +
>> + idx = vq->vq_used_cons_idx & (size - 1);
>> + while (desc_is_used(&desc[idx]) &&
>> + vq->vq_free_cnt < size) {
>> + vq->vq_free_cnt++;
>> + idx = ++vq->vq_used_cons_idx & (size - 1);
>
>Driver needs to keep track of the number of descriptors
>to be skipped for each used descriptor.
I've added support for this due already (because of your comment on
the rxvq flush). Will be in v4.
>> + }
>> +}
>> +
>> +uint16_t
>> +virtio_xmit_pkts_packed(void *tx_queue, struct rte_mbuf **tx_pkts,
>> + uint16_t nb_pkts)
>> +{
>[...]
>> +}
>> int
>> virtio_dev_rx_queue_done(void *rxq, uint16_t offset)
>> {
>
>Please add an empty line between the functions.
ok.
>
>
>> @@ -547,6 +642,10 @@ virtio_dev_tx_queue_setup_finish(struct rte_eth_dev *dev,
>>
>> PMD_INIT_FUNC_TRACE();
>>
>> + if (vtpci_packed_queue(hw)) {
>> + vq->vq_ring.avail_wrap_counter = 1;
>
>virtio_dev_tx_queue_setup_finish() will be called during a
>dev_stop()/dev_start(). The problem is that, the dev_stop()
>doesn't really stop the device. So we can't reset the wrap
>counter to 1 in dev_start().
I see, would virtio_init_device() work?
thanks!
regards,
Jens
>
>> + }
>> +
>> if (hw->use_simple_tx) {
>> for (desc_idx = 0; desc_idx < mid_idx; desc_idx++) {
>> vq->vq_ring.avail->ring[desc_idx] =
>> @@ -567,7 +666,8 @@ virtio_dev_tx_queue_setup_finish(struct rte_eth_dev *dev,
>> vq->vq_ring.avail->ring[desc_idx] = desc_idx;
>> }
>>
>> - VIRTQUEUE_DUMP(vq);
>> + if (!vtpci_packed_queue(hw))
>
>The check isn't needed.
>
>
>> + VIRTQUEUE_DUMP(vq);
>>
>> return 0;
>> }
>> --
>> 2.14.3
>>
@@ -383,6 +383,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
@@ -1328,7 +1330,11 @@ set_rxtx_funcs(struct rte_eth_dev *eth_dev)
eth_dev->rx_pkt_burst = &virtio_recv_pkts;
}
- if (hw->use_simple_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_simple_tx) {
PMD_INIT_LOG(INFO, "virtio: using simple Tx path on port %u",
eth_dev->data->port_id);
eth_dev->tx_pkt_burst = virtio_xmit_pkts_simple;
@@ -36,6 +36,7 @@
1ULL << VIRTIO_NET_F_GUEST_ANNOUNCE | \
1u << VIRTIO_RING_F_INDIRECT_DESC | \
1ULL << VIRTIO_F_VERSION_1 | \
+ 1ULL << VIRTIO_F_RING_PACKED | \
1ULL << VIRTIO_F_IOMMU_PLATFORM)
#define VIRTIO_PMD_SUPPORTED_GUEST_FEATURES \
@@ -85,6 +86,8 @@ uint16_t virtio_recv_mergeable_pkts(void *rx_queue, struct rte_mbuf **rx_pkts,
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_recv_pkts_vec(void *rx_queue, struct rte_mbuf **rx_pkts,
uint16_t nb_pkts);
@@ -38,6 +38,101 @@
#define VIRTIO_DUMP_PACKET(m, len) do { } while (0)
#endif
+#define VIRTIO_SIMPLE_FLAGS ((uint32_t)ETH_TXQ_FLAGS_NOMULTSEGS | \
+ ETH_TXQ_FLAGS_NOOFFLOADS)
+
+/* Cleanup from completed transmits. */
+static void
+virtio_xmit_cleanup_packed(struct virtqueue *vq)
+{
+ uint16_t idx;
+ uint16_t size = vq->vq_nentries;
+ struct vring_desc_packed *desc = vq->vq_ring.desc_packed;
+
+ idx = vq->vq_used_cons_idx & (size - 1);
+ while (desc_is_used(&desc[idx]) &&
+ vq->vq_free_cnt < size) {
+ vq->vq_free_cnt++;
+ idx = ++vq->vq_used_cons_idx & (size - 1);
+ }
+}
+
+uint16_t
+virtio_xmit_pkts_packed(void *tx_queue, struct rte_mbuf **tx_pkts,
+ uint16_t nb_pkts)
+{
+ struct virtnet_tx *txvq = tx_queue;
+ struct virtqueue *vq = txvq->vq;
+ uint16_t i;
+ struct vring_desc_packed *desc = vq->vq_ring.desc_packed;
+ uint16_t idx;
+ struct vq_desc_extra *dxp;
+
+ if (unlikely(nb_pkts < 1))
+ return nb_pkts;
+
+ PMD_TX_LOG(DEBUG, "%d packets to xmit", nb_pkts);
+
+ if (likely(vq->vq_free_cnt < vq->vq_free_thresh))
+ virtio_xmit_cleanup_packed(vq);
+
+ for (i = 0; i < nb_pkts; i++) {
+ struct rte_mbuf *txm = tx_pkts[i];
+ struct virtio_tx_region *txr = txvq->virtio_net_hdr_mz->addr;
+ uint16_t head_idx;
+ int wrap_counter;
+
+ if (unlikely(txm->nb_segs + 1 > vq->vq_free_cnt)) {
+ virtio_xmit_cleanup_packed(vq);
+
+ if (unlikely(txm->nb_segs + 1 > vq->vq_free_cnt)) {
+ PMD_TX_LOG(ERR,
+ "No free tx descriptors to transmit");
+ break;
+ }
+ }
+
+ txvq->stats.bytes += txm->pkt_len;
+
+ vq->vq_free_cnt -= txm->nb_segs + 1;
+
+ idx = (vq->vq_avail_idx++) & (vq->vq_nentries - 1);
+ head_idx = idx;
+ wrap_counter = vq->vq_ring.avail_wrap_counter;
+
+ if ((vq->vq_avail_idx & (vq->vq_nentries - 1)) == 0)
+ toggle_wrap_counter(&vq->vq_ring);
+
+ dxp = &vq->vq_descx[idx];
+ if (dxp->cookie != NULL)
+ rte_pktmbuf_free(dxp->cookie);
+ dxp->cookie = txm;
+
+ desc[idx].addr = txvq->virtio_net_hdr_mem +
+ RTE_PTR_DIFF(&txr[idx].tx_hdr, txr);
+ desc[idx].len = vq->hw->vtnet_hdr_size;
+ desc[idx].flags |= VRING_DESC_F_NEXT;
+
+ do {
+ idx = (vq->vq_avail_idx++) & (vq->vq_nentries - 1);
+ desc[idx].addr = VIRTIO_MBUF_DATA_DMA_ADDR(txm, vq);
+ desc[idx].len = txm->data_len;
+ desc[idx].flags |= VRING_DESC_F_NEXT;
+ if (idx == (vq->vq_nentries - 1))
+ toggle_wrap_counter(&vq->vq_ring);
+ } while ((txm = txm->next) != NULL);
+
+ desc[idx].flags &= ~VRING_DESC_F_NEXT;
+
+ rte_smp_wmb();
+ _set_desc_avail(&desc[head_idx], wrap_counter);
+ }
+
+ txvq->stats.packets += i;
+ txvq->stats.errors += nb_pkts - i;
+
+ return i;
+}
int
virtio_dev_rx_queue_done(void *rxq, uint16_t offset)
{
@@ -547,6 +642,10 @@ virtio_dev_tx_queue_setup_finish(struct rte_eth_dev *dev,
PMD_INIT_FUNC_TRACE();
+ if (vtpci_packed_queue(hw)) {
+ vq->vq_ring.avail_wrap_counter = 1;
+ }
+
if (hw->use_simple_tx) {
for (desc_idx = 0; desc_idx < mid_idx; desc_idx++) {
vq->vq_ring.avail->ring[desc_idx] =
@@ -567,7 +666,8 @@ virtio_dev_tx_queue_setup_finish(struct rte_eth_dev *dev,
vq->vq_ring.avail->ring[desc_idx] = desc_idx;
}
- VIRTQUEUE_DUMP(vq);
+ if (!vtpci_packed_queue(hw))
+ VIRTQUEUE_DUMP(vq);
return 0;
}