[dpdk-dev] [PATCH] latencystats: fix timestamp marking and latency calculation

[dpdk-dev] [PATCH] latencystats: fix timestamp marking and latency calculation

[Reshma Pattan]

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>
---
 lib/librte_latencystats/rte_latencystats.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/lib/librte_latencystats/rte_latencystats.c b/lib/librte_latencystats/rte_latencystats.c
index 1fdec68e3..8870226bb 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,8 @@ 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) &&
+				pkts[i]->timestamp)
 			latency[cnt++] = now - pkts[i]->timestamp;
 	}
 
-- 
2.14.4

Re: [dpdk-dev] [PATCH] latencystats: fix timestamp marking and latency calculation

[longtb5]

Hi Reshma,

> -----Original Message-----
> From: reshma.pattan@intel.com [mailto:reshma.pattan@intel.com]
> Sent: Friday, September 21, 2018 11:02 PM
> To: longtb5@viettel.com.vn; konstantin.ananyev@intel.com; dev@dpdk.org
> Cc: Reshma Pattan <reshma.pattan@intel.com>
> Subject: [PATCH] latencystats: fix timestamp marking and latency
calculation
> 
> 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.
> 

This solution as suggested by Konstantin is great. Not only does it solve
the problem but also now the usage of mbuf->timestamp is not exclusive to
latencystats anymore. The application can make use of timestamp at the same
as latencystats simply by toggling PKT_RX_TIMESTAMP. I think we should
update the doc to include this information. 

> 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>
> ---
>  lib/librte_latencystats/rte_latencystats.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/librte_latencystats/rte_latencystats.c
> b/lib/librte_latencystats/rte_latencystats.c
> index 1fdec68e3..8870226bb 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,8 @@ 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) &&
> +				pkts[i]->timestamp)

Just a nit, but I think we don't have to check for pkts[i]->timestamp here.

>  			latency[cnt++] = now - pkts[i]->timestamp;
>  	}
> 
> --
> 2.14.4

Best regards,
BL

Re: [dpdk-dev] [PATCH] latencystats: fix timestamp marking and latency calculation

[longtb5]

> -----Original Message-----
> From: reshma.pattan@intel.com [mailto:reshma.pattan@intel.com]
> Sent: Friday, September 21, 2018 11:02 PM
> To: longtb5@viettel.com.vn; konstantin.ananyev@intel.com; dev@dpdk.org
> Cc: Reshma Pattan <reshma.pattan@intel.com>
> Subject: [PATCH] latencystats: fix timestamp marking and latency
calculation
> 
> 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>

Re: [dpdk-dev] [PATCH] latencystats: fix timestamp marking and latency calculation

[Pattan, Reshma]

Hi,

> -----Original Message-----
> From: longtb5@viettel.com.vn [mailto:longtb5@viettel.com.vn]
> Sent: Saturday, September 22, 2018 3:58 AM
> To: Pattan, Reshma <reshma.pattan@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; dev@dpdk.org
> Subject: RE: [PATCH] latencystats: fix timestamp marking and latency
> calculation
> 
> Hi Reshma,
> 
> > -----Original Message-----
> > From: reshma.pattan@intel.com [mailto:reshma.pattan@intel.com]
> > Sent: Friday, September 21, 2018 11:02 PM
> > To: longtb5@viettel.com.vn; konstantin.ananyev@intel.com;
> dev@dpdk.org
> > Cc: Reshma Pattan <reshma.pattan@intel.com>
> > Subject: [PATCH] latencystats: fix timestamp marking and latency
> calculation
> >
> > 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.
> >
> 
> This solution as suggested by Konstantin is great. Not only does it solve the
> problem but also now the usage of mbuf->timestamp is not exclusive to
> latencystats anymore. The application can make use of timestamp at the
> same as latencystats simply by toggling PKT_RX_TIMESTAMP. I think we
> should update the doc to include this information.
> 

Do you mean latency stats document? Or Mbuf doc.  

Thanks,
Reshma

[dpdk-dev] [PATCH v2] latencystats: fix timestamp marking and latency calculation

[Reshma Pattan]

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)) {
 			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

Re: [dpdk-dev] [PATCH v2] latencystats: fix timestamp marking and latency calculation

[Ananyev, Konstantin]

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

[dpdk-dev] [PATCH v3] latencystats: fix timestamp marking and latency calculation

[Reshma Pattan]

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")

CC: stable@dpdk.org

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>
Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

---
v3: style related fix
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..5715549e4 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;
 	}
 
-- 
2.17.1

Re: [dpdk-dev] [PATCH] latencystats: fix timestamp marking and latency calculation

[longtb5]

Hi Reshma,

I mean in the latencystats document. A few lines about what is taken away
by the library (the mbuf timestamp when PKT_RX_TIMESTAMP is set) would be
very helpful.

Best regards,
BL

----- Original Message -----
From: "reshma pattan" <reshma.pattan@intel.com>
To: longtb5@viettel.com.vn
Cc: "Konstantin Ananyev" <konstantin.ananyev@intel.com>, dev@dpdk.org
Sent: Monday, September 24, 2018 8:07:18 PM
Subject: RE: [PATCH] latencystats: fix timestamp marking and latency calculation

Hi,

> -----Original Message-----
> From: longtb5@viettel.com.vn [mailto:longtb5@viettel.com.vn]
> Sent: Saturday, September 22, 2018 3:58 AM
> To: Pattan, Reshma <reshma.pattan@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; dev@dpdk.org
> Subject: RE: [PATCH] latencystats: fix timestamp marking and latency
> calculation
> 
> Hi Reshma,
> 
> > -----Original Message-----
> > From: reshma.pattan@intel.com [mailto:reshma.pattan@intel.com]
> > Sent: Friday, September 21, 2018 11:02 PM
> > To: longtb5@viettel.com.vn; konstantin.ananyev@intel.com;
> dev@dpdk.org
> > Cc: Reshma Pattan <reshma.pattan@intel.com>
> > Subject: [PATCH] latencystats: fix timestamp marking and latency
> calculation
> >
> > 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.
> >
> 
> This solution as suggested by Konstantin is great. Not only does it solve the
> problem but also now the usage of mbuf->timestamp is not exclusive to
> latencystats anymore. The application can make use of timestamp at the
> same as latencystats simply by toggling PKT_RX_TIMESTAMP. I think we
> should update the doc to include this information.
> 

Do you mean latency stats document? Or Mbuf doc.  

Thanks,
Reshma

Re: [dpdk-dev] [PATCH v3] latencystats: fix timestamp marking and latency calculation

[Thomas Monjalon]

25/09/2018 16:51, Reshma Pattan:
> 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")
> 
> CC: stable@dpdk.org
> 
> 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>
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> 
> ---
> v3: style related fix
> v2: remove check for mbuf->timestamp
> ---

Applied, thanks