* [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
2018-09-20 13:08 ` [dpdk-dev] Incorrect latencystats implementation Pattan, Reshma
0 siblings, 2 replies; 10+ 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] 10+ messages in thread* [dpdk-dev] [PATCH] latency: clear mbuf timestamp after latency calculation 2018-09-19 8:17 [dpdk-dev] Incorrect latencystats implementation longtb5 @ 2018-09-19 8:22 ` longtb5 2018-09-20 10:25 ` Pattan, Reshma 2018-09-20 11:16 ` [dpdk-dev] [PATCH v2] " longtb5 2018-09-20 13:08 ` [dpdk-dev] Incorrect latencystats implementation Pattan, Reshma 1 sibling, 2 replies; 10+ messages in thread From: longtb5 @ 2018-09-19 8:22 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] 10+ messages in thread
* Re: [dpdk-dev] [PATCH] latency: clear mbuf timestamp after latency calculation 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 2018-09-20 11:16 ` [dpdk-dev] [PATCH v2] " longtb5 1 sibling, 1 reply; 10+ messages in thread From: Pattan, Reshma @ 2018-09-20 10:25 UTC (permalink / raw) To: longtb5; +Cc: dev, stable 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> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [dpdk-dev] [PATCH] latency: clear mbuf timestamp after latency calculation 2018-09-20 10:25 ` Pattan, Reshma @ 2018-09-20 12:16 ` longtb5 0 siblings, 0 replies; 10+ messages in thread From: longtb5 @ 2018-09-20 12:16 UTC (permalink / raw) To: reshma.pattan; +Cc: dev, stable 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> ^ permalink raw reply [flat|nested] 10+ messages in thread
* [dpdk-dev] [PATCH v2] latency: clear mbuf timestamp after latency calculation 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 11:16 ` longtb5 1 sibling, 0 replies; 10+ messages in thread From: longtb5 @ 2018-09-20 11:16 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. 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [dpdk-dev] Incorrect latencystats implementation 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 13:08 ` Pattan, Reshma 2018-09-21 1:58 ` longtb5 1 sibling, 1 reply; 10+ messages in thread From: Pattan, Reshma @ 2018-09-20 13:08 UTC (permalink / raw) To: longtb5; +Cc: dev > -----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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [dpdk-dev] Incorrect latencystats implementation 2018-09-20 13:08 ` [dpdk-dev] Incorrect latencystats implementation Pattan, Reshma @ 2018-09-21 1:58 ` longtb5 2018-09-21 11:15 ` Pattan, Reshma 0 siblings, 1 reply; 10+ messages in thread From: longtb5 @ 2018-09-21 1:58 UTC (permalink / raw) To: reshma.pattan; +Cc: dev 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [dpdk-dev] Incorrect latencystats implementation 2018-09-21 1:58 ` longtb5 @ 2018-09-21 11:15 ` Pattan, Reshma 2018-09-21 12:14 ` Ananyev, Konstantin 0 siblings, 1 reply; 10+ messages in thread From: Pattan, Reshma @ 2018-09-21 11:15 UTC (permalink / raw) To: longtb5; +Cc: dev 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [dpdk-dev] Incorrect latencystats implementation 2018-09-21 11:15 ` Pattan, Reshma @ 2018-09-21 12:14 ` Ananyev, Konstantin 2018-09-21 14:58 ` Pattan, Reshma 0 siblings, 1 reply; 10+ messages in thread From: Ananyev, Konstantin @ 2018-09-21 12:14 UTC (permalink / raw) To: Pattan, Reshma, longtb5; +Cc: dev 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [dpdk-dev] Incorrect latencystats implementation 2018-09-21 12:14 ` Ananyev, Konstantin @ 2018-09-21 14:58 ` Pattan, Reshma 0 siblings, 0 replies; 10+ messages in thread From: Pattan, Reshma @ 2018-09-21 14:58 UTC (permalink / raw) To: Ananyev, Konstantin, longtb5; +Cc: dev 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-09-21 14:58 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 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 2018-09-20 11:16 ` [dpdk-dev] [PATCH v2] " longtb5 2018-09-20 13:08 ` [dpdk-dev] Incorrect latencystats implementation Pattan, Reshma 2018-09-21 1:58 ` longtb5 2018-09-21 11:15 ` Pattan, Reshma 2018-09-21 12:14 ` Ananyev, Konstantin 2018-09-21 14:58 ` Pattan, Reshma
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).