[dpdk-stable] [PATCH] latency: clear mbuf timestamp after latency calculation

[dpdk-stable] [PATCH] latency: clear mbuf timestamp after latency calculation

[longtb5]

The timestamp of a mbuf should be cleared after that mbuf was used for
latency calculation, otherwise future packets which reuse the same mbuf
would inherit that previous timestamp. The latencystats library looks
for mbuf with non-zero timestamp, thus incorrectly inherited value would
result in incorrect latency measurement.

Cc: stable@dpdk.org

Signed-off-by: Bao-Long Tran <longtb5@viettel.com.vn>
---
 lib/librte_latencystats/rte_latencystats.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/librte_latencystats/rte_latencystats.c b/lib/librte_latencystats/rte_latencystats.c
index 1fdec68..2d5384e 100644
--- a/lib/librte_latencystats/rte_latencystats.c
+++ b/lib/librte_latencystats/rte_latencystats.c
@@ -156,8 +156,10 @@ calc_latency(uint16_t pid __rte_unused,
 
 	now = rte_rdtsc();
 	for (i = 0; i < nb_pkts; i++) {
-		if (pkts[i]->timestamp)
+		if (pkts[i]->timestamp) {
 			latency[cnt++] = now - pkts[i]->timestamp;
+			pkts[i]->timestamp = 0;
+		}
 	}
 
 	for (i = 0; i < cnt; i++) {
-- 
2.7.4

[dpdk-stable] [PATCH] latency: clear mbuf timestamp after latency calculation

[longtb5]

The timestamp of a mbuf should be cleared after that mbuf was used for
latency calculation, otherwise future packets which reuse the same mbuf
would inherit that previous timestamp. The latencystats library looks
for mbuf with non-zero timestamp, thus incorrectly inherited value would
result in incorrect latency measurement.

Cc: stable@dpdk.org

Signed-off-by: Bao-Long Tran <longtb5@viettel.com.vn>
---
 lib/librte_latencystats/rte_latencystats.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/librte_latencystats/rte_latencystats.c b/lib/librte_latencystats/rte_latencystats.c
index 1fdec68..2d5384e 100644
--- a/lib/librte_latencystats/rte_latencystats.c
+++ b/lib/librte_latencystats/rte_latencystats.c
@@ -156,8 +156,10 @@ calc_latency(uint16_t pid __rte_unused,
 
 	now = rte_rdtsc();
 	for (i = 0; i < nb_pkts; i++) {
-		if (pkts[i]->timestamp)
+		if (pkts[i]->timestamp) {
 			latency[cnt++] = now - pkts[i]->timestamp;
+			pkts[i]->timestamp = 0;
+		}
 	}
 
 	for (i = 0; i < cnt; i++) {
-- 
2.7.4

Re: [dpdk-stable] [PATCH] latency: clear mbuf timestamp after latency calculation

[Pattan, Reshma]

Hi,

> -----Original Message-----
> From: longtb5@viettel.com.vn [mailto:longtb5@viettel.com.vn]
> Sent: Wednesday, September 19, 2018 9:23 AM
> To: Pattan, Reshma <reshma.pattan@intel.com>
> Cc: dev@dpdk.org; Bao-Long Tran <longtb5@viettel.com.vn>;
> stable@dpdk.org
> Subject: [PATCH] latency: clear mbuf timestamp after latency calculation
> 
> The timestamp of a mbuf should be cleared after that mbuf was used for
> latency calculation, otherwise future packets which reuse the same mbuf
> would inherit that previous timestamp. The latencystats library looks for
> mbuf with non-zero timestamp, thus incorrectly inherited value would result
> in incorrect latency measurement.
> 
> Cc: stable@dpdk.org
> 
> Signed-off-by: Bao-Long Tran <longtb5@viettel.com.vn>

You need to add the Fixes line just before CC:  in the commit message. 

Original commit that introduced the bug was  5cd3cac9ed. So fixes should  be  added like below
Fixes: 5cd3cac9ed ("latency: added new library for latency stats").  

You can send v2 with fixes line and my ack. Other than that 

Acked-by: Reshma Pattan <reshma.pattan@intel.com>

Re: [dpdk-stable] [PATCH] latency: clear mbuf timestamp after latency calculation

[longtb5]

Hi,

Thanks, I have sent a v2. 

Any comment on the problem of dropped mbuf that I described in the cover
letter?  In our application the  max_latency_ns metric is useless since
after running for a while it would always take on obviously incorrect value
(up to a few minutes). I suspect the impact on avg_latency_ns is much less
severe but significant nonetheless.

> -----Original Message-----
> From: reshma.pattan@intel.com [mailto:reshma.pattan@intel.com]
> Sent: Thursday, September 20, 2018 5:25 PM
> To: longtb5@viettel.com.vn
> Cc: dev@dpdk.org; stable@dpdk.org
> Subject: RE: [PATCH] latency: clear mbuf timestamp after latency
calculation
> 
> Hi,
> 
> > -----Original Message-----
> > From: longtb5@viettel.com.vn [mailto:longtb5@viettel.com.vn]
> > Sent: Wednesday, September 19, 2018 9:23 AM
> > To: Pattan, Reshma <reshma.pattan@intel.com>
> > Cc: dev@dpdk.org; Bao-Long Tran <longtb5@viettel.com.vn>;
> > stable@dpdk.org
> > Subject: [PATCH] latency: clear mbuf timestamp after latency
> > calculation
> >
> > The timestamp of a mbuf should be cleared after that mbuf was used for
> > latency calculation, otherwise future packets which reuse the same
> > mbuf would inherit that previous timestamp. The latencystats library
> > looks for mbuf with non-zero timestamp, thus incorrectly inherited
> > value would result in incorrect latency measurement.
> >
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Bao-Long Tran <longtb5@viettel.com.vn>
> 
> You need to add the Fixes line just before CC:  in the commit message.
> 
> Original commit that introduced the bug was  5cd3cac9ed. So fixes should
be
> added like below
> Fixes: 5cd3cac9ed ("latency: added new library for latency stats").
> 
> You can send v2 with fixes line and my ack. Other than that
> 
> Acked-by: Reshma Pattan <reshma.pattan@intel.com>

[dpdk-stable] [PATCH] latency: clear mbuf timestamp after latency calculation

[longtb5]

The timestamp of a mbuf should be cleared after that mbuf was used for
latency calculation, otherwise future packets which reuse the same mbuf
would inherit that previous timestamp. The latencystats library looks
for mbuf with non-zero timestamp, thus incorrectly inherited value would
result in incorrect latency measurement.

Cc: stable@dpdk.org

Signed-off-by: Bao-Long Tran <longtb5@viettel.com.vn>
---
 lib/librte_latencystats/rte_latencystats.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/librte_latencystats/rte_latencystats.c b/lib/librte_latencystats/rte_latencystats.c
index 1fdec68..2d5384e 100644
--- a/lib/librte_latencystats/rte_latencystats.c
+++ b/lib/librte_latencystats/rte_latencystats.c
@@ -156,8 +156,10 @@ calc_latency(uint16_t pid __rte_unused,
 
 	now = rte_rdtsc();
 	for (i = 0; i < nb_pkts; i++) {
-		if (pkts[i]->timestamp)
+		if (pkts[i]->timestamp) {
 			latency[cnt++] = now - pkts[i]->timestamp;
+			pkts[i]->timestamp = 0;
+		}
 	}
 
 	for (i = 0; i < cnt; i++) {
-- 
2.7.4

Re: [dpdk-stable] [PATCH] latency: clear mbuf timestamp after latency calculation

[Luca Boccassi]

On Wed, 2018-09-19 at 15:15 +0700, longtb5@viettel.com.vn wrote:
> The timestamp of a mbuf should be cleared after that mbuf was used
> for
> latency calculation, otherwise future packets which reuse the same
> mbuf
> would inherit that previous timestamp. The latencystats library looks
> for mbuf with non-zero timestamp, thus incorrectly inherited value
> would
> result in incorrect latency measurement.
> 
> Cc: stable@dpdk.org
> 
> Signed-off-by: Bao-Long Tran <longtb5@viettel.com.vn>
> ---
>  lib/librte_latencystats/rte_latencystats.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_latencystats/rte_latencystats.c
> b/lib/librte_latencystats/rte_latencystats.c
> index 1fdec68..2d5384e 100644
> --- a/lib/librte_latencystats/rte_latencystats.c
> +++ b/lib/librte_latencystats/rte_latencystats.c
> @@ -156,8 +156,10 @@ calc_latency(uint16_t pid __rte_unused,
>  
>  	now = rte_rdtsc();
>  	for (i = 0; i < nb_pkts; i++) {
> -		if (pkts[i]->timestamp)
> +		if (pkts[i]->timestamp) {
>  			latency[cnt++] = now - pkts[i]->timestamp;
> +			pkts[i]->timestamp = 0;
> +		}
>  	}
>  
>  	for (i = 0; i < cnt; i++) {

Hi,

Is this patch only for stable releases? If so, which one? Or was dev@
absent by mistake?

Thanks

-- 
Kind regards,
Luca Boccassi