[dpdk-dev] Incorrect latencystats implementation

[dpdk-dev] Incorrect latencystats implementation

[longtb5]

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

[dpdk-dev] [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-dev] [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>

[dpdk-dev] [PATCH v2] 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.

Fixes: 5cd3cac9ed ("latency: added new library for latency stats").
Cc: stable@dpdk.org

Signed-off-by: Bao-Long Tran <longtb5@viettel.com.vn>
Acked-by: Reshma Pattan <reshma.pattan@intel.com>
---
 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-dev] [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>

Re: [dpdk-dev] Incorrect latencystats implementation

[Pattan, Reshma]

> -----Original Message-----
> From: longtb5@viettel.com.vn [mailto:longtb5@viettel.com.vn]
> Sent: Wednesday, September 19, 2018 9:17 AM
> To: Pattan, Reshma <reshma.pattan@intel.com>
> Cc: dev@dpdk.org; Bao-Long Tran <longtb5@viettel.com.vn>
> Subject: Incorrect latencystats implementation
> 
> 
> 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.

What happens when applications drop the packets? Do they free the mbuf?
In such case, can application set the timestamp to 0 before freeing the mbuf, instead of making
these changes in latency library.? 

Regards,
Reshma

Re: [dpdk-dev] Incorrect latencystats implementation

[longtb5]

Hi Reshma,

> -----Original Message-----
> From: reshma.pattan@intel.com [mailto:reshma.pattan@intel.com]
> Sent: Thursday, September 20, 2018 8:09 PM
> To: longtb5@viettel.com.vn
> Cc: dev@dpdk.org
> Subject: RE: Incorrect latencystats implementation
> 
> 
> 
> > -----Original Message-----
> > From: longtb5@viettel.com.vn [mailto:longtb5@viettel.com.vn]
> > Sent: Wednesday, September 19, 2018 9:17 AM
> > To: Pattan, Reshma <reshma.pattan@intel.com>
> > Cc: dev@dpdk.org; Bao-Long Tran <longtb5@viettel.com.vn>
> > Subject: Incorrect latencystats implementation
> >
> >
> > 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.
> 
> What happens when applications drop the packets? Do they free the mbuf?
> In such case, can application set the timestamp to 0 before freeing the
mbuf,
> instead of making these changes in latency library.?
> 

Yes, applications can set the mbuf timestamp before freeing. But in my
opinion that would not be a clean solution. Applications should not have to
worry about the timestamp field at all, since that is an implementation
detail of the library. For simple apps, wrapping rte_pktmbuf_free() to
perform timestamp reset could be done without much hassle, but that kind of
ad-hoc solution would become messy for more complex ones where packets are
dropped at different places. From a usability point of view, as an user I
want the lib to provide latency measurements without me having to touch
existing codebase other than adding codes that use the APIs.

> Regards,
> Reshma

Thanks and regards,
BL

Re: [dpdk-dev] Incorrect latencystats implementation

[Pattan, Reshma]

Hi,

> -----Original Message-----
> From: longtb5@viettel.com.vn [mailto:longtb5@viettel.com.vn]
> Sent: Friday, September 21, 2018 2:58 AM
> To: Pattan, Reshma <reshma.pattan@intel.com>
> Cc: dev@dpdk.org
> Subject: RE: Incorrect latencystats implementation
> 
> Hi Reshma,
> 
> > -----Original Message-----
> > From: reshma.pattan@intel.com [mailto:reshma.pattan@intel.com]
> > Sent: Thursday, September 20, 2018 8:09 PM
> > To: longtb5@viettel.com.vn
> > Cc: dev@dpdk.org
> > Subject: RE: Incorrect latencystats implementation
> >
> >
> >
> > > -----Original Message-----
> > > From: longtb5@viettel.com.vn [mailto:longtb5@viettel.com.vn]
> > > Sent: Wednesday, September 19, 2018 9:17 AM
> > > To: Pattan, Reshma <reshma.pattan@intel.com>
> > > Cc: dev@dpdk.org; Bao-Long Tran <longtb5@viettel.com.vn>
> > > Subject: Incorrect latencystats implementation
> > >
> > >
> > > 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.
> >
> > What happens when applications drop the packets? Do they free the mbuf?
> > In such case, can application set the timestamp to 0 before freeing
> > the
> mbuf,
> > instead of making these changes in latency library.?
> >
> 
> Yes, applications can set the mbuf timestamp before freeing. But in my
> opinion that would not be a clean solution. Applications should not have to
> worry about the timestamp field at all, since that is an implementation detail
> of the library. For simple apps, wrapping rte_pktmbuf_free() to perform
> timestamp reset could be done without much hassle, but that kind of ad-hoc
> solution would become messy for more complex ones where packets are
> dropped at different places. From a usability point of view, as an user I want
> the lib to provide latency measurements without me having to touch existing
> codebase other than adding codes that use the APIs.
> 

I will send a patch to  add timestamp reset in rte_pktmbuf_free().  That will be a cleaner way  I think.
Let's see what other says on the patch.

Thanks,
Reshma

Re: [dpdk-dev] Incorrect latencystats implementation

[Ananyev, Konstantin]

Hi Reshma,

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Pattan, Reshma
> Sent: Friday, September 21, 2018 12:15 PM
> To: longtb5@viettel.com.vn
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] Incorrect latencystats implementation
> 
> Hi,
> 
> > -----Original Message-----
> > From: longtb5@viettel.com.vn [mailto:longtb5@viettel.com.vn]
> > Sent: Friday, September 21, 2018 2:58 AM
> > To: Pattan, Reshma <reshma.pattan@intel.com>
> > Cc: dev@dpdk.org
> > Subject: RE: Incorrect latencystats implementation
> >
> > Hi Reshma,
> >
> > > -----Original Message-----
> > > From: reshma.pattan@intel.com [mailto:reshma.pattan@intel.com]
> > > Sent: Thursday, September 20, 2018 8:09 PM
> > > To: longtb5@viettel.com.vn
> > > Cc: dev@dpdk.org
> > > Subject: RE: Incorrect latencystats implementation
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: longtb5@viettel.com.vn [mailto:longtb5@viettel.com.vn]
> > > > Sent: Wednesday, September 19, 2018 9:17 AM
> > > > To: Pattan, Reshma <reshma.pattan@intel.com>
> > > > Cc: dev@dpdk.org; Bao-Long Tran <longtb5@viettel.com.vn>
> > > > Subject: Incorrect latencystats implementation
> > > >
> > > >
> > > > 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.
> > >
> > > What happens when applications drop the packets? Do they free the mbuf?
> > > In such case, can application set the timestamp to 0 before freeing
> > > the
> > mbuf,
> > > instead of making these changes in latency library.?
> > >
> >
> > Yes, applications can set the mbuf timestamp before freeing. But in my
> > opinion that would not be a clean solution. Applications should not have to
> > worry about the timestamp field at all, since that is an implementation detail
> > of the library. For simple apps, wrapping rte_pktmbuf_free() to perform
> > timestamp reset could be done without much hassle, but that kind of ad-hoc
> > solution would become messy for more complex ones where packets are
> > dropped at different places. From a usability point of view, as an user I want
> > the lib to provide latency measurements without me having to touch existing
> > codebase other than adding codes that use the APIs.
> >
> 
> I will send a patch to  add timestamp reset in rte_pktmbuf_free().  That will be a cleaner way  I think.
> Let's see what other says on the patch.

That would probably affect performance.
Actually, looking at rte_mbuf.h - timestamp field supposed to be valid only if PKT_RX_TIMESTAMP is set.
>From other side, as I remember, PMD RX routine should reset RX flags of the received PMD.
So in theory you can rely on PKT_RX_TIMESTAMP to determine use this mbuf for latency calcs or not.
Would that help?
Konstantin

> 
> Thanks,
> Reshma

Re: [dpdk-dev] Incorrect latencystats implementation

[Pattan, Reshma]

Hi,

> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Friday, September 21, 2018 1:14 PM
> To: Pattan, Reshma <reshma.pattan@intel.com>; longtb5@viettel.com.vn
> Cc: dev@dpdk.org
> Subject: RE: Incorrect latencystats implementation
> 
> Hi Reshma,
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Pattan, Reshma
> > Sent: Friday, September 21, 2018 12:15 PM
> > To: longtb5@viettel.com.vn
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] Incorrect latencystats implementation
> >
> > Hi,
> >
> > > -----Original Message-----
> > > From: longtb5@viettel.com.vn [mailto:longtb5@viettel.com.vn]
> > > Sent: Friday, September 21, 2018 2:58 AM
> > > To: Pattan, Reshma <reshma.pattan@intel.com>
> > > Cc: dev@dpdk.org
> > > Subject: RE: Incorrect latencystats implementation
> > >
> > > Hi Reshma,
> > >
> > > > -----Original Message-----
> > > > From: reshma.pattan@intel.com [mailto:reshma.pattan@intel.com]
> > > > Sent: Thursday, September 20, 2018 8:09 PM
> > > > To: longtb5@viettel.com.vn
> > > > Cc: dev@dpdk.org
> > > > Subject: RE: Incorrect latencystats implementation
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: longtb5@viettel.com.vn [mailto:longtb5@viettel.com.vn]
> > > > > Sent: Wednesday, September 19, 2018 9:17 AM
> > > > > To: Pattan, Reshma <reshma.pattan@intel.com>
> > > > > Cc: dev@dpdk.org; Bao-Long Tran <longtb5@viettel.com.vn>
> > > > > Subject: Incorrect latencystats implementation
> > > > >
> > > > >
> > > > > 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.
> > > >
> > > > What happens when applications drop the packets? Do they free the
> mbuf?
> > > > In such case, can application set the timestamp to 0 before
> > > > freeing the
> > > mbuf,
> > > > instead of making these changes in latency library.?
> > > >
> > >
> > > Yes, applications can set the mbuf timestamp before freeing. But in
> > > my opinion that would not be a clean solution. Applications should
> > > not have to worry about the timestamp field at all, since that is an
> > > implementation detail of the library. For simple apps, wrapping
> > > rte_pktmbuf_free() to perform timestamp reset could be done without
> > > much hassle, but that kind of ad-hoc solution would become messy for
> > > more complex ones where packets are dropped at different places.
> > > From a usability point of view, as an user I want the lib to provide
> > > latency measurements without me having to touch existing codebase
> other than adding codes that use the APIs.
> > >
> >
> > I will send a patch to  add timestamp reset in rte_pktmbuf_free().  That will
> be a cleaner way  I think.
> > Let's see what other says on the patch.
> 
> That would probably affect performance.
> Actually, looking at rte_mbuf.h - timestamp field supposed to be valid only if
> PKT_RX_TIMESTAMP is set.
> From other side, as I remember, PMD RX routine should reset RX flags of the
> received PMD.
> So in theory you can rely on PKT_RX_TIMESTAMP to determine use this mbuf
> for latency calcs or not.
> Would that help?

Yes this will work. Thanks for the suggestion. Let me have a further look and send the patch.

[dpdk-dev] [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