[dpdk-dev] [PATCH] virtio: split virtio rx/tx queue

2016-05-30 Thread Yuanhan Liu 
On Mon, May 30, 2016 at 02:40:00AM +, Xie, Huawei wrote:
> On 5/27/2016 5:06 PM, Yuanhan Liu wrote:
> > On Tue, May 24, 2016 at 09:38:32PM +0800, Huawei Xie wrote:
> >>vq->vq_ring_mem = mz->phys_addr;
> >>vq->vq_ring_virt_mem = mz->addr;
> >> -  PMD_INIT_LOG(DEBUG, "vq->vq_ring_mem:  0x%"PRIx64, 
> >> (uint64_t)mz->phys_addr);
> >> -  PMD_INIT_LOG(DEBUG, "vq->vq_ring_virt_mem: 0x%"PRIx64, 
> >> (uint64_t)(uintptr_t)mz->addr);
> >> -  vq->virtio_net_hdr_mz  = NULL;
> >> -  vq->virtio_net_hdr_mem = 0;
> >> +  PMD_INIT_LOG(DEBUG, "vq->vq_ring_mem:  0x%"PRIx64,
> >> +   (uint64_t)mz->phys_addr);
> >> +  PMD_INIT_LOG(DEBUG, "vq->vq_ring_virt_mem: 0x%"PRIx64,
> >> +   (uint64_t)(uintptr_t)mz->addr);
> >> +
> >> +  hdr_mz = rte_memzone_reserve_aligned(vq_hdr_name, sz_hdr_mz, socket_id,
> >> +   0, RTE_CACHE_LINE_SIZE);
> > We don't need allocate hdr_mz for Rx queue, and in such case, sz_hdr_mz
> > is 0. I'm wondering what hdr_mz would be then, NULL?
> >
> > Anyway, you should skip the hdr_mz allocation for Rx queue, and I also
> > would suggest you to move the vq_hdr_name setup here.
> 
> will check sz_hdr_mz before the zone allocation.
> 
> 
> >
> >> +  if (hdr_mz == NULL) {
> >> +  if (rte_errno == EEXIST)
> >> +  hdr_mz = rte_memzone_lookup(vq_hdr_name);
> >> +  if (hdr_mz == NULL) {
> >> +  ret = -ENOMEM;
> >> +  goto fail_q_alloc;
> >> +  }
> >> +  }
> >>  
> > ...
> >>  
> >>PMD_INIT_FUNC_TRACE();
> >>ret = virtio_dev_queue_setup(dev, VTNET_CQ, VTNET_SQ_CQ_QUEUE_IDX,
> >> -  vtpci_queue_idx, 0, socket_id, );
> >> +  vtpci_queue_idx, 0, socket_id, (void **));
> > Unnecessary cast. Note that there are few others like that in this
> > patch.
> 
> This cast is needed.

Oh, right, indeed. Sorry.

> >
> >> -  PMD_RX_LOG(DEBUG, "dequeue:%d", num);
> >> -  PMD_RX_LOG(DEBUG, "packet len:%d", len[0]);
> >> +  PMD_RX_LOG(DEBUG, "dequeue:%d\n", num);
> >> +  PMD_RX_LOG(DEBUG, "packet len:%d\n", len[0]);
> > We should not append "\n" for PMD_RX_LOG; this macro alreadys does it.
> 
> Weird. Will remove it. Thanks.
> 
> >
> > Another note is that you might want to run checkpatch; I saw quite many
> > warnings.
> 
> Had checked. The warnings are all due to 80 char limitation of
> virtio_rxq_stats_strings. Just 4 or 5 chars cross 80 line limit. I
> prefer to keep the fields aligned.

Agreed. However, I was talking about others warnings.

--yliu

---
CHECK:BRACES: braces {} should be used on all arms of this statement
#198: FILE: drivers/net/virtio/virtio_ethdev.c:343:
+   if (queue_type == VTNET_RQ)
[...]
+   else if (queue_type == VTNET_TQ) {
[...]
+   } else if (queue_type == VTNET_CQ) {
[...]

CHECK:CAMELCASE: Avoid CamelCase: 
#280: FILE: drivers/net/virtio/virtio_ethdev.c:404:
+   PMD_INIT_LOG(DEBUG, "vq->vq_ring_mem:  0x%"PRIx64,

CHECK:CONCATENATED_STRING: Concatenated strings should use spaces
between elements
#280: FILE: drivers/net/virtio/virtio_ethdev.c:404:
+   PMD_INIT_LOG(DEBUG, "vq->vq_ring_mem:  0x%"PRIx64,

CHECK:CONCATENATED_STRING: Concatenated strings should use spaces
between elements
#282: FILE: drivers/net/virtio/virtio_ethdev.c:406:
+   PMD_INIT_LOG(DEBUG, "vq->vq_ring_virt_mem: 0x%"PRIx64,

WARNING:UNSPECIFIED_INT: Prefer 'unsigned int' to bare use of 'unsigned'
#482: FILE: drivers/net/virtio/virtio_ethdev.c:753:
+   unsigned nstats = dev->data->nb_tx_queues * VIRTIO_NB_TXQ_XSTATS
+

WARNING:LONG_LINE: line over 80 characters
#551: FILE: drivers/net/virtio/virtio_ethdev.c:819:
+   memset(txvq->stats.size_bins, 0,
sizeof(txvq->stats.size_bins[0]) * 8);

WARNING:LONG_LINE: line over 80 characters
#571: FILE: drivers/net/virtio/virtio_ethdev.c:832:
+   memset(rxvq->stats.size_bins, 0,
sizeof(rxvq->stats.size_bins[0]) * 8);

CHECK:MULTIPLE_ASSIGNMENTS: multiple assignments should be avoided
#1529: FILE: drivers/net/virtio/virtio_rxtx_simple.c:362:
+   nb_commit = nb_pkts = RTE_MIN((vq->vq_free_cnt >> 1), nb_pkts);

CHECK:SPACING: No space is necessary after a cast
#1530: FILE: drivers/net/virtio/virtio_rxtx_simple.c:363:
+   desc_idx = (uint16_t) (vq->vq_avail_idx & desc_idx_max);

[dpdk-dev] [PATCH] virtio: split virtio rx/tx queue

2016-05-30 Thread Xie, Huawei 
On 5/30/2016 11:00 AM, Yuanhan Liu wrote:
> On Mon, May 30, 2016 at 02:40:00AM +, Xie, Huawei wrote:
>> On 5/27/2016 5:06 PM, Yuanhan Liu wrote:
>>> On Tue, May 24, 2016 at 09:38:32PM +0800, Huawei Xie wrote:
vq->vq_ring_mem = mz->phys_addr;
vq->vq_ring_virt_mem = mz->addr;
 -  PMD_INIT_LOG(DEBUG, "vq->vq_ring_mem:  0x%"PRIx64, 
 (uint64_t)mz->phys_addr);
 -  PMD_INIT_LOG(DEBUG, "vq->vq_ring_virt_mem: 0x%"PRIx64, 
 (uint64_t)(uintptr_t)mz->addr);
 -  vq->virtio_net_hdr_mz  = NULL;
 -  vq->virtio_net_hdr_mem = 0;
 +  PMD_INIT_LOG(DEBUG, "vq->vq_ring_mem:  0x%"PRIx64,
 +   (uint64_t)mz->phys_addr);
 +  PMD_INIT_LOG(DEBUG, "vq->vq_ring_virt_mem: 0x%"PRIx64,
 +   (uint64_t)(uintptr_t)mz->addr);
 +
 +  hdr_mz = rte_memzone_reserve_aligned(vq_hdr_name, sz_hdr_mz, socket_id,
 +   0, RTE_CACHE_LINE_SIZE);
>>> We don't need allocate hdr_mz for Rx queue, and in such case, sz_hdr_mz
>>> is 0. I'm wondering what hdr_mz would be then, NULL?
>>>
>>> Anyway, you should skip the hdr_mz allocation for Rx queue, and I also
>>> would suggest you to move the vq_hdr_name setup here.
>> will check sz_hdr_mz before the zone allocation.
>>
>>
 +  if (hdr_mz == NULL) {
 +  if (rte_errno == EEXIST)
 +  hdr_mz = rte_memzone_lookup(vq_hdr_name);
 +  if (hdr_mz == NULL) {
 +  ret = -ENOMEM;
 +  goto fail_q_alloc;
 +  }
 +  }
  
>>> ...
  
PMD_INIT_FUNC_TRACE();
ret = virtio_dev_queue_setup(dev, VTNET_CQ, VTNET_SQ_CQ_QUEUE_IDX,
 -  vtpci_queue_idx, 0, socket_id, );
 +  vtpci_queue_idx, 0, socket_id, (void **));
>>> Unnecessary cast. Note that there are few others like that in this
>>> patch.
>> This cast is needed.
> Oh, right, indeed. Sorry.
>
 -  PMD_RX_LOG(DEBUG, "dequeue:%d", num);
 -  PMD_RX_LOG(DEBUG, "packet len:%d", len[0]);
 +  PMD_RX_LOG(DEBUG, "dequeue:%d\n", num);
 +  PMD_RX_LOG(DEBUG, "packet len:%d\n", len[0]);
>>> We should not append "\n" for PMD_RX_LOG; this macro alreadys does it.
>> Weird. Will remove it. Thanks.
>>
>>> Another note is that you might want to run checkpatch; I saw quite many
>>> warnings.
>> Had checked. The warnings are all due to 80 char limitation of
>> virtio_rxq_stats_strings. Just 4 or 5 chars cross 80 line limit. I
>> prefer to keep the fields aligned.
> Agreed. However, I was talking about others warnings.

ok, i see you are using checkpatch of newer Linux kernel.


>
>   --yliu
>
> ---
> CHECK:BRACES: braces {} should be used on all arms of this statement
> #198: FILE: drivers/net/virtio/virtio_ethdev.c:343:

would fix this.

> +   if (queue_type == VTNET_RQ)
> [...]
> +   else if (queue_type == VTNET_TQ) {
> [...]
> +   } else if (queue_type == VTNET_CQ) {
> [...]
>
> CHECK:CAMELCASE: Avoid CamelCase: 
> #280: FILE: drivers/net/virtio/virtio_ethdev.c:404:
> +   PMD_INIT_LOG(DEBUG, "vq->vq_ring_mem:  0x%"PRIx64,
>
> CHECK:CONCATENATED_STRING: Concatenated strings should use spaces
> between elements
> #280: FILE: drivers/net/virtio/virtio_ethdev.c:404:
> +   PMD_INIT_LOG(DEBUG, "vq->vq_ring_mem:  0x%"PRIx64,
>
> CHECK:CONCATENATED_STRING: Concatenated strings should use spaces
> between elements
> #282: FILE: drivers/net/virtio/virtio_ethdev.c:406:
> +   PMD_INIT_LOG(DEBUG, "vq->vq_ring_virt_mem: 0x%"PRIx64,
>
> WARNING:UNSPECIFIED_INT: Prefer 'unsigned int' to bare use of 'unsigned'
> #482: FILE: drivers/net/virtio/virtio_ethdev.c:753:
> +   unsigned nstats = dev->data->nb_tx_queues * VIRTIO_NB_TXQ_XSTATS
> +
>
> WARNING:LONG_LINE: line over 80 characters
> #551: FILE: drivers/net/virtio/virtio_ethdev.c:819:
> +   memset(txvq->stats.size_bins, 0,
> sizeof(txvq->stats.size_bins[0]) * 8);
>
> WARNING:LONG_LINE: line over 80 characters
> #571: FILE: drivers/net/virtio/virtio_ethdev.c:832:
> +   memset(rxvq->stats.size_bins, 0,
> sizeof(rxvq->stats.size_bins[0]) * 8);

would fix this.

>
> CHECK:MULTIPLE_ASSIGNMENTS: multiple assignments should be avoided
> #1529: FILE: drivers/net/virtio/virtio_rxtx_simple.c:362:
> +   nb_commit = nb_pkts = RTE_MIN((vq->vq_free_cnt >> 1), nb_pkts);
>
> CHECK:SPACING: No space is necessary after a cast
> #1530: FILE: drivers/net/virtio/virtio_rxtx_simple.c:363:
> +   desc_idx = (uint16_t) (vq->vq_avail_idx & desc_idx_max);

All other warnings are due to the newer checkpatch script, and not
introduced by this patch, so wouldn't fix in this patch.
But i think some are better programming practice, like 'Concatenated
strings should use spaces between elements', so would post a patch to
fix them in virtio driver (if possible, all other drivers).

[dpdk-dev] [PATCH] virtio: split virtio rx/tx queue

2016-05-30 Thread Xie, Huawei 
On 5/27/2016 5:06 PM, Yuanhan Liu wrote:
> On Tue, May 24, 2016 at 09:38:32PM +0800, Huawei Xie wrote:
>>  vq->vq_ring_mem = mz->phys_addr;
>>  vq->vq_ring_virt_mem = mz->addr;
>> -PMD_INIT_LOG(DEBUG, "vq->vq_ring_mem:  0x%"PRIx64, 
>> (uint64_t)mz->phys_addr);
>> -PMD_INIT_LOG(DEBUG, "vq->vq_ring_virt_mem: 0x%"PRIx64, 
>> (uint64_t)(uintptr_t)mz->addr);
>> -vq->virtio_net_hdr_mz  = NULL;
>> -vq->virtio_net_hdr_mem = 0;
>> +PMD_INIT_LOG(DEBUG, "vq->vq_ring_mem:  0x%"PRIx64,
>> + (uint64_t)mz->phys_addr);
>> +PMD_INIT_LOG(DEBUG, "vq->vq_ring_virt_mem: 0x%"PRIx64,
>> + (uint64_t)(uintptr_t)mz->addr);
>> +
>> +hdr_mz = rte_memzone_reserve_aligned(vq_hdr_name, sz_hdr_mz, socket_id,
>> + 0, RTE_CACHE_LINE_SIZE);
> We don't need allocate hdr_mz for Rx queue, and in such case, sz_hdr_mz
> is 0. I'm wondering what hdr_mz would be then, NULL?
>
> Anyway, you should skip the hdr_mz allocation for Rx queue, and I also
> would suggest you to move the vq_hdr_name setup here.

will check sz_hdr_mz before the zone allocation.


>
>> +if (hdr_mz == NULL) {
>> +if (rte_errno == EEXIST)
>> +hdr_mz = rte_memzone_lookup(vq_hdr_name);
>> +if (hdr_mz == NULL) {
>> +ret = -ENOMEM;
>> +goto fail_q_alloc;
>> +}
>> +}
>>  
> ...
>>  
>>  PMD_INIT_FUNC_TRACE();
>>  ret = virtio_dev_queue_setup(dev, VTNET_CQ, VTNET_SQ_CQ_QUEUE_IDX,
>> -vtpci_queue_idx, 0, socket_id, );
>> +vtpci_queue_idx, 0, socket_id, (void **));
> Unnecessary cast. Note that there are few others like that in this
> patch.

This cast is needed.

>
>> -PMD_RX_LOG(DEBUG, "dequeue:%d", num);
>> -PMD_RX_LOG(DEBUG, "packet len:%d", len[0]);
>> +PMD_RX_LOG(DEBUG, "dequeue:%d\n", num);
>> +PMD_RX_LOG(DEBUG, "packet len:%d\n", len[0]);
> We should not append "\n" for PMD_RX_LOG; this macro alreadys does it.

Weird. Will remove it. Thanks.

>
> Another note is that you might want to run checkpatch; I saw quite many
> warnings.

Had checked. The warnings are all due to 80 char limitation of
virtio_rxq_stats_strings. Just 4 or 5 chars cross 80 line limit. I
prefer to keep the fields aligned.

>
>   --yliu
>

[dpdk-dev] [PATCH] virtio: split virtio rx/tx queue

2016-05-27 Thread Yuanhan Liu 
On Tue, May 24, 2016 at 09:38:32PM +0800, Huawei Xie wrote:
>   vq->vq_ring_mem = mz->phys_addr;
>   vq->vq_ring_virt_mem = mz->addr;
> - PMD_INIT_LOG(DEBUG, "vq->vq_ring_mem:  0x%"PRIx64, 
> (uint64_t)mz->phys_addr);
> - PMD_INIT_LOG(DEBUG, "vq->vq_ring_virt_mem: 0x%"PRIx64, 
> (uint64_t)(uintptr_t)mz->addr);
> - vq->virtio_net_hdr_mz  = NULL;
> - vq->virtio_net_hdr_mem = 0;
> + PMD_INIT_LOG(DEBUG, "vq->vq_ring_mem:  0x%"PRIx64,
> +  (uint64_t)mz->phys_addr);
> + PMD_INIT_LOG(DEBUG, "vq->vq_ring_virt_mem: 0x%"PRIx64,
> +  (uint64_t)(uintptr_t)mz->addr);
> +
> + hdr_mz = rte_memzone_reserve_aligned(vq_hdr_name, sz_hdr_mz, socket_id,
> +  0, RTE_CACHE_LINE_SIZE);

We don't need allocate hdr_mz for Rx queue, and in such case, sz_hdr_mz
is 0. I'm wondering what hdr_mz would be then, NULL?

Anyway, you should skip the hdr_mz allocation for Rx queue, and I also
would suggest you to move the vq_hdr_name setup here.

> + if (hdr_mz == NULL) {
> + if (rte_errno == EEXIST)
> + hdr_mz = rte_memzone_lookup(vq_hdr_name);
> + if (hdr_mz == NULL) {
> + ret = -ENOMEM;
> + goto fail_q_alloc;
> + }
> + }
>  
...
>  
>   PMD_INIT_FUNC_TRACE();
>   ret = virtio_dev_queue_setup(dev, VTNET_CQ, VTNET_SQ_CQ_QUEUE_IDX,
> - vtpci_queue_idx, 0, socket_id, );
> + vtpci_queue_idx, 0, socket_id, (void **));

Unnecessary cast. Note that there are few others like that in this
patch.


> - PMD_RX_LOG(DEBUG, "dequeue:%d", num);
> - PMD_RX_LOG(DEBUG, "packet len:%d", len[0]);
> + PMD_RX_LOG(DEBUG, "dequeue:%d\n", num);
> + PMD_RX_LOG(DEBUG, "packet len:%d\n", len[0]);

We should not append "\n" for PMD_RX_LOG; this macro alreadys does it.

Another note is that you might want to run checkpatch; I saw quite many
warnings.

--yliu

[dpdk-dev] [PATCH] virtio: split virtio rx/tx queue

2016-05-25 Thread Thomas Monjalon 
2016-05-24 21:38, Huawei Xie:
> We keep a common vq structure, containing only vq related fields,
> and then split others into RX, TX and control queue respectively.
> 
> Signed-off-by: Huawei Xie 

Is it a v2? There is neither changelog nor v2 in the title.

[dpdk-dev] [PATCH] virtio: split virtio rx/tx queue

2016-05-24 Thread Huawei Xie 
We keep a common vq structure, containing only vq related fields,
and then split others into RX, TX and control queue respectively.

Signed-off-by: Huawei Xie 
---
 drivers/net/virtio/virtio_ethdev.c  | 352 ++--
 drivers/net/virtio/virtio_ethdev.h  |   2 +-
 drivers/net/virtio/virtio_pci.c |   4 +-
 drivers/net/virtio/virtio_pci.h |   3 +-
 drivers/net/virtio/virtio_rxtx.c| 294 ++
 drivers/net/virtio/virtio_rxtx.h|  56 -
 drivers/net/virtio/virtio_rxtx_simple.c |  83 
 drivers/net/virtio/virtqueue.h  |  70 +++
 8 files changed, 488 insertions(+), 376 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c 
b/drivers/net/virtio/virtio_ethdev.c
index c3fb628..26fc489 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -114,40 +114,61 @@ struct rte_virtio_xstats_name_off {
 };

 /* [rt]x_qX_ is prepended to the name string here */
-static const struct rte_virtio_xstats_name_off rte_virtio_q_stat_strings[] = {
-   {"good_packets",   offsetof(struct virtqueue, packets)},
-   {"good_bytes", offsetof(struct virtqueue, bytes)},
-   {"errors", offsetof(struct virtqueue, errors)},
-   {"multicast_packets",  offsetof(struct virtqueue, multicast)},
-   {"broadcast_packets",  offsetof(struct virtqueue, broadcast)},
-   {"undersize_packets",  offsetof(struct virtqueue, size_bins[0])},
-   {"size_64_packets",offsetof(struct virtqueue, size_bins[1])},
-   {"size_65_127_packets",offsetof(struct virtqueue, size_bins[2])},
-   {"size_128_255_packets",   offsetof(struct virtqueue, size_bins[3])},
-   {"size_256_511_packets",   offsetof(struct virtqueue, size_bins[4])},
-   {"size_512_1023_packets",  offsetof(struct virtqueue, size_bins[5])},
-   {"size_1024_1517_packets", offsetof(struct virtqueue, size_bins[6])},
-   {"size_1518_max_packets",  offsetof(struct virtqueue, size_bins[7])},
+static const struct rte_virtio_xstats_name_off rte_virtio_rxq_stat_strings[] = 
{
+   {"good_packets",   offsetof(struct virtnet_rx, stats.packets)},
+   {"good_bytes", offsetof(struct virtnet_rx, stats.bytes)},
+   {"errors", offsetof(struct virtnet_rx, stats.errors)},
+   {"multicast_packets",  offsetof(struct virtnet_rx, 
stats.multicast)},
+   {"broadcast_packets",  offsetof(struct virtnet_rx, 
stats.broadcast)},
+   {"undersize_packets",  offsetof(struct virtnet_rx, 
stats.size_bins[0])},
+   {"size_64_packets",offsetof(struct virtnet_rx, 
stats.size_bins[1])},
+   {"size_65_127_packets",offsetof(struct virtnet_rx, 
stats.size_bins[2])},
+   {"size_128_255_packets",   offsetof(struct virtnet_rx, 
stats.size_bins[3])},
+   {"size_256_511_packets",   offsetof(struct virtnet_rx, 
stats.size_bins[4])},
+   {"size_512_1023_packets",  offsetof(struct virtnet_rx, 
stats.size_bins[5])},
+   {"size_1024_1517_packets", offsetof(struct virtnet_rx, 
stats.size_bins[6])},
+   {"size_1518_max_packets",  offsetof(struct virtnet_rx, 
stats.size_bins[7])},
 };

-#define VIRTIO_NB_Q_XSTATS (sizeof(rte_virtio_q_stat_strings) / \
-   sizeof(rte_virtio_q_stat_strings[0]))
+/* [rt]x_qX_ is prepended to the name string here */
+static const struct rte_virtio_xstats_name_off rte_virtio_txq_stat_strings[] = 
{
+   {"good_packets",   offsetof(struct virtnet_tx, stats.packets)},
+   {"good_bytes", offsetof(struct virtnet_tx, stats.bytes)},
+   {"errors", offsetof(struct virtnet_tx, stats.errors)},
+   {"multicast_packets",  offsetof(struct virtnet_tx, 
stats.multicast)},
+   {"broadcast_packets",  offsetof(struct virtnet_tx, 
stats.broadcast)},
+   {"undersize_packets",  offsetof(struct virtnet_tx, 
stats.size_bins[0])},
+   {"size_64_packets",offsetof(struct virtnet_tx, 
stats.size_bins[1])},
+   {"size_65_127_packets",offsetof(struct virtnet_tx, 
stats.size_bins[2])},
+   {"size_128_255_packets",   offsetof(struct virtnet_tx, 
stats.size_bins[3])},
+   {"size_256_511_packets",   offsetof(struct virtnet_tx, 
stats.size_bins[4])},
+   {"size_512_1023_packets",  offsetof(struct virtnet_tx, 
stats.size_bins[5])},
+   {"size_1024_1517_packets", offsetof(struct virtnet_tx, 
stats.size_bins[6])},
+   {"size_1518_max_packets",  offsetof(struct virtnet_tx, 
stats.size_bins[7])},
+};
+
+#define VIRTIO_NB_RXQ_XSTATS (sizeof(rte_virtio_rxq_stat_strings) / \
+   sizeof(rte_virtio_rxq_stat_strings[0]))
+#define VIRTIO_NB_TXQ_XSTATS (sizeof(rte_virtio_txq_stat_strings) / \
+   sizeof(rte_virtio_txq_stat_strings[0]))

 static int
-virtio_send_command(struct virtqueue *vq, struct virtio_pmd_ctrl *ctrl,

[dpdk-dev] [PATCH] virtio: split virtio rx/tx queue

2016-05-09 Thread Yuanhan Liu 
On Mon, May 09, 2016 at 05:44:03AM +, Xie, Huawei wrote:
> 
> 
> > -Original Message-
> > From: Yuanhan Liu [mailto:yuanhan.liu at linux.intel.com]
> > Sent: Monday, May 09, 2016 1:15 PM
> > To: Xie, Huawei
> > Cc: dev at dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH] virtio: split virtio rx/tx queue
> > 
> > On Thu, May 05, 2016 at 05:29:27AM +, Xie, Huawei wrote:
> > > What I mean is firstly we split the queue, without breaking the common
> > > setup; then introduce RX/TX specific setup calling extracted common
> > > setup, so we don't have a chance to introduce duplicated code.
> > 
> > In such way, you have actually introduced duplicated code, haven't you?
> > You may argue, "yes, but I will fix it in a later patch."  This is to
> > introducing a build error and fixing it later to me.
> > 
> 
> Yuanhan, I don't see how hard it is to understand this. Duplicated code isn't 
> introduced. I will send the patch later.

Good. I just want to make sure no duplicated code is introduced.

--yliu

[dpdk-dev] [PATCH] virtio: split virtio rx/tx queue

2016-05-09 Thread Xie, Huawei 


> -Original Message-
> From: Yuanhan Liu [mailto:yuanhan.liu at linux.intel.com]
> Sent: Monday, May 09, 2016 1:15 PM
> To: Xie, Huawei
> Cc: dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] virtio: split virtio rx/tx queue
> 
> On Thu, May 05, 2016 at 05:29:27AM +, Xie, Huawei wrote:
> > What I mean is firstly we split the queue, without breaking the common
> > setup; then introduce RX/TX specific setup calling extracted common
> > setup, so we don't have a chance to introduce duplicated code.
> 
> In such way, you have actually introduced duplicated code, haven't you?
> You may argue, "yes, but I will fix it in a later patch."  This is to
> introducing a build error and fixing it later to me.
> 

Yuanhan, I don't see how hard it is to understand this. Duplicated code isn't 
introduced. I will send the patch later.

>   --yliu

[dpdk-dev] [PATCH] virtio: split virtio rx/tx queue

2016-05-08 Thread Yuanhan Liu 
On Thu, May 05, 2016 at 05:29:27AM +, Xie, Huawei wrote:
> What I mean is firstly we split the queue, without breaking the common
> setup; then introduce RX/TX specific setup calling extracted common
> setup, so we don't have a chance to introduce duplicated code.

In such way, you have actually introduced duplicated code, haven't you?
You may argue, "yes, but I will fix it in a later patch."  This is to
introducing a build error and fixing it later to me.

--yliu

[dpdk-dev] [PATCH] virtio: split virtio rx/tx queue

2016-05-05 Thread Xie, Huawei 
On 5/5/2016 11:46 AM, Yuanhan Liu wrote:
> On Thu, May 05, 2016 at 03:29:44AM +, Xie, Huawei wrote:
>> On 5/5/2016 11:03 AM, Yuanhan Liu wrote:
>>> On Thu, May 05, 2016 at 01:54:25AM +, Xie, Huawei wrote:
 On 5/5/2016 7:59 AM, Yuanhan Liu wrote:
> On Wed, May 04, 2016 at 08:50:27AM +0800, Huawei Xie wrote:
>> -int virtio_dev_queue_setup(struct rte_eth_dev *dev,
>> -int queue_type,
>> -uint16_t queue_idx,
>> +static int
>> +virtio_dev_cq_queue_setup(struct rte_eth_dev *dev,
> While it's good to split Rx/Tx specific stuff, but why are you trying to
> remove a common queue_setup function that does common setups, such as 
> vring
> memory allocation.
>
> This results to much duplicated code: following diff summary also shows
> it clearly:
 The motivation to do this is we need separate RX/TX queue setup.
>>> We actually have done that. If you look at current rx/tx/ctrl_queue_setup()
>>> code, we invoked the common function; we also did some queue specific
>>> settings. It has not been done in a very clean way though: there are quite
>>> many "if .. else .." as you stated. And that's what you are going to 
>>> resolve,
>>> but IMO, you went far: you made __same__ code 3 copies, one for rx, tx and
>>> ctrl queue, respectively.
>>>
 The switch/case in the common queue setup looks bad.
>>> Assuming you are talking about the "if .. else .." ...
>>>
>>> While I agree with you on that, introducing so many duplicated code is 
>>> worse.
>>>
 I am aware of the common operations, and i had planned to extract them,
 maybe i could do this in this patchset.
>>> If you meant to do in another patch on top of this patch, then it looks
>>> like the wrong way to go: breaking something first and then fixing it
>>> later does not sound a good practice to me.
>> To your later comment, we could split first, then do the queue setup rework.
> Well, if you insist, I'm Okay. But please don't do it in the way this
> patch does, that introduces quite many duplicated codes.

Yuanhan, I have no insist.

Our target is 1) remove the queue type if else checking in the
virtio_dev_queue_setup 2) extract the common setup for vq and call them
in specific RX/TX/CQ setup.
For 2, which is really meaningful to me is the queue size retrieve,
queue allocation

What I mean is firstly we split the queue, without breaking the common
setup; then introduce RX/TX specific setup calling extracted common
setup, so we don't have a chance to introduce duplicated code.


>   --yliu
>

[dpdk-dev] [PATCH] virtio: split virtio rx/tx queue

2016-05-05 Thread Xie, Huawei 
On 5/5/2016 11:03 AM, Yuanhan Liu wrote:
> On Thu, May 05, 2016 at 01:54:25AM +, Xie, Huawei wrote:
>> On 5/5/2016 7:59 AM, Yuanhan Liu wrote:
>>> On Wed, May 04, 2016 at 08:50:27AM +0800, Huawei Xie wrote:
 -int virtio_dev_queue_setup(struct rte_eth_dev *dev,
 -  int queue_type,
 -  uint16_t queue_idx,
 +static int
 +virtio_dev_cq_queue_setup(struct rte_eth_dev *dev,
>>> While it's good to split Rx/Tx specific stuff, but why are you trying to
>>> remove a common queue_setup function that does common setups, such as vring
>>> memory allocation.
>>>
>>> This results to much duplicated code: following diff summary also shows
>>> it clearly:
>> The motivation to do this is we need separate RX/TX queue setup.
> We actually have done that. If you look at current rx/tx/ctrl_queue_setup()
> code, we invoked the common function; we also did some queue specific
> settings. It has not been done in a very clean way though: there are quite
> many "if .. else .." as you stated. And that's what you are going to resolve,
> but IMO, you went far: you made __same__ code 3 copies, one for rx, tx and
> ctrl queue, respectively.
>
>> The switch/case in the common queue setup looks bad.
> Assuming you are talking about the "if .. else .." ...
>
> While I agree with you on that, introducing so many duplicated code is worse.
>
>> I am aware of the common operations, and i had planned to extract them,
>> maybe i could do this in this patchset.
> If you meant to do in another patch on top of this patch, then it looks
> like the wrong way to go: breaking something first and then fixing it
> later does not sound a good practice to me.

To your later comment, we could split first, then do the queue setup rework.

>
>>> 7 files changed, 655 insertions(+), 422 deletions(-)
>>>
>>> which makes it harder for maintaining.
>>>
 -}
 +  rxvq = (struct virtnet_rx *)RTE_PTR_ADD(vq,
 +  sizeof(*vq) + vq_size * sizeof(struct vq_desc_extra));
 +  rxvq->vq = vq;
 +  vq->sw_ring = sw_ring;
>>> sw_ring is needed for rx queue only, why not moving it to rx queue struct?
>> Actually this is not about sw_ring.
>> I had planned to use sw_ring for both RX/TX and remove the vq_desc_extra.
>> Two issues
>> 1. RX uses both sw_ring and vq_desc_extra
>> 2. ndescs in vq_desc_extra isn't really needed, we could simply
>> calculate this when we walk through the desc chain, and in most cases,
>> it is 1 or 2.
>>
>> As it is not related to this rework, will do this in a separate patch.
> Yes, it's not related to this patch, and this patch does rx/tx split
> only. So, thinking that sw_ring is for rx only, you should move there.
>
> It will not against with your plan; you can make corresponding change
> there. But for this patch, let's do the split only.
>
> BTW, I still would suggest you to build the patch on top of the cleanup
> and memory leak fix patches from Jianfeng. Your patch won't apply on
> top of current dpdk-next-virtio, and one way or another, you need do
> a rebase.
>
> Last, if I were you, I would split this patch in two: one to move
> the queue specific settings to it's queue setup function, another
> to split rx/tx fields. That would make it easier for review.
>
>   --yliu
>

[dpdk-dev] [PATCH] virtio: split virtio rx/tx queue

2016-05-05 Thread Xie, Huawei 
On 5/5/2016 7:59 AM, Yuanhan Liu wrote:
> On Wed, May 04, 2016 at 08:50:27AM +0800, Huawei Xie wrote:
>> -int virtio_dev_queue_setup(struct rte_eth_dev *dev,
>> -int queue_type,
>> -uint16_t queue_idx,
>> +static int
>> +virtio_dev_cq_queue_setup(struct rte_eth_dev *dev,
> While it's good to split Rx/Tx specific stuff, but why are you trying to
> remove a common queue_setup function that does common setups, such as vring
> memory allocation.
>
> This results to much duplicated code: following diff summary also shows
> it clearly:

The motivation to do this is we need separate RX/TX queue setup.
The switch/case in the common queue setup looks bad.

I am aware of the common operations, and i had planned to extract them,
maybe i could do this in this patchset.


>
> 7 files changed, 655 insertions(+), 422 deletions(-)
>
> which makes it harder for maintaining.
>
>> -}
>> +rxvq = (struct virtnet_rx *)RTE_PTR_ADD(vq,
>> +sizeof(*vq) + vq_size * sizeof(struct vq_desc_extra));
>> +rxvq->vq = vq;
>> +vq->sw_ring = sw_ring;
> sw_ring is needed for rx queue only, why not moving it to rx queue struct?

Actually this is not about sw_ring.
I had planned to use sw_ring for both RX/TX and remove the vq_desc_extra.
Two issues
1. RX uses both sw_ring and vq_desc_extra
2. ndescs in vq_desc_extra isn't really needed, we could simply
calculate this when we walk through the desc chain, and in most cases,
it is 1 or 2.

As it is not related to this rework, will do this in a separate patch.

>
>>  static void
>> -virtio_update_packet_stats(struct virtqueue *vq, struct rte_mbuf *mbuf)
>> +virtio_update_rxq_stats(struct virtnet_rx *rxvq, struct rte_mbuf *mbuf)
>>  {
>>  uint32_t s = mbuf->pkt_len;
>>  struct ether_addr *ea;
>>  
>>  if (s == 64) {
>> -vq->size_bins[1]++;
>> +rxvq->stats.size_bins[1]++;
>>  } else if (s > 64 && s < 1024) {
>>  uint32_t bin;
>>  
>>  /* count zeros, and offset into correct bin */
>>  bin = (sizeof(s) * 8) - __builtin_clz(s) - 5;
>> -vq->size_bins[bin]++;
>> +rxvq->stats.size_bins[bin]++;
>>  } else {
>>  if (s < 64)
>> -vq->size_bins[0]++;
>> +rxvq->stats.size_bins[0]++;
>>  else if (s < 1519)
>> -vq->size_bins[6]++;
>> +rxvq->stats.size_bins[6]++;
>>  else if (s >= 1519)
>> -vq->size_bins[7]++;
>> +rxvq->stats.size_bins[7]++;
>>  }
>>  
>>  ea = rte_pktmbuf_mtod(mbuf, struct ether_addr *);
>>  if (is_multicast_ether_addr(ea)) {
>>  if (is_broadcast_ether_addr(ea))
>> -vq->broadcast++;
>> +rxvq->stats.broadcast++;
>>  else
>> -vq->multicast++;
>> +rxvq->stats.multicast++;
>> +}
>> +}
>> +
>> +static void
>> +virtio_update_txq_stats(struct virtnet_tx *txvq, struct rte_mbuf *mbuf)
> Why not taking "struct virtnet_stats *stats" as the arg, so that we
> don't have to implment two exactly same functions.

ok to me.

>
>> diff --git a/drivers/net/virtio/virtio_rxtx.h 
>> b/drivers/net/virtio/virtio_rxtx.h
>> index a76c3e5..ced55a3 100644
>> --- a/drivers/net/virtio/virtio_rxtx.h
>> +++ b/drivers/net/virtio/virtio_rxtx.h
>> @@ -34,7 +34,59 @@
>>  #define RTE_PMD_VIRTIO_RX_MAX_BURST 64
>>  
>>  #ifdef RTE_MACHINE_CPUFLAG_SSSE3
>> -int virtio_rxq_vec_setup(struct virtqueue *rxq);
>> +
>> +struct virtnet_stats {
> Another remind again: you should put following codes before the
> "#ifdef".
>
>   --yliu
>

[dpdk-dev] [PATCH] virtio: split virtio rx/tx queue

2016-05-04 Thread Yuanhan Liu 
On Thu, May 05, 2016 at 03:29:44AM +, Xie, Huawei wrote:
> On 5/5/2016 11:03 AM, Yuanhan Liu wrote:
> > On Thu, May 05, 2016 at 01:54:25AM +, Xie, Huawei wrote:
> >> On 5/5/2016 7:59 AM, Yuanhan Liu wrote:
> >>> On Wed, May 04, 2016 at 08:50:27AM +0800, Huawei Xie wrote:
>  -int virtio_dev_queue_setup(struct rte_eth_dev *dev,
>  -int queue_type,
>  -uint16_t queue_idx,
>  +static int
>  +virtio_dev_cq_queue_setup(struct rte_eth_dev *dev,
> >>> While it's good to split Rx/Tx specific stuff, but why are you trying to
> >>> remove a common queue_setup function that does common setups, such as 
> >>> vring
> >>> memory allocation.
> >>>
> >>> This results to much duplicated code: following diff summary also shows
> >>> it clearly:
> >> The motivation to do this is we need separate RX/TX queue setup.
> > We actually have done that. If you look at current rx/tx/ctrl_queue_setup()
> > code, we invoked the common function; we also did some queue specific
> > settings. It has not been done in a very clean way though: there are quite
> > many "if .. else .." as you stated. And that's what you are going to 
> > resolve,
> > but IMO, you went far: you made __same__ code 3 copies, one for rx, tx and
> > ctrl queue, respectively.
> >
> >> The switch/case in the common queue setup looks bad.
> > Assuming you are talking about the "if .. else .." ...
> >
> > While I agree with you on that, introducing so many duplicated code is 
> > worse.
> >
> >> I am aware of the common operations, and i had planned to extract them,
> >> maybe i could do this in this patchset.
> > If you meant to do in another patch on top of this patch, then it looks
> > like the wrong way to go: breaking something first and then fixing it
> > later does not sound a good practice to me.
> 
> To your later comment, we could split first, then do the queue setup rework.

Well, if you insist, I'm Okay. But please don't do it in the way this
patch does, that introduces quite many duplicated codes.

--yliu

[dpdk-dev] [PATCH] virtio: split virtio rx/tx queue

2016-05-04 Thread Yuanhan Liu 
On Thu, May 05, 2016 at 01:54:25AM +, Xie, Huawei wrote:
> On 5/5/2016 7:59 AM, Yuanhan Liu wrote:
> > On Wed, May 04, 2016 at 08:50:27AM +0800, Huawei Xie wrote:
> >> -int virtio_dev_queue_setup(struct rte_eth_dev *dev,
> >> -  int queue_type,
> >> -  uint16_t queue_idx,
> >> +static int
> >> +virtio_dev_cq_queue_setup(struct rte_eth_dev *dev,
> > While it's good to split Rx/Tx specific stuff, but why are you trying to
> > remove a common queue_setup function that does common setups, such as vring
> > memory allocation.
> >
> > This results to much duplicated code: following diff summary also shows
> > it clearly:
> 
> The motivation to do this is we need separate RX/TX queue setup.

We actually have done that. If you look at current rx/tx/ctrl_queue_setup()
code, we invoked the common function; we also did some queue specific
settings. It has not been done in a very clean way though: there are quite
many "if .. else .." as you stated. And that's what you are going to resolve,
but IMO, you went far: you made __same__ code 3 copies, one for rx, tx and
ctrl queue, respectively.

> The switch/case in the common queue setup looks bad.

Assuming you are talking about the "if .. else .." ...

While I agree with you on that, introducing so many duplicated code is worse.

> I am aware of the common operations, and i had planned to extract them,
> maybe i could do this in this patchset.

If you meant to do in another patch on top of this patch, then it looks
like the wrong way to go: breaking something first and then fixing it
later does not sound a good practice to me.

> >
> > 7 files changed, 655 insertions(+), 422 deletions(-)
> >
> > which makes it harder for maintaining.
> >
> >> -}
> >> +  rxvq = (struct virtnet_rx *)RTE_PTR_ADD(vq,
> >> +  sizeof(*vq) + vq_size * sizeof(struct vq_desc_extra));
> >> +  rxvq->vq = vq;
> >> +  vq->sw_ring = sw_ring;
> > sw_ring is needed for rx queue only, why not moving it to rx queue struct?
> 
> Actually this is not about sw_ring.
> I had planned to use sw_ring for both RX/TX and remove the vq_desc_extra.
> Two issues
> 1. RX uses both sw_ring and vq_desc_extra
> 2. ndescs in vq_desc_extra isn't really needed, we could simply
> calculate this when we walk through the desc chain, and in most cases,
> it is 1 or 2.
> 
> As it is not related to this rework, will do this in a separate patch.

Yes, it's not related to this patch, and this patch does rx/tx split
only. So, thinking that sw_ring is for rx only, you should move there.

It will not against with your plan; you can make corresponding change
there. But for this patch, let's do the split only.

BTW, I still would suggest you to build the patch on top of the cleanup
and memory leak fix patches from Jianfeng. Your patch won't apply on
top of current dpdk-next-virtio, and one way or another, you need do
a rebase.

Last, if I were you, I would split this patch in two: one to move
the queue specific settings to it's queue setup function, another
to split rx/tx fields. That would make it easier for review.

--yliu

[dpdk-dev] [PATCH] virtio: split virtio rx/tx queue

2016-05-04 Thread Yuanhan Liu 
On Wed, May 04, 2016 at 08:50:27AM +0800, Huawei Xie wrote:
> -int virtio_dev_queue_setup(struct rte_eth_dev *dev,
> - int queue_type,
> - uint16_t queue_idx,
> +static int
> +virtio_dev_cq_queue_setup(struct rte_eth_dev *dev,

While it's good to split Rx/Tx specific stuff, but why are you trying to
remove a common queue_setup function that does common setups, such as vring
memory allocation.

This results to much duplicated code: following diff summary also shows
it clearly:

7 files changed, 655 insertions(+), 422 deletions(-)

which makes it harder for maintaining.

> -}
> + rxvq = (struct virtnet_rx *)RTE_PTR_ADD(vq,
> + sizeof(*vq) + vq_size * sizeof(struct vq_desc_extra));
> + rxvq->vq = vq;
> + vq->sw_ring = sw_ring;

sw_ring is needed for rx queue only, why not moving it to rx queue struct?

>  static void
> -virtio_update_packet_stats(struct virtqueue *vq, struct rte_mbuf *mbuf)
> +virtio_update_rxq_stats(struct virtnet_rx *rxvq, struct rte_mbuf *mbuf)
>  {
>   uint32_t s = mbuf->pkt_len;
>   struct ether_addr *ea;
>  
>   if (s == 64) {
> - vq->size_bins[1]++;
> + rxvq->stats.size_bins[1]++;
>   } else if (s > 64 && s < 1024) {
>   uint32_t bin;
>  
>   /* count zeros, and offset into correct bin */
>   bin = (sizeof(s) * 8) - __builtin_clz(s) - 5;
> - vq->size_bins[bin]++;
> + rxvq->stats.size_bins[bin]++;
>   } else {
>   if (s < 64)
> - vq->size_bins[0]++;
> + rxvq->stats.size_bins[0]++;
>   else if (s < 1519)
> - vq->size_bins[6]++;
> + rxvq->stats.size_bins[6]++;
>   else if (s >= 1519)
> - vq->size_bins[7]++;
> + rxvq->stats.size_bins[7]++;
>   }
>  
>   ea = rte_pktmbuf_mtod(mbuf, struct ether_addr *);
>   if (is_multicast_ether_addr(ea)) {
>   if (is_broadcast_ether_addr(ea))
> - vq->broadcast++;
> + rxvq->stats.broadcast++;
>   else
> - vq->multicast++;
> + rxvq->stats.multicast++;
> + }
> +}
> +
> +static void
> +virtio_update_txq_stats(struct virtnet_tx *txvq, struct rte_mbuf *mbuf)

Why not taking "struct virtnet_stats *stats" as the arg, so that we
don't have to implment two exactly same functions.


> diff --git a/drivers/net/virtio/virtio_rxtx.h 
> b/drivers/net/virtio/virtio_rxtx.h
> index a76c3e5..ced55a3 100644
> --- a/drivers/net/virtio/virtio_rxtx.h
> +++ b/drivers/net/virtio/virtio_rxtx.h
> @@ -34,7 +34,59 @@
>  #define RTE_PMD_VIRTIO_RX_MAX_BURST 64
>  
>  #ifdef RTE_MACHINE_CPUFLAG_SSSE3
> -int virtio_rxq_vec_setup(struct virtqueue *rxq);
> +
> +struct virtnet_stats {

Another remind again: you should put following codes before the
"#ifdef".

--yliu

[dpdk-dev] [PATCH] virtio: split virtio rx/tx queue

2016-05-04 Thread Huawei Xie 
Currently virtio RX/TX paths use common vq structure.
The initial idea is to split virtio RX and TX queues completely as they
have different memory requirement and we could arrange data friendly for
optimization for different paths in future.

With this patch, we keep a common vq structure, as we have too
many common vq operations. Split fields into virtnet_rx
and virtnet_tx respectively.

Signed-off-by: Huawei Xie 
---
 drivers/net/virtio/virtio_ethdev.c  | 333 +---
 drivers/net/virtio/virtio_pci.c |   4 +-
 drivers/net/virtio/virtio_pci.h |   3 +-
 drivers/net/virtio/virtio_rxtx.c| 531 +++-
 drivers/net/virtio/virtio_rxtx.h|  54 +++-
 drivers/net/virtio/virtio_rxtx_simple.c |  85 ++---
 drivers/net/virtio/virtqueue.h  |  67 ++--
 7 files changed, 655 insertions(+), 422 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c 
b/drivers/net/virtio/virtio_ethdev.c
index 63a368a..4d4e59e 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -115,40 +115,62 @@ struct rte_virtio_xstats_name_off {
 };

 /* [rt]x_qX_ is prepended to the name string here */
-static const struct rte_virtio_xstats_name_off rte_virtio_q_stat_strings[] = {
-   {"good_packets",   offsetof(struct virtqueue, packets)},
-   {"good_bytes", offsetof(struct virtqueue, bytes)},
-   {"errors", offsetof(struct virtqueue, errors)},
-   {"multicast_packets",  offsetof(struct virtqueue, multicast)},
-   {"broadcast_packets",  offsetof(struct virtqueue, broadcast)},
-   {"undersize_packets",  offsetof(struct virtqueue, size_bins[0])},
-   {"size_64_packets",offsetof(struct virtqueue, size_bins[1])},
-   {"size_65_127_packets",offsetof(struct virtqueue, size_bins[2])},
-   {"size_128_255_packets",   offsetof(struct virtqueue, size_bins[3])},
-   {"size_256_511_packets",   offsetof(struct virtqueue, size_bins[4])},
-   {"size_512_1023_packets",  offsetof(struct virtqueue, size_bins[5])},
-   {"size_1024_1517_packets", offsetof(struct virtqueue, size_bins[6])},
-   {"size_1518_max_packets",  offsetof(struct virtqueue, size_bins[7])},
+static const struct rte_virtio_xstats_name_off rte_virtio_rxq_stat_strings[] = 
{
+   {"good_packets",   offsetof(struct virtnet_rx, stats.packets)},
+   {"good_bytes", offsetof(struct virtnet_rx, stats.bytes)},
+   {"errors", offsetof(struct virtnet_rx, stats.errors)},
+   {"multicast_packets",  offsetof(struct virtnet_rx, 
stats.multicast)},
+   {"broadcast_packets",  offsetof(struct virtnet_rx, 
stats.broadcast)},
+   {"undersize_packets",  offsetof(struct virtnet_rx, 
stats.size_bins[0])},
+   {"size_64_packets",offsetof(struct virtnet_rx, 
stats.size_bins[1])},
+   {"size_65_127_packets",offsetof(struct virtnet_rx, 
stats.size_bins[2])},
+   {"size_128_255_packets",   offsetof(struct virtnet_rx, 
stats.size_bins[3])},
+   {"size_256_511_packets",   offsetof(struct virtnet_rx, 
stats.size_bins[4])},
+   {"size_512_1023_packets",  offsetof(struct virtnet_rx, 
stats.size_bins[5])},
+   {"size_1024_1517_packets", offsetof(struct virtnet_rx, 
stats.size_bins[6])},
+   {"size_1518_max_packets",  offsetof(struct virtnet_rx, 
stats.size_bins[7])},
 };

-#define VIRTIO_NB_Q_XSTATS (sizeof(rte_virtio_q_stat_strings) / \
-   sizeof(rte_virtio_q_stat_strings[0]))
+/* [rt]x_qX_ is prepended to the name string here */
+static const struct rte_virtio_xstats_name_off rte_virtio_txq_stat_strings[] = 
{
+   {"good_packets",   offsetof(struct virtnet_tx, stats.packets)},
+   {"good_bytes", offsetof(struct virtnet_tx, stats.bytes)},
+   {"errors", offsetof(struct virtnet_tx, stats.errors)},
+   {"multicast_packets",  offsetof(struct virtnet_tx, 
stats.multicast)},
+   {"broadcast_packets",  offsetof(struct virtnet_tx, 
stats.broadcast)},
+   {"undersize_packets",  offsetof(struct virtnet_tx, 
stats.size_bins[0])},
+   {"size_64_packets",offsetof(struct virtnet_tx, 
stats.size_bins[1])},
+   {"size_65_127_packets",offsetof(struct virtnet_tx, 
stats.size_bins[2])},
+   {"size_128_255_packets",   offsetof(struct virtnet_tx, 
stats.size_bins[3])},
+   {"size_256_511_packets",   offsetof(struct virtnet_tx, 
stats.size_bins[4])},
+   {"size_512_1023_packets",  offsetof(struct virtnet_tx, 
stats.size_bins[5])},
+   {"size_1024_1517_packets", offsetof(struct virtnet_tx, 
stats.size_bins[6])},
+   {"size_1518_max_packets",  offsetof(struct virtnet_tx, 
stats.size_bins[7])},
+};
+
+#define VIRTIO_NB_RXQ_XSTATS (sizeof(rte_virtio_rxq_stat_strings) / \
+   sizeof(rte_virtio_rxq_stat_strings[0]))
+#define VIRTIO_NB_TXQ_XSTATS