DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] latency: clear mbuf timestamp after latency calculation
@ 2018-09-19  8:18 longtb5
  0 siblings, 0 replies; 4+ messages in thread
From: longtb5 @ 2018-09-19  8:18 UTC (permalink / raw)
  To: reshma.pattan; +Cc: dev, Bao-Long Tran, stable

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

^ permalink raw reply	[flat|nested] 4+ messages in thread
* [dpdk-dev] Incorrect latencystats implementation
@ 2018-09-19  8:17 longtb5
  2018-09-19  8:22 ` [dpdk-dev] [PATCH] latency: clear mbuf timestamp after latency calculation longtb5
  0 siblings, 1 reply; 4+ messages in thread
From: longtb5 @ 2018-09-19  8:17 UTC (permalink / raw)
  To: reshma.pattan; +Cc: dev, Bao-Long Tran

Currently, lib latencystats works as follow:
    * For every sample period, one packet is selected by RX callback to be later
      used for latency measurement. That packet is marked with a timestamp.  
    * In TX callback, mbufs with non-zero timestamp are used for calculation.

I see a problem with the current implementation. The timestamp field of a mbuf
will not be cleared when that mbuf is freed back to the pool. Thus TX callback
could potentially use an incorrect mbuf, one that had not been selected on RX
but still has non-zero timestamp anyway because it was inherited from a
previously selected packet. The simple fix for this is to reset the timestamp of
the mbuf after it was used for latency calculation.

However, that fix is not enough in the case where a selected mbuf get dropped by
the application before reaching TX. That timestamp will eventually be cleared
the next time that mbuf is used and not get dropped, but then the incorrect
residued timestamp will still be used and it would affect all measurement
results, most severely max_latency_ns.

I have submit a patch to implement the trivial fix. For the drop case I can
think of 2 options. We can either clear timestamp when putting mbufs back to
their pool, or change lib latencystats implementation to perform packet
selection at TX callback and let RX callback add timestamp to every packet. Both
option could affect performance but I think the second option is less
aggressive.

Bao-Long Tran (1):
  latency: clear mbuf timestamp after latency calculation

 lib/librte_latencystats/rte_latencystats.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

-- 
2.7.4

^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-09-20 12:16 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-19  8:18 [dpdk-dev] [PATCH] latency: clear mbuf timestamp after latency calculation longtb5
  -- strict thread matches above, loose matches on Subject: below --
2018-09-19  8:17 [dpdk-dev] Incorrect latencystats implementation longtb5
2018-09-19  8:22 ` [dpdk-dev] [PATCH] latency: clear mbuf timestamp after latency calculation longtb5
2018-09-20 10:25   ` Pattan, Reshma
2018-09-20 12:16     ` longtb5

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).