[v2] latencystats: fix timestamp marking and latency calculation

Message ID 1537878252-21061-1-git-send-email-reshma.pattan@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series [v2] latencystats: fix timestamp marking and latency calculation |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Pattan, Reshma Sept. 25, 2018, 12:24 p.m. UTC
  Latency calculation logic is not correct for the case where
packets gets dropped before TX. As for the dropped packets,
the timestamp is not cleared, and such packets still gets
counted for latency calculation in next runs, that will result
in inaccurate latency measurement.

So fix this issue as below,

Before setting timestamp in mbuf, check mbuf don't have
any prior valid time stamp flag set and after marking
the timestamp, set mbuf flags to indicate timestamp is
valid.

Before calculating timestamp check mbuf flags are set to
indicate timestamp is valid.

With the above logic it is guaranteed that correct timestamps
have been used.

Fixes: 5cd3cac9ed ("latency: added new library for latency stats")

Reported-by: Bao-Long Tran <longtb5@viettel.com.vn>
Signed-off-by: Reshma Pattan <reshma.pattan@intel.com>
Tested-by: Bao-Long Tran <longtb5@viettel.com.vn>

---
v2: remove check for mbuf->timestamp
---
 lib/librte_latencystats/rte_latencystats.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
  

Comments

Ananyev, Konstantin Sept. 25, 2018, 2:30 p.m. UTC | #1
Hi Reshma,

> 
> 
> Latency calculation logic is not correct for the case where
> packets gets dropped before TX. As for the dropped packets,
> the timestamp is not cleared, and such packets still gets
> counted for latency calculation in next runs, that will result
> in inaccurate latency measurement.
> 
> So fix this issue as below,
> 
> Before setting timestamp in mbuf, check mbuf don't have
> any prior valid time stamp flag set and after marking
> the timestamp, set mbuf flags to indicate timestamp is
> valid.
> 
> Before calculating timestamp check mbuf flags are set to
> indicate timestamp is valid.
> 
> With the above logic it is guaranteed that correct timestamps
> have been used.
> 
> Fixes: 5cd3cac9ed ("latency: added new library for latency stats")
> 
> Reported-by: Bao-Long Tran <longtb5@viettel.com.vn>
> Signed-off-by: Reshma Pattan <reshma.pattan@intel.com>
> Tested-by: Bao-Long Tran <longtb5@viettel.com.vn>
> 
> ---
> v2: remove check for mbuf->timestamp
> ---
>  lib/librte_latencystats/rte_latencystats.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/librte_latencystats/rte_latencystats.c b/lib/librte_latencystats/rte_latencystats.c
> index 1fdec68e3..0f702b722 100644
> --- a/lib/librte_latencystats/rte_latencystats.c
> +++ b/lib/librte_latencystats/rte_latencystats.c
> @@ -125,8 +125,11 @@ add_time_stamps(uint16_t pid __rte_unused,
>  	for (i = 0; i < nb_pkts; i++) {
>  		diff_tsc = now - prev_tsc;
>  		timer_tsc += diff_tsc;
> -		if (timer_tsc >= samp_intvl) {
> +
> +		if ((pkts[i]->ol_flags & PKT_RX_TIMESTAMP) == 0
> +			&& (timer_tsc >= samp_intvl)) {

As a nit - I think you need extra tab here, to follow dpdk codying style.
Apart from that:
Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

>  			pkts[i]->timestamp = now;
> +			pkts[i]->ol_flags |= PKT_RX_TIMESTAMP;
>  			timer_tsc = 0;
>  		}
>  		prev_tsc = now;
> @@ -156,7 +159,7 @@ calc_latency(uint16_t pid __rte_unused,
> 
>  	now = rte_rdtsc();
>  	for (i = 0; i < nb_pkts; i++) {
> -		if (pkts[i]->timestamp)
> +		if (pkts[i]->ol_flags & PKT_RX_TIMESTAMP)
>  			latency[cnt++] = now - pkts[i]->timestamp;
>  	}
> 
> --
> 2.14.4
  

Patch

diff --git a/lib/librte_latencystats/rte_latencystats.c b/lib/librte_latencystats/rte_latencystats.c
index 1fdec68e3..0f702b722 100644
--- a/lib/librte_latencystats/rte_latencystats.c
+++ b/lib/librte_latencystats/rte_latencystats.c
@@ -125,8 +125,11 @@  add_time_stamps(uint16_t pid __rte_unused,
 	for (i = 0; i < nb_pkts; i++) {
 		diff_tsc = now - prev_tsc;
 		timer_tsc += diff_tsc;
-		if (timer_tsc >= samp_intvl) {
+
+		if ((pkts[i]->ol_flags & PKT_RX_TIMESTAMP) == 0
+			&& (timer_tsc >= samp_intvl)) {
 			pkts[i]->timestamp = now;
+			pkts[i]->ol_flags |= PKT_RX_TIMESTAMP;
 			timer_tsc = 0;
 		}
 		prev_tsc = now;
@@ -156,7 +159,7 @@  calc_latency(uint16_t pid __rte_unused,
 
 	now = rte_rdtsc();
 	for (i = 0; i < nb_pkts; i++) {
-		if (pkts[i]->timestamp)
+		if (pkts[i]->ol_flags & PKT_RX_TIMESTAMP)
 			latency[cnt++] = now - pkts[i]->timestamp;
 	}