[dpdk-dev] [PATCH v3] pipeline: add statistics for librte_pipeline
2015-05-28 19:26, Dumitrescu, Cristian: > I think we have the following options identified so far for stats collection > configuration: > > 1. Stats configuration through the RTE_LOG_LEVEL > 2. Single configuration flag global for all DPDK libraries > 3. Single configuration flag per DPDK library > > It would be good if Thomas and Stephen, as well as others, would reply with > their preference order. There are important design questions in these threads. I think that the best way to come to a conclusion is to submit a design rule - to state whether statistics must be considered as a feature or as debug, - and to decide whether stats must be always available or disabled globally or per-library. It should be written in a new doc. I suggest docs/guidelines/design.rst. Then we'll have to discuss and vote on this base. It will avoid future discussions. The underlying discussion is to decide if every cycle is important even if there is a real usability drawback. In order to reach a conclusion, it seems reasonnable to target a consensus 2 weeks after the first submission of these design rules. Thanks Cristian to follow up.
[dpdk-dev] [PATCH v3] pipeline: add statistics for librte_pipeline
The confusion is due to whether you consider stats as a library feature or Debug feature. Mostly log levels are considered as debug features in the production system and controlled system wide flag not per library flags. While statistics could be considered as a library feature which could be turned on and off depends on the application needs. I am with Cristian to have per library feature configuration flag for statistics. Regards Kannan Babu -Original Message- From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Dumitrescu, Cristian Sent: Friday, May 29, 2015 12:56 AM To: Rajagopalan Sivaramakrishnan; dev at dpdk.org Subject: Re: [dpdk-dev] [PATCH v3] pipeline: add statistics for librte_pipeline Hi Raja, Thanks for your input. I think we have the following options identified so far for stats collection configuration: 1. Stats configuration through the RTE_LOG_LEVEL 2. Single configuration flag global for all DPDK libraries 3. Single configuration flag per DPDK library It would be good if Thomas and Stephen, as well as others, would reply with their preference order. My personal preference order is: 3., 2., 1., but I can work with any of the above that is identified by the majority of the replies. My goal right now is reaching a conclusion on this item as soon as we can. Regards, Cristian > -Original Message- > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Rajagopalan > Sivaramakrishnan > Sent: Wednesday, May 27, 2015 11:45 PM > To: dev at dpdk.org > Subject: Re: [dpdk-dev] [PATCH v3] pipeline: add statistics for > librte_pipeline > > > > > You also reiterate that you would like to have the stats always enabled. > You > > can definitely do this, it is one of the available choices, but why > > not also accommodate the users that want to pick the opposite > > choice? Why force apps to spend cycles on stats if the app either > > does not want these > counters > > (library counters not relevant for that app, maybe the app is only > interested > > in maintaining some other stats that it implements itself) or do not > > want them anymore (maybe they only needed them during debug phase), etc? > > Jay asked this question, and I did my best in my reply to describe > > our motivation (http://www.dpdk.org/ml/archives/dev/2015- > May/017992.html). > > Maybe you missed that post, it would be good to get your reply on > > this one too. > > > > I want to see DPDK get out of the config madness. > > This is real code, not an Intel benchmark special. > > > I agree that statistics will definitely be required in most real-world > production environments and the overhead from per-core stats gathering > will be minimal if the data structures are such that CPU cache > thrashing is avoided. > However, if there are scenarios where it is desirable to turn stats > off, I think we can live with a config option. > I am not comfortable with using the log level to enable/disable > statistics as they are not really related. A separate config option > for stats collection seems like a reasonable compromise. > > Raja
[dpdk-dev] [PATCH v3] pipeline: add statistics for librte_pipeline
My first preference would be to enable stats always. However, if the majority feels that it should be optional, your preference of 3, 2, 1 seems fine to me. I hope the same decision will apply to port/table/other stats. Raja On 5/28/15, 12:26 PM, "Dumitrescu, Cristian" wrote: >Hi Raja, > >Thanks for your input. > >I think we have the following options identified so far for stats >collection configuration: > >1. Stats configuration through the RTE_LOG_LEVEL >2. Single configuration flag global for all DPDK libraries >3. Single configuration flag per DPDK library > >It would be good if Thomas and Stephen, as well as others, would reply >with their preference order. > >My personal preference order is: 3., 2., 1., but I can work with any of >the above that is identified by the majority of the replies. My goal >right now is reaching a conclusion on this item as soon as we can. > >Regards, >Cristian > > > >> -Original Message- >> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Rajagopalan >> Sivaramakrishnan >> Sent: Wednesday, May 27, 2015 11:45 PM >> To: dev at dpdk.org >> Subject: Re: [dpdk-dev] [PATCH v3] pipeline: add statistics for >>librte_pipeline >> >> >> > > You also reiterate that you would like to have the stats always >>enabled. >> You >> > can definitely do this, it is one of the available choices, but why >>not also >> > accommodate the users that want to pick the opposite choice? Why force >> > apps to spend cycles on stats if the app either does not want these >> counters >> > (library counters not relevant for that app, maybe the app is only >> interested >> > in maintaining some other stats that it implements itself) or do not >>want >> > them anymore (maybe they only needed them during debug phase), etc? >> > Jay asked this question, and I did my best in my reply to describe our >> > motivation (http://www.dpdk.org/ml/archives/dev/2015- >> May/017992.html). >> > Maybe you missed that post, it would be good to get your reply on >>this one >> > too. >> > >> > I want to see DPDK get out of the config madness. >> > This is real code, not an Intel benchmark special. >> >> >> I agree that statistics will definitely be required in most real-world >>production >> environments and the overhead >> from per-core stats gathering will be minimal if the data structures >>are such >> that CPU cache thrashing is avoided. >> However, if there are scenarios where it is desirable to turn stats >>off, I think >> we can live with a config option. >> I am not comfortable with using the log level to enable/disable >>statistics as >> they are not really related. A >> separate config option for stats collection seems like a reasonable >> compromise. >> >> Raja
[dpdk-dev] [PATCH v3] pipeline: add statistics for librte_pipeline
Hi Raja, Thanks for your input. I think we have the following options identified so far for stats collection configuration: 1. Stats configuration through the RTE_LOG_LEVEL 2. Single configuration flag global for all DPDK libraries 3. Single configuration flag per DPDK library It would be good if Thomas and Stephen, as well as others, would reply with their preference order. My personal preference order is: 3., 2., 1., but I can work with any of the above that is identified by the majority of the replies. My goal right now is reaching a conclusion on this item as soon as we can. Regards, Cristian > -Original Message- > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Rajagopalan > Sivaramakrishnan > Sent: Wednesday, May 27, 2015 11:45 PM > To: dev at dpdk.org > Subject: Re: [dpdk-dev] [PATCH v3] pipeline: add statistics for > librte_pipeline > > > > > You also reiterate that you would like to have the stats always enabled. > You > > can definitely do this, it is one of the available choices, but why not also > > accommodate the users that want to pick the opposite choice? Why force > > apps to spend cycles on stats if the app either does not want these > counters > > (library counters not relevant for that app, maybe the app is only > interested > > in maintaining some other stats that it implements itself) or do not want > > them anymore (maybe they only needed them during debug phase), etc? > > Jay asked this question, and I did my best in my reply to describe our > > motivation (http://www.dpdk.org/ml/archives/dev/2015- > May/017992.html). > > Maybe you missed that post, it would be good to get your reply on this one > > too. > > > > I want to see DPDK get out of the config madness. > > This is real code, not an Intel benchmark special. > > > I agree that statistics will definitely be required in most real-world > production > environments and the overhead > from per-core stats gathering will be minimal if the data structures are such > that CPU cache thrashing is avoided. > However, if there are scenarios where it is desirable to turn stats off, I > think > we can live with a config option. > I am not comfortable with using the log level to enable/disable statistics as > they are not really related. A > separate config option for stats collection seems like a reasonable > compromise. > > Raja
[dpdk-dev] [PATCH v3] pipeline: add statistics for librte_pipeline ports and tables
> -Original Message-
> From: Stephen Hemminger [mailto:stephen at networkplumber.org]
> Sent: Tuesday, May 26, 2015 10:48 PM
> To: Dumitrescu, Cristian
> Cc: Gajdzica, MaciejX T; dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3] pipeline: add statistics for
> librte_pipeline
> ports and tables
>
> On Tue, 26 May 2015 21:35:22 +
> "Dumitrescu, Cristian" wrote:
>
> >
> >
> > > -Original Message-
> > > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Stephen
> > > Hemminger
> > > Sent: Tuesday, May 26, 2015 3:57 PM
> > > To: Gajdzica, MaciejX T
> > > Cc: dev at dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH v3] pipeline: add statistics for
> librte_pipeline
> > > ports and tables
> > >
> > > On Tue, 26 May 2015 15:39:18 +0200
> > > Maciej Gajdzica wrote:
> > >
> > > > +#if RTE_LOG_LEVEL == RTE_LOG_DEBUG
> > > > +#define RTE_PIPELINE_STATS_ADD(counter, val) \
> > > > + ({ (counter) += (val); })
> > > > +
> > > > +#define RTE_PIPELINE_STATS_ADD_M(counter, mask) \
> > > > + ({ (counter) += __builtin_popcountll(mask); })
> > > > +#else
> > > > +#define RTE_PIPELINE_STATS_ADD(counter, val)
> > > > +#define RTE_PIPELINE_STATS_ADD_M(counter, mask)
> > > > +#endif
> > >
> > > This is worse idea than the previous one.
> > > I want statistics done on a per lcore basis, and always available
> > > because real users on production system want statistics (but they
> > > don't want log spam).
> >
> > Stephen,
> >
> > Thomas and myself converged towards this solution, Thomas asked if
> anybody objects, you did not (http://www.dpdk.org/ml/archives/dev/2015-
> May/018099.html) . You say this idea is bad, but what exactly is your
> objection? Do you have an alternative proposal?
>
> Yes. Always keep statistics.
>
> We use functions like this internally.
>
> struct xxx_stats {
> uint64_t mib[XXX_MIB_MAX];
> };
> extern struct xxx_stats xxx_stats[RTE_MAX_LCORE];
>
> #define XXXSTAT_INC(type) xxxstats[rte_lcore_id()].mibs[type]++
>
>
This can only be applied if stats are always enabled. I know this is your
preferred choice, but other people have the requirement to be able to have the
stats collection configurable, and we should also accommodate the needs of
those people. Your code snippet does not provide a solution for this problem.
> > You already mentioned in the previous thread you would like to have per
> lcore statistics. I was kindly asking you to describe your idea, but you did
> not
> do this yet (http://www.dpdk.org/ml/archives/dev/2015-May/017956.html ).
> Can you please describe it? Each port instance has its own statistics
> counters.
> Each lcore can run one or more pipeline instances, therefore each lcore
> typically runs several port instances of the same or different type (each port
> instance with its own statistics), so how is "per lcore stats" requirement
> applicable here?
>
> I thought you were familiar with technique of having per-cpu structures and
> variables
> widely used in Linux kernel to prevent cache thrashing. Although you call it
> false sharing,
> it isn't false., it happens when same counter ping-pongs between multiple
> threads for
> no added benefit.
The "per lcore stats" is applicable to stats structures that are accessed by
more than one lcore. In our case, each port instance is handled by a single
lcore, so it is never the case that two lcores would access the stats of the
same port instance. So, we can safely say that port stats are "per lcore"
already.
>
>
> > You also reiterate that you would like to have the stats always enabled. You
> can definitely do this, it is one of the available choices, but why not also
> accommodate the users that want to pick the opposite choice? Why force
> apps to spend cycles on stats if the app either does not want these counters
> (library counters not relevant for that app, maybe the app is only interested
> in maintaining some other stats that it implements itself) or do not want
> them anymore (maybe they only needed them during debug phase), etc?
> Jay asked this question, and I did my best in my reply to describe our
> motivation (http://www.dpdk.org/ml/archives/dev/2015-May/017992.html).
> Maybe you missed that post, it would be good to get your reply on this one
> too.
>
> I want to see DPDK get out of the config madness.
> This is real code, not an Intel benchmark special.
You seem to make this assumption that our real reason for hav
[dpdk-dev] [PATCH v3] pipeline: add statistics for librte_pipeline ports and tables
> -Original Message-
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Stephen
> Hemminger
> Sent: Tuesday, May 26, 2015 3:57 PM
> To: Gajdzica, MaciejX T
> Cc: dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3] pipeline: add statistics for
> librte_pipeline
> ports and tables
>
> On Tue, 26 May 2015 15:39:18 +0200
> Maciej Gajdzica wrote:
>
> > +#if RTE_LOG_LEVEL == RTE_LOG_DEBUG
> > +#define RTE_PIPELINE_STATS_ADD(counter, val) \
> > + ({ (counter) += (val); })
> > +
> > +#define RTE_PIPELINE_STATS_ADD_M(counter, mask) \
> > + ({ (counter) += __builtin_popcountll(mask); })
> > +#else
> > +#define RTE_PIPELINE_STATS_ADD(counter, val)
> > +#define RTE_PIPELINE_STATS_ADD_M(counter, mask)
> > +#endif
>
> This is worse idea than the previous one.
> I want statistics done on a per lcore basis, and always available
> because real users on production system want statistics (but they
> don't want log spam).
Stephen,
Thomas and myself converged towards this solution, Thomas asked if anybody
objects, you did not (http://www.dpdk.org/ml/archives/dev/2015-May/018099.html)
. You say this idea is bad, but what exactly is your objection? Do you have an
alternative proposal?
You already mentioned in the previous thread you would like to have per lcore
statistics. I was kindly asking you to describe your idea, but you did not do
this yet (http://www.dpdk.org/ml/archives/dev/2015-May/017956.html ). Can you
please describe it? Each port instance has its own statistics counters. Each
lcore can run one or more pipeline instances, therefore each lcore typically
runs several port instances of the same or different type (each port instance
with its own statistics), so how is "per lcore stats" requirement applicable
here?
You also reiterate that you would like to have the stats always enabled. You
can definitely do this, it is one of the available choices, but why not also
accommodate the users that want to pick the opposite choice? Why force apps to
spend cycles on stats if the app either does not want these counters (library
counters not relevant for that app, maybe the app is only interested in
maintaining some other stats that it implements itself) or do not want them
anymore (maybe they only needed them during debug phase), etc? Jay asked this
question, and I did my best in my reply to describe our motivation
(http://www.dpdk.org/ml/archives/dev/2015-May/017992.html). Maybe you missed
that post, it would be good to get your reply on this one too.
Thanks,
Cristian
[dpdk-dev] [PATCH v3] pipeline: add statistics for librte_pipeline ports and tables
This patch adds statistics collection for librte_pipeline.
Those statistics ale disabled by default during build time.
Signed-off-by: Pawel Wodkowski
---
lib/librte_pipeline/rte_pipeline.c | 185 +---
lib/librte_pipeline/rte_pipeline.h | 98 +++
2 files changed, 272 insertions(+), 11 deletions(-)
diff --git a/lib/librte_pipeline/rte_pipeline.c
b/lib/librte_pipeline/rte_pipeline.c
index 36d92c9..3ef36ad 100644
--- a/lib/librte_pipeline/rte_pipeline.c
+++ b/lib/librte_pipeline/rte_pipeline.c
@@ -48,6 +48,17 @@
#define RTE_TABLE_INVALID UINT32_MAX
+#if RTE_LOG_LEVEL == RTE_LOG_DEBUG
+#define RTE_PIPELINE_STATS_ADD(counter, val) \
+ ({ (counter) += (val); })
+
+#define RTE_PIPELINE_STATS_ADD_M(counter, mask) \
+ ({ (counter) += __builtin_popcountll(mask); })
+#else
+#define RTE_PIPELINE_STATS_ADD(counter, val)
+#define RTE_PIPELINE_STATS_ADD_M(counter, mask)
+#endif
+
struct rte_port_in {
/* Input parameters */
struct rte_port_in_ops ops;
@@ -63,6 +74,8 @@ struct rte_port_in {
/* List of enabled ports */
struct rte_port_in *next;
+
+ uint64_t n_pkts_dropped_by_ah;
};
struct rte_port_out {
@@ -74,6 +87,8 @@ struct rte_port_out {
/* Handle to low-level port */
void *h_port;
+
+ uint64_t n_pkts_dropped_by_ah;
};
struct rte_table {
@@ -90,6 +105,12 @@ struct rte_table {
/* Handle to the low-level table object */
void *h_table;
+
+ /* Stats for this table. */
+ uint64_t n_pkts_dropped_by_lkp_hit_ah;
+ uint64_t n_pkts_dropped_by_lkp_miss_ah;
+ uint64_t n_pkts_dropped_lkp_hit;
+ uint64_t n_pkts_dropped_lkp_miss;
};
#define RTE_PIPELINE_MAX_NAME_SZ 124
@@ -1040,6 +1061,8 @@ rte_pipeline_action_handler_port_bulk(struct rte_pipeline
*p,
port_out->f_action_bulk(p->pkts, _mask, port_out->arg_ah);
p->action_mask0[RTE_PIPELINE_ACTION_DROP] |= pkts_mask ^ mask;
+ RTE_PIPELINE_STATS_ADD_M(port_out->n_pkts_dropped_by_ah,
+ pkts_mask ^ mask);
}
/* Output port TX */
@@ -1071,6 +1094,9 @@ rte_pipeline_action_handler_port(struct rte_pipeline *p,
uint64_t pkts_mask)
p->action_mask0[RTE_PIPELINE_ACTION_DROP] |=
(pkt_mask ^ 1LLU) << i;
+
RTE_PIPELINE_STATS_ADD(port_out->n_pkts_dropped_by_ah,
+ pkt_mask ^ 1LLU);
+
/* Output port TX */
if (pkt_mask != 0)
port_out->ops.f_tx(port_out->h_port,
@@ -1104,6 +1130,9 @@ rte_pipeline_action_handler_port(struct rte_pipeline *p,
uint64_t pkts_mask)
p->action_mask0[RTE_PIPELINE_ACTION_DROP] |=
(pkt_mask ^ 1LLU) << i;
+
RTE_PIPELINE_STATS_ADD(port_out->n_pkts_dropped_by_ah,
+ pkt_mask ^ 1LLU);
+
/* Output port TX */
if (pkt_mask != 0)
port_out->ops.f_tx(port_out->h_port,
@@ -1140,6 +1169,9 @@ rte_pipeline_action_handler_port_meta(struct rte_pipeline
*p,
p->action_mask0[RTE_PIPELINE_ACTION_DROP] |=
(pkt_mask ^ 1LLU) << i;
+
RTE_PIPELINE_STATS_ADD(port_out->n_pkts_dropped_by_ah,
+ pkt_mask ^ 1ULL);
+
/* Output port TX */
if (pkt_mask != 0)
port_out->ops.f_tx(port_out->h_port,
@@ -1174,6 +1206,9 @@ rte_pipeline_action_handler_port_meta(struct rte_pipeline
*p,
p->action_mask0[RTE_PIPELINE_ACTION_DROP] |=
(pkt_mask ^ 1LLU) << i;
+
RTE_PIPELINE_STATS_ADD(port_out->n_pkts_dropped_by_ah,
+ pkt_mask ^ 1ULL);
+
/* Output port TX */
if (pkt_mask != 0)
port_out->ops.f_tx(port_out->h_port,
@@ -1232,10 +1267,10 @@ rte_pipeline_run(struct rte_pipeline *p)
if (port_in->f_action != NULL) {
uint64_t mask = pkts_mask;
- port_in->f_action(p->pkts, n_pkts, _mask,
- port_in->arg_ah);
- p->action_mask0[RTE_PIPELINE_ACTION_DROP] |=
- pkts_mask ^ mask;
+ port_in->f_action(p->pkts, n_pkts, _mask,
port_in->arg_ah);
+
[dpdk-dev] [PATCH v3] pipeline: add statistics for librte_pipeline ports and tables
On Tue, 26 May 2015 21:35:22 +
"Dumitrescu, Cristian" wrote:
>
>
> > -Original Message-
> > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Stephen
> > Hemminger
> > Sent: Tuesday, May 26, 2015 3:57 PM
> > To: Gajdzica, MaciejX T
> > Cc: dev at dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v3] pipeline: add statistics for
> > librte_pipeline
> > ports and tables
> >
> > On Tue, 26 May 2015 15:39:18 +0200
> > Maciej Gajdzica wrote:
> >
> > > +#if RTE_LOG_LEVEL == RTE_LOG_DEBUG
> > > +#define RTE_PIPELINE_STATS_ADD(counter, val) \
> > > + ({ (counter) += (val); })
> > > +
> > > +#define RTE_PIPELINE_STATS_ADD_M(counter, mask) \
> > > + ({ (counter) += __builtin_popcountll(mask); })
> > > +#else
> > > +#define RTE_PIPELINE_STATS_ADD(counter, val)
> > > +#define RTE_PIPELINE_STATS_ADD_M(counter, mask)
> > > +#endif
> >
> > This is worse idea than the previous one.
> > I want statistics done on a per lcore basis, and always available
> > because real users on production system want statistics (but they
> > don't want log spam).
>
> Stephen,
>
> Thomas and myself converged towards this solution, Thomas asked if anybody
> objects, you did not
> (http://www.dpdk.org/ml/archives/dev/2015-May/018099.html) . You say this
> idea is bad, but what exactly is your objection? Do you have an alternative
> proposal?
Yes. Always keep statistics.
We use functions like this internally.
struct xxx_stats {
uint64_t mib[XXX_MIB_MAX];
};
extern struct xxx_stats xxx_stats[RTE_MAX_LCORE];
#define XXXSTAT_INC(type) xxxstats[rte_lcore_id()].mibs[type]++
> You already mentioned in the previous thread you would like to have per lcore
> statistics. I was kindly asking you to describe your idea, but you did not do
> this yet (http://www.dpdk.org/ml/archives/dev/2015-May/017956.html ). Can you
> please describe it? Each port instance has its own statistics counters. Each
> lcore can run one or more pipeline instances, therefore each lcore typically
> runs several port instances of the same or different type (each port instance
> with its own statistics), so how is "per lcore stats" requirement applicable
> here?
I thought you were familiar with technique of having per-cpu structures and
variables
widely used in Linux kernel to prevent cache thrashing. Although you call it
false sharing,
it isn't false., it happens when same counter ping-pongs between multiple
threads for
no added benefit.
> You also reiterate that you would like to have the stats always enabled. You
> can definitely do this, it is one of the available choices, but why not also
> accommodate the users that want to pick the opposite choice? Why force apps
> to spend cycles on stats if the app either does not want these counters
> (library counters not relevant for that app, maybe the app is only interested
> in maintaining some other stats that it implements itself) or do not want
> them anymore (maybe they only needed them during debug phase), etc? Jay asked
> this question, and I did my best in my reply to describe our motivation
> (http://www.dpdk.org/ml/archives/dev/2015-May/017992.html). Maybe you missed
> that post, it would be good to get your reply on this one too.
I want to see DPDK get out of the config madness.
This is real code, not an Intel benchmark special.
[dpdk-dev] [PATCH v3] pipeline: add statistics for librte_pipeline ports and tables
On Tue, 26 May 2015 15:39:18 +0200
Maciej Gajdzica wrote:
> +#if RTE_LOG_LEVEL == RTE_LOG_DEBUG
> +#define RTE_PIPELINE_STATS_ADD(counter, val) \
> + ({ (counter) += (val); })
> +
> +#define RTE_PIPELINE_STATS_ADD_M(counter, mask) \
> + ({ (counter) += __builtin_popcountll(mask); })
> +#else
> +#define RTE_PIPELINE_STATS_ADD(counter, val)
> +#define RTE_PIPELINE_STATS_ADD_M(counter, mask)
> +#endif
This is worse idea than the previous one.
I want statistics done on a per lcore basis, and always available
because real users on production system want statistics (but they
don't want log spam).
