Hi Neil, many thanks for your comments. My answers inline.
> -----Original Message----- > From: Neil McKee [mailto:neil.mc...@inmon.com] > Sent: Tuesday, July 5, 2016 7:36 PM > To: Wojciechowicz, RobertX <robertx.wojciechow...@intel.com> > Cc: dev@openvswitch.org > Subject: Re: [ovs-dev] [PATCH RFC] sflow: Export OVS PMD threads statistics > via sFlow > > Robert, > > Two comments: > > (1) To add an sFlow extension like this you tag it with an > IANA-assigned enterprise number from: > > https://www.iana.org/assignments/enterprise-numbers/enterprise- > numbers > > The enterprise number goes in the high 20 bits, as described on page > 25 of http://sflow.org/sflow_version_5.txt > > So instead of : > > SFLCOUNTERS_OVSPMD = 2208 > > You need something like: > > #define IANA_ENTERPRISE_VMWARE 6876 > SFLCOUNTERS_OVSPMD = (IANA_ENTERPRISE_VMWARE << 12) + 2208 > > Or perhaps: > > #define IANA_ENTERPRISE_INTEL 343 > SFLCOUNTERS_OVSPMD = (IANA_ENTERPRISE_INTEL << 12) + 2208 > > And the "2208" could be "1" since this would be the first structure > from that enterprise (I think). [RW] Sure, I wanted to discuss the idea of exporting PMD statistics via sflow in the first place. I will prepare the correct counter type ID if this idea will be accepted. > > (2) It would be better to export a summary instead. For example: > > struct dpif_pmd_stats { > unsigned num_threads; /* Number of PMD threads */ > uint64_t cycles_polling; /* Number of PMD thread polling cycles */ > uint64_t cycles_processing; /* Number of PMD thread processing cycles */ > uint64_t cycles_total; /* Number of all PMD thread cycles */ > }; > > This would make the data easier to digest and act upon downstream -- > remember, there may be many thousands of agents sending this every 20 > seconds. It would also avoid the potential problem of a large number > of threads resulting in an MTU problem if a single counter-sample was > ever more than 1500 bytes. But of course the particular summary you > choose depends on what decision-making you want to drive with this > feed? [RW] Right, I'm still thinking about this. I modeled it after "ovs-appctl pmd-stats-show", because it would be great to have information how much headroom we have on a particular pmd thread. In the future we could also add other metrics. But as you said for thousands of agents and large number of threads it might be too much. Now I'm starting to think that average core utilization might be enough for the analysis. > > Neil > > > > > On Tue, Jul 5, 2016 at 5:26 AM, Robert Wojciechowicz > <robertx.wojciechow...@intel.com> wrote: > > The OVS PMD threads utilization has been identified > > as important metric when managing large deployments. > > This patch exposes via sFlow PMD utilization metrics, > > which are also available using ovs-appctl utility > > (command: dpif-netdev/pmd-stats-show). > > > > Signed-off-by: Robert Wojciechowicz <robertx.wojciechow...@intel.com> > > --- > > lib/dpif-netdev.c | 38 ++++++++++++++++++++++++ > > lib/dpif-netlink.c | 1 + > > lib/dpif-provider.h | 4 +++ > > lib/dpif.c | 12 ++++++++ > > lib/dpif.h | 10 +++++++ > > lib/sflow.h | 20 ++++++++++++- > > lib/sflow_receiver.c | 19 ++++++++++++ > > ofproto/ofproto-dpif-sflow.c | 70 > +++++++++++++++++++++++++++++++++++++++++++- > > 8 files changed, 172 insertions(+), 2 deletions(-) > > > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > > index 119e169..48367ce 100644 > > --- a/lib/dpif-netdev.c > > +++ b/lib/dpif-netdev.c > > @@ -1100,6 +1100,43 @@ dpif_netdev_get_stats(const struct dpif *dpif, > struct dpif_dp_stats *stats) > > return 0; > > } > > > > +/* Collect PMD and main thread statistics. > > + * Modeled after the appctl utility (pmd-stats-show command). > > + * > > + * XXX: Dynamically allocates array for threads, which should be > > + * freed by caller. > > + */ > > +static int > > +dpif_netdev_get_pmd_stats(const struct dpif *dpif, > > + struct dpif_pmd_stats **stats) > > +{ > > + struct dp_netdev *dp = get_dp_netdev(dpif); > > + int ithr = 0, icyc = 0; > > + struct dp_netdev_pmd_thread *pmd; > > + > > + size_t nthreads = cmap_count(&dp->poll_threads); > > + struct dpif_pmd_stats* pmd_stats = xmalloc(nthreads * sizeof > *pmd_stats); > > + memset(pmd_stats, 0, nthreads * sizeof *pmd_stats); > > + > > + CMAP_FOR_EACH (pmd, node, &dp->poll_threads) { > > + uint64_t cycles[PMD_N_CYCLES]; > > + > > + pmd_stats[ithr].numa_id = pmd->numa_id; > > + pmd_stats[ithr].core_id = pmd->core_id; > > + for (icyc = 0; icyc < ARRAY_SIZE(cycles); icyc++) { > > + atomic_read_relaxed(&pmd->cycles.n[icyc], &cycles[icyc]); > > + } > > + pmd_stats[ithr].cycles_polling = cycles[PMD_CYCLES_POLLING]; > > + pmd_stats[ithr].cycles_processing = > cycles[PMD_CYCLES_PROCESSING]; > > + for (icyc = 0; icyc < ARRAY_SIZE(cycles); icyc++) { > > + pmd_stats[ithr].cycles_total += cycles[icyc]; > > + } > > + ++ithr; > > + } > > + *stats = pmd_stats; > > + return nthreads; > > +} > > + > > static void > > dp_netdev_reload_pmd__(struct dp_netdev_pmd_thread *pmd) > > { > > @@ -4168,6 +4205,7 @@ const struct dpif_class dpif_netdev_class = { > > dpif_netdev_run, > > dpif_netdev_wait, > > dpif_netdev_get_stats, > > + dpif_netdev_get_pmd_stats, > > dpif_netdev_port_add, > > dpif_netdev_port_del, > > dpif_netdev_port_query_by_number, > > diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c > > index 9bff3a8..e6a7e07 100644 > > --- a/lib/dpif-netlink.c > > +++ b/lib/dpif-netlink.c > > @@ -2348,6 +2348,7 @@ const struct dpif_class dpif_netlink_class = { > > dpif_netlink_run, > > NULL, /* wait */ > > dpif_netlink_get_stats, > > + NULL, > > dpif_netlink_port_add, > > dpif_netlink_port_del, > > dpif_netlink_port_query_by_number, > > diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h > > index 25f4280..0768837 100644 > > --- a/lib/dpif-provider.h > > +++ b/lib/dpif-provider.h > > @@ -155,6 +155,10 @@ struct dpif_class { > > /* Retrieves statistics for 'dpif' into 'stats'. */ > > int (*get_stats)(const struct dpif *dpif, struct dpif_dp_stats *stats); > > > > + /* Retrieves PMD statistics for 'dpif' into 'stats'. */ > > + int (*get_pmd_stats)(const struct dpif *dpif, > > + struct dpif_pmd_stats **stats); > > + > > /* Adds 'netdev' as a new port in 'dpif'. If '*port_no' is not > > * UINT32_MAX, attempts to use that as the port's port number. > > * > > diff --git a/lib/dpif.c b/lib/dpif.c > > index c4f24c7..a3a126a 100644 > > --- a/lib/dpif.c > > +++ b/lib/dpif.c > > @@ -503,6 +503,18 @@ dpif_get_dp_stats(const struct dpif *dpif, struct > dpif_dp_stats *stats) > > return error; > > } > > > > +/* Retrieves PMD statistics for 'dpif' into 'stats'. > > + * Returns number of PMDs. */ > > +int > > +dpif_get_pmd_stats(const struct dpif *dpif, struct dpif_pmd_stats > **stats) > > +{ > > + int count = 0; > > + if (dpif->dpif_class->get_pmd_stats) { > > + count = dpif->dpif_class->get_pmd_stats(dpif, stats); > > + } > > + return count; > > +} > > + > > const char * > > dpif_port_open_type(const char *datapath_type, const char *port_type) > > { > > diff --git a/lib/dpif.h b/lib/dpif.h > > index 6788301..05f669b 100644 > > --- a/lib/dpif.h > > +++ b/lib/dpif.h > > @@ -431,6 +431,15 @@ const char *dpif_type(const struct dpif *); > > > > int dpif_delete(struct dpif *); > > > > +/* Statistics for PMD threads. */ > > +struct dpif_pmd_stats { > > + int numa_id; /* PMD thread numa id */ > > + unsigned core_id; /* PMD thread core id */ > > + uint64_t cycles_polling; /* Number of PMD thread polling cycles */ > > + uint64_t cycles_processing; /* Number of PMD thread processing > cycles */ > > + uint64_t cycles_total; /* Number of all PMD thread cycles */ > > +}; > > + > > /* Statistics for a dpif as a whole. */ > > struct dpif_dp_stats { > > uint64_t n_hit; /* Number of flow table matches. */ > > @@ -442,6 +451,7 @@ struct dpif_dp_stats { > > uint32_t n_masks; /* Number of mega flow masks. */ > > }; > > int dpif_get_dp_stats(const struct dpif *, struct dpif_dp_stats *); > > +int dpif_get_pmd_stats(const struct dpif *, struct dpif_pmd_stats **); > > > > > > /* Port operations. */ > > diff --git a/lib/sflow.h b/lib/sflow.h > > index 95bedd9..d55dd5b 100644 > > --- a/lib/sflow.h > > +++ b/lib/sflow.h > > @@ -577,6 +577,22 @@ typedef struct _SFLOVSDP_counters { > > > > #define SFL_CTR_OVSDP_XDR_SIZE 24 > > > > +/* OVS PMD threads stats */ > > + > > +typedef struct _SFLPMDStat { > > + int numa_id; > > + unsigned core_id; > > + uint64_t cycles_polling; > > + uint64_t cycles_processing; > > + uint64_t cycles_total; > > +} SFLPMDStat; > > + > > +typedef struct _SFLOVSPMD_counters { > > + int pmd_count; > > + SFLPMDStat* pmd_stats; > > +} SFLOVSPMD_counters; > > + > > + > > /* Counters data */ > > > > enum SFLCounters_type_tag { > > @@ -590,7 +606,8 @@ enum SFLCounters_type_tag { > > SFLCOUNTERS_OPENFLOWPORT = 1004, > > SFLCOUNTERS_PORTNAME = 1005, > > SFLCOUNTERS_APP_RESOURCES = 2203, > > - SFLCOUNTERS_OVSDP = 2207 > > + SFLCOUNTERS_OVSDP = 2207, > > + SFLCOUNTERS_OVSPMD = 2208 > > }; > > > > typedef union _SFLCounters_type { > > @@ -604,6 +621,7 @@ typedef union _SFLCounters_type { > > SFLPortName portName; > > SFLAPPResources_counters appResources; > > SFLOVSDP_counters ovsdp; > > + SFLOVSPMD_counters ovspmd; > > } SFLCounters_type; > > > > typedef struct _SFLCounters_sample_element { > > diff --git a/lib/sflow_receiver.c b/lib/sflow_receiver.c > > index cde1359..948ae15 100644 > > --- a/lib/sflow_receiver.c > > +++ b/lib/sflow_receiver.c > > @@ -655,6 +655,11 @@ static int > computeCountersSampleSize(SFLReceiver *receiver, > SFL_COUNTERS_SAMPLE_ > > case SFLCOUNTERS_PORTNAME: elemSiz = > stringEncodingLength(&elem->counterBlock.portName.portName); break; > > case SFLCOUNTERS_APP_RESOURCES: elemSiz = > SFL_CTR_APP_RESOURCES_XDR_SIZE; break; > > case SFLCOUNTERS_OVSDP: elemSiz = SFL_CTR_OVSDP_XDR_SIZE; > break; > > + case SFLCOUNTERS_OVSPMD: elemSiz = \ > > + elem->counterBlock.ovspmd.pmd_count \ > > + * sizeof *elem->counterBlock.ovspmd.pmd_stats \ > > + + sizeof elem->counterBlock.ovspmd.pmd_count; > > + break; > > default: > > sflError(receiver, "unexpected counters_tag"); > > return -1; > > @@ -675,6 +680,7 @@ static int computeCountersSampleSize(SFLReceiver > *receiver, SFL_COUNTERS_SAMPLE_ > > int sfl_receiver_writeCountersSample(SFLReceiver *receiver, > SFL_COUNTERS_SAMPLE_TYPE *cs) > > { > > int packedSize; > > + int i = 0; > > if(cs == NULL) return -1; > > // if the sample pkt is full enough so that this sample might put > > // it over the limit, then we should send it now. > > @@ -795,6 +801,19 @@ int sfl_receiver_writeCountersSample(SFLReceiver > *receiver, SFL_COUNTERS_SAMPLE_ > > putNet32(receiver, elem->counterBlock.ovsdp.n_flows); > > putNet32(receiver, elem->counterBlock.ovsdp.n_masks); > > break; > > + case SFLCOUNTERS_OVSPMD: > > + putNet32(receiver, elem->counterBlock.ovspmd.pmd_count); > > + for (i=0; i<elem->counterBlock.ovspmd.pmd_count; i++) { > > + putNet32(receiver, elem- > >counterBlock.ovspmd.pmd_stats[i].numa_id); > > + putNet32(receiver, elem- > >counterBlock.ovspmd.pmd_stats[i].core_id); > > + putNet64(receiver, > > + > > elem->counterBlock.ovspmd.pmd_stats[i].cycles_polling); > > + putNet64(receiver, > > + > > elem->counterBlock.ovspmd.pmd_stats[i].cycles_processing); > > + putNet64(receiver, > > + elem->counterBlock.ovspmd.pmd_stats[i].cycles_total); > > + } > > + break; > > default: > > sflError(receiver, "unexpected counters_tag"); > > return -1; > > diff --git a/ofproto/ofproto-dpif-sflow.c b/ofproto/ofproto-dpif-sflow.c > > index fbc82b7..0699903 100644 > > --- a/ofproto/ofproto-dpif-sflow.c > > +++ b/ofproto/ofproto-dpif-sflow.c > > @@ -216,6 +216,40 @@ sflow_get_dp_stats(struct dpif_sflow *ds > OVS_UNUSED, > > dp_totals->n_mask_hit += dp_stats.n_mask_hit; > > dp_totals->n_masks += dp_stats.n_masks; > > } > > + > > + dpif_close(dpif); > > + } > > + } > > + sset_destroy(&names); > > + } > > + } > > + sset_destroy(&types); > > + return count; > > +} > > + > > +/* Call to get the PMD stats. Modeled after the appctl utility. > > + * > > + * Return number of PMDs found > > + */ > > +static int > > +sflow_get_pmd_stats(struct dpif_sflow *ds OVS_UNUSED, > > + struct dpif_pmd_stats **stats) > > +{ > > + struct sset types; > > + const char *type; > > + int count = 0; > > + > > + sset_init(&types); > > + dp_enumerate_types(&types); > > + SSET_FOR_EACH (type, &types) { > > + struct sset names; > > + const char *name; > > + sset_init(&names); > > + if (dp_enumerate_names(type, &names) == 0) { > > + SSET_FOR_EACH (name, &names) { > > + struct dpif *dpif; > > + if (dpif_open(name, type, &dpif) == 0) { > > + count = dpif_get_pmd_stats(dpif, stats); > > dpif_close(dpif); > > } > > } > > @@ -259,9 +293,12 @@ sflow_agent_get_global_counters(void *ds_, > SFLPoller *poller, > > OVS_REQUIRES(mutex) > > { > > struct dpif_sflow *ds = ds_; > > - SFLCounters_sample_element dp_elem, res_elem; > > + SFLCounters_sample_element dp_elem, res_elem, pmd_elem; > > struct dpif_dp_stats dp_totals; > > + struct dpif_pmd_stats *pmd_stats = NULL; > > struct rusage usage; > > + int i = 0; > > + int count = 0; > > > > if (!sflow_global_counters_subid_test(poller->agent->subId)) { > > /* Another sub-agent is currently responsible for this. */ > > @@ -280,6 +317,29 @@ sflow_agent_get_global_counters(void *ds_, > SFLPoller *poller, > > SFLADD_ELEMENT(cs, &dp_elem); > > } > > > > + /* PMD threads stats */ > > + count = sflow_get_pmd_stats(ds, &pmd_stats); > > + if (count > 0) { > > + memset(&pmd_elem, 0, sizeof pmd_elem); > > + pmd_elem.tag = SFLCOUNTERS_OVSPMD; > > + pmd_elem.counterBlock.ovspmd.pmd_count = count; > > + pmd_elem.counterBlock.ovspmd.pmd_stats = \ > > + xmalloc(count * sizeof > *pmd_elem.counterBlock.ovspmd.pmd_stats); > > + for (i=0; i<count; i++) { > > + pmd_elem.counterBlock.ovspmd.pmd_stats[i].numa_id = \ > > + pmd_stats[i].numa_id; > > + pmd_elem.counterBlock.ovspmd.pmd_stats[i].core_id = \ > > + pmd_stats[i].core_id; > > + pmd_elem.counterBlock.ovspmd.pmd_stats[i].cycles_polling = \ > > + pmd_stats[i].cycles_polling; > > + pmd_elem.counterBlock.ovspmd.pmd_stats[i].cycles_processing = > \ > > + pmd_stats[i].cycles_processing; > > + pmd_elem.counterBlock.ovspmd.pmd_stats[i].cycles_total = \ > > + pmd_stats[i].cycles_total; > > + } > > + SFLADD_ELEMENT(cs, &pmd_elem); > > + } > > + > > /* resource usage */ > > getrusage(RUSAGE_SELF, &usage); > > res_elem.tag = SFLCOUNTERS_APP_RESOURCES; > > @@ -295,7 +355,15 @@ sflow_agent_get_global_counters(void *ds_, > SFLPoller *poller, > > SFL_UNDEF_GAUGE(res_elem.counterBlock.appResources.conn_max); > > > > SFLADD_ELEMENT(cs, &res_elem); > > + > > sfl_poller_writeCountersSample(poller, cs); > > + > > + if (pmd_stats) { > > + free(pmd_elem.counterBlock.ovspmd.pmd_stats); > > + pmd_elem.counterBlock.ovspmd.pmd_stats = NULL; > > + free(pmd_stats); > > + pmd_stats = NULL; > > + } > > } > > > > static void > > -- > > 1.8.3.1 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > http://openvswitch.org/mailman/listinfo/dev _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
