[PATCH] virtio: xstats name issue

[PATCH] virtio: xstats name issue

[Zhiyong Yang]

The patch fixes some xstats name issues and make the xstats name conform to
code implementation(the function virtio_update_packet_stats).

Signed-off-by: Zhiyong Yang <zhiyong.yang@intel.com>
---
 drivers/net/virtio/virtio_ethdev.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 07d6449..4cee067 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -125,8 +125,8 @@ static const struct rte_virtio_xstats_name_off rte_virtio_rxq_stat_strings[] = {
 	{"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])},
+	{"size_1024_1518_packets", offsetof(struct virtnet_rx, stats.size_bins[6])},
+	{"size_1519_max_packets",  offsetof(struct virtnet_rx, stats.size_bins[7])},
 };
 
 /* [rt]x_qX_ is prepended to the name string here */
@@ -142,8 +142,8 @@ static const struct rte_virtio_xstats_name_off rte_virtio_txq_stat_strings[] = {
 	{"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])},
+	{"size_1024_1518_packets", offsetof(struct virtnet_tx, stats.size_bins[6])},
+	{"size_1519_max_packets",  offsetof(struct virtnet_tx, stats.size_bins[7])},
 };
 
 #define VIRTIO_NB_RXQ_XSTATS (sizeof(rte_virtio_rxq_stat_strings) / \
-- 
2.5.5

Re: [PATCH] virtio: xstats name issue

[Yuanhan Liu]

Few generic (trivial) comments first:

- cc to related maintainers 

- follow the right prefix, "net/virtio" but not "virtio"

- start commit summary with a verb, and start with "fix" for bug fixing patch.

On Thu, Sep 01, 2016 at 04:01:14PM +0800, Zhiyong Yang wrote:
> The patch fixes some xstats name issues and make the xstats name conform to
> code implementation(the function virtio_update_packet_stats).

I would fix it inside virtio_update_packet_stats(), to keep the consistency
of name style: starts with an even number, and ends with an odd number.

	--yliu

> 
> Signed-off-by: Zhiyong Yang <zhiyong.yang@intel.com>
> ---
>  drivers/net/virtio/virtio_ethdev.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index 07d6449..4cee067 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -125,8 +125,8 @@ static const struct rte_virtio_xstats_name_off rte_virtio_rxq_stat_strings[] = {
>  	{"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])},
> +	{"size_1024_1518_packets", offsetof(struct virtnet_rx, stats.size_bins[6])},
> +	{"size_1519_max_packets",  offsetof(struct virtnet_rx, stats.size_bins[7])},
>  };
>  
>  /* [rt]x_qX_ is prepended to the name string here */
> @@ -142,8 +142,8 @@ static const struct rte_virtio_xstats_name_off rte_virtio_txq_stat_strings[] = {
>  	{"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])},
> +	{"size_1024_1518_packets", offsetof(struct virtnet_tx, stats.size_bins[6])},
> +	{"size_1519_max_packets",  offsetof(struct virtnet_tx, stats.size_bins[7])},
>  };
>  
>  #define VIRTIO_NB_RXQ_XSTATS (sizeof(rte_virtio_rxq_stat_strings) / \
> -- 
> 2.5.5

Re: [PATCH] virtio: xstats name issue

[Yang, Zhiyong]

Hi, Yuanhan:

	Thanks for your comments and suggestions.

Zhiyong

> -----Original Message-----
> From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> Sent: Monday, September 5, 2016 12:33 PM
> To: Yang, Zhiyong <zhiyong.yang@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] virtio: xstats name issue
> 
> Few generic (trivial) comments first:
> 
> - cc to related maintainers
> 
> - follow the right prefix, "net/virtio" but not "virtio"
> 
> - start commit summary with a verb, and start with "fix" for bug fixing patch.
> 
> On Thu, Sep 01, 2016 at 04:01:14PM +0800, Zhiyong Yang wrote:
> > The patch fixes some xstats name issues and make the xstats name
> > conform to code implementation(the function virtio_update_packet_stats).
> 
> I would fix it inside virtio_update_packet_stats(), to keep the consistency of
> name style: starts with an even number, and ends with an odd number.
> 
> 	--yliu

Re: [PATCH] virtio: xstats name issue

[Yuanhan Liu]

On Mon, Sep 05, 2016 at 12:33:29PM +0800, Yuanhan Liu wrote:
> Few generic (trivial) comments first:
> 
> - cc to related maintainers 
> 
> - follow the right prefix, "net/virtio" but not "virtio"
> 
> - start commit summary with a verb, and start with "fix" for bug fixing patch.

I forgot to mention that you need to add a fixline for bug fix patch.

	--yliu

[PATCH v2] net/virtio: fix xstats name issue

[Zhiyong Yang]

The patch fixes some xstats name issues and make the xstats name conform
to the definition of etherStatsPkts1024to1518Octets in rfc2819 page 23.

Fixes: 76d4c652e07d ("virtio: add extended stats")

---

Changes in v2:

modify commit summary.
add the fixline.

Signed-off-by: Zhiyong Yang <zhiyong.yang@intel.com>
---
 drivers/net/virtio/virtio_ethdev.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 07d6449..4cee067 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -125,8 +125,8 @@ static const struct rte_virtio_xstats_name_off rte_virtio_rxq_stat_strings[] = {
 	{"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])},
+	{"size_1024_1518_packets", offsetof(struct virtnet_rx, stats.size_bins[6])},
+	{"size_1519_max_packets",  offsetof(struct virtnet_rx, stats.size_bins[7])},
 };
 
 /* [rt]x_qX_ is prepended to the name string here */
@@ -142,8 +142,8 @@ static const struct rte_virtio_xstats_name_off rte_virtio_txq_stat_strings[] = {
 	{"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])},
+	{"size_1024_1518_packets", offsetof(struct virtnet_tx, stats.size_bins[6])},
+	{"size_1519_max_packets",  offsetof(struct virtnet_tx, stats.size_bins[7])},
 };
 
 #define VIRTIO_NB_RXQ_XSTATS (sizeof(rte_virtio_rxq_stat_strings) / \
-- 
2.5.5

Re: [PATCH v2] net/virtio: fix xstats name issue

[Yuanhan Liu]

On Tue, Sep 06, 2016 at 03:50:12PM +0800, Zhiyong Yang wrote:
> The patch fixes some xstats name issues

Please, state **clearly** what the issue is: it's far away from being
enough just mentioning "fix a issue" without actually telling what it
is.

For this case, you could describe the issue like:

    We have a stats named "size_1024_1517_packets", while the code
    actually counts the range "[1024, 1518]", which is obviously wrong.

You could even add the related code piece here (you see the context
is missing in the patch, which is harder for reviewer to see what's
actually going wrong):

	else if (s < 1519)
		stats->size_bins[6]++;
    
> and make the xstats name conform
> to the definition of etherStatsPkts1024to1518Octets in rfc2819 page 23.

It's a bit abrupt to metion the RFC here, without some background. It
could be better if we mention something like: (just followed by above
issue description).

    We could either fix it by correcting the "if" check in the code,
    or fix it by just renaming the stats to conform to the code. The
    later solution is taken because that's what the RFC2819 suggests.


> Fixes: 76d4c652e07d ("virtio: add extended stats")
> 
> ---

Besides that, the three dash "---" means the end of your commit log,
therefore, it should go --->

> 
> Changes in v2:
> 
> modify commit summary.
> add the fixline.
> 
> Signed-off-by: Zhiyong Yang <zhiyong.yang@intel.com>

... here

Otherwise, your SoB will be chopped off while apply.

Another thing to note is, it's a good candidate to me for stable branch,
thus, you may need add following in the commit log before you SoB:

    Cc: <stable@dpdk.org>

So, mind to send v3, with above fixes?

Thanks.

	--yliu

Re: [PATCH v2] net/virtio: fix xstats name issue

[Yang, Zhiyong]

hi, yuanhan:

Thanks a lot  for your clear comments in detail. 
Patch v3 will be sent later.

-zhiyong-

> -----Original Message-----
> From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> Sent: Tuesday, September 6, 2016 8:28 PM
> To: Yang, Zhiyong <zhiyong.yang@intel.com>
> Cc: dev@dpdk.org; Xie, Huawei <huawei.xie@intel.com>
> Subject: Re: [PATCH v2] net/virtio: fix xstats name issue
> 
> On Tue, Sep 06, 2016 at 03:50:12PM +0800, Zhiyong Yang wrote:
> > The patch fixes some xstats name issues
> 
> Please, state **clearly** what the issue is: it's far away from being enough
> just mentioning "fix a issue" without actually telling what it is.
> 
> For this case, you could describe the issue like:
> 
>     We have a stats named "size_1024_1517_packets", while the code
>     actually counts the range "[1024, 1518]", which is obviously wrong.
> 
> You could even add the related code piece here (you see the context is
> missing in the patch, which is harder for reviewer to see what's actually going
> wrong):
> 
> 	else if (s < 1519)
> 		stats->size_bins[6]++;
> 
> > and make the xstats name conform
> > to the definition of etherStatsPkts1024to1518Octets in rfc2819 page 23.
> 
> It's a bit abrupt to metion the RFC here, without some background. It could
> be better if we mention something like: (just followed by above issue
> description).
> 
>     We could either fix it by correcting the "if" check in the code,
>     or fix it by just renaming the stats to conform to the code. The
>     later solution is taken because that's what the RFC2819 suggests.
> 
> 
> > Fixes: 76d4c652e07d ("virtio: add extended stats")
> >
> > ---
> 
> Besides that, the three dash "---" means the end of your commit log,
> therefore, it should go --->
> 
> >
> > Changes in v2:
> >
> > modify commit summary.
> > add the fixline.
> >
> > Signed-off-by: Zhiyong Yang <zhiyong.yang@intel.com>
> 
> ... here
> 
> Otherwise, your SoB will be chopped off while apply.
> 
> Another thing to note is, it's a good candidate to me for stable branch, thus,
> you may need add following in the commit log before you SoB:
> 
>     Cc: <stable@dpdk.org>
> 
> So, mind to send v3, with above fixes?
> 
> Thanks.
> 
> 	--yliu

[PATCH v3] net/virtio: fix xstats name issue

[Zhiyong Yang]

We have a stats named "size_1024_1517_packets", while the code
actually counts the range "[1024, 1518]", which is obviously wrong.
The code is as follows in the function virtio_update_packet_stats.

else if (s < 1519)
		stats->size_bins[6]++;

We could either fix it by correcting the "if" check in the code,
or fix it by just renaming the stats to conform to the code. The
latter solution is taken because that's what the RFC2819 suggests.

Fixes: 76d4c652e07d ("virtio: add extended stats")

Cc: <stable@dpdk.org>
Signed-off-by: Zhiyong Yang <zhiyong.yang@intel.com>
---
Changes in v3:

add the issue description in detail.

Changes in v2:

modify commit summary and add the fixline.

 drivers/net/virtio/virtio_ethdev.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 07d6449..4cee067 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -125,8 +125,8 @@ static const struct rte_virtio_xstats_name_off rte_virtio_rxq_stat_strings[] = {
 	{"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])},
+	{"size_1024_1518_packets", offsetof(struct virtnet_rx, stats.size_bins[6])},
+	{"size_1519_max_packets",  offsetof(struct virtnet_rx, stats.size_bins[7])},
 };
 
 /* [rt]x_qX_ is prepended to the name string here */
@@ -142,8 +142,8 @@ static const struct rte_virtio_xstats_name_off rte_virtio_txq_stat_strings[] = {
 	{"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])},
+	{"size_1024_1518_packets", offsetof(struct virtnet_tx, stats.size_bins[6])},
+	{"size_1519_max_packets",  offsetof(struct virtnet_tx, stats.size_bins[7])},
 };
 
 #define VIRTIO_NB_RXQ_XSTATS (sizeof(rte_virtio_rxq_stat_strings) / \
-- 
2.5.5

Re: [dpdk-stable] [PATCH v3] net/virtio: fix xstats name issue

[Yuanhan Liu]

On Wed, Sep 07, 2016 at 02:11:00PM +0800, Zhiyong Yang wrote:
> We have a stats named "size_1024_1517_packets", while the code
> actually counts the range "[1024, 1518]", which is obviously wrong.
> The code is as follows in the function virtio_update_packet_stats.
> 
> else if (s < 1519)
> 		stats->size_bins[6]++;
> 
> We could either fix it by correcting the "if" check in the code,
> or fix it by just renaming the stats to conform to the code. The
> latter solution is taken because that's what the RFC2819 suggests.
> 
> Fixes: 76d4c652e07d ("virtio: add extended stats")
> 
> Cc: <stable@dpdk.org>
> Signed-off-by: Zhiyong Yang <zhiyong.yang@intel.com>

Applied to dpdk-next-virtio.

Thanks!

	--yliu