Re: [ovs-dev] [PATCH] netdev-dpdk: Refactor custom stats.
> -Original Message-
> From: Ilya Maximets [mailto:i.maxim...@samsung.com]
> Sent: Wednesday, March 21, 2018 3:07 PM
> To: Weglicki, MichalX ; ovs-dev@openvswitch.org
> Cc: Heetae Ahn ; Ben Pfaff ; Stokes,
> Ian
> Subject: Re: [PATCH] netdev-dpdk: Refactor custom stats.
>
> On 20.03.2018 12:35, Weglicki, MichalX wrote:
> >> -Original Message-
> >> From: Ilya Maximets [mailto:i.maxim...@samsung.com]
> >> Sent: Tuesday, March 20, 2018 9:50 AM
> >> To: Weglicki, MichalX ; ovs-dev@openvswitch.org
> >> Cc: Heetae Ahn ; Ben Pfaff ;
> >> Stokes, Ian
> >> Subject: Re: [PATCH] netdev-dpdk: Refactor custom stats.
> >>
> >> On 19.03.2018 13:22, Weglicki, MichalX wrote:
> >>> Hello,
> >>
> >> Hello.
> >>
> >>>
> >>> I've went through the patch quite carefully.
> >>
> >> Thanks for reviewing this.
> >>
> >>> Main change refactors creation cached IDs and Names from IF-like code
> >>> block to "Goto" code block.
> >>
> >> Current code is over-nested. It has nesting level of 6 (7 including
> >> function definition).
> >> It's like:
> >> netdev_dpdk_configure_xstats
> >> {
> >> if () {
> >> if () {
> >> ...
> >> } else {
> >> ...
> >> if () {
> >> ...
> >> if () {
> >> } else if {
> >> }
> >> ...
> >> if () {
> >> for () {
> >> if () { <-- level #7 !
> >> }
> >> }
> >> } else {
> >> }
> >> }
> >> }
> >> } else {
> >> }
> >> if () {
> >> }
> >> }
> >>
> >>
> >> IMHO, This is not readable. Especially, combining with constant line wraps
> >> because
> >> of limited line lengths. I wanted to make plain execution sequence to
> >> simplify
> >> understanding of the code. Most of the 'if' statements are just error
> >> checking.
> >> And the single exit point from such conditions usually implemented by
> >> 'goto's.
> >> It's a common practice for system programming. It doesn't worth to keep so
> >> deep
> >> nesting level for error checking conditions. This also matches the style
> >> of all
> >> the other code in netdev-dpdk and not only here.
> >
> > For me it is straightforward error checking, so I don't really see an
> > advantage.
> > Not everything is implemented using "goto" in netdev-dpdk. I assume this is
> > preference thing so maybe we could ask someone to make a call, Ben/Ian?
>
>
> Would like to hear some opinions too.
>
> >
> >>
> >>>
> >>> There are also some initializations removal, which I don't personally
> >>> agree with - even if those seems to be not needed, code
> may
> >> always evolve in the future.
> >>
> >> Could you, please, specify?
> > -
> > -dev->rte_xstats_names = NULL;
> > -dev->rte_xstats_names_size = 0;
> > -
> > -dev->rte_xstats_ids = NULL;
> > -dev->rte_xstats_ids_size = 0;
> >
> > I know you are checking it in runtime against other variables, but still I
> > believe that such initialization should remain nevertheless.
> >
> >>
> >>> There is one xcalloc pointless check, true.
> >>>
> >>> The last important thing is that counters configuration can change during
> >>> runtime, and I was facing a problem, where amount
> of
> >> counters was different during some configuration stages. That is why my
> >> initial implementation was looking for counter name
> based
> >> on given ID, not returned index in array => That was a reason to keep
> >> whole list of names, but only limited IDs(filtered out). Also I
> do
> >> think that returned array should be checked against IDs, as implementation
> >> can always change in the future as well.
> >>
> >> Hmm. OK. But I think, in this case we could even more simplify the code.
> >> As I understand, the 'netdev_dpdk_reconfigure' is the only function th
Re: [ovs-dev] [PATCH] netdev-dpdk: Refactor custom stats.
> -Original Message-
> From: Ilya Maximets [mailto:i.maxim...@samsung.com]
> Sent: Tuesday, March 20, 2018 9:50 AM
> To: Weglicki, MichalX ; ovs-dev@openvswitch.org
> Cc: Heetae Ahn ; Ben Pfaff ; Stokes,
> Ian
> Subject: Re: [PATCH] netdev-dpdk: Refactor custom stats.
>
> On 19.03.2018 13:22, Weglicki, MichalX wrote:
> > Hello,
>
> Hello.
>
> >
> > I've went through the patch quite carefully.
>
> Thanks for reviewing this.
>
> > Main change refactors creation cached IDs and Names from IF-like code block
> > to "Goto" code block.
>
> Current code is over-nested. It has nesting level of 6 (7 including function
> definition).
> It's like:
> netdev_dpdk_configure_xstats
> {
> if () {
> if () {
> ...
> } else {
> ...
> if () {
> ...
> if () {
> } else if {
> }
> ...
> if () {
> for () {
> if () { <-- level #7 !
> }
> }
> } else {
> }
> }
> }
> } else {
> }
> if () {
> }
> }
>
>
> IMHO, This is not readable. Especially, combining with constant line wraps
> because
> of limited line lengths. I wanted to make plain execution sequence to simplify
> understanding of the code. Most of the 'if' statements are just error
> checking.
> And the single exit point from such conditions usually implemented by 'goto's.
> It's a common practice for system programming. It doesn't worth to keep so
> deep
> nesting level for error checking conditions. This also matches the style of
> all
> the other code in netdev-dpdk and not only here.
For me it is straightforward error checking, so I don't really see an
advantage.
Not everything is implemented using "goto" in netdev-dpdk. I assume this is
preference thing so maybe we could ask someone to make a call, Ben/Ian?
>
> >
> > There are also some initializations removal, which I don't personally agree
> > with - even if those seems to be not needed, code may
> always evolve in the future.
>
> Could you, please, specify?
-
-dev->rte_xstats_names = NULL;
-dev->rte_xstats_names_size = 0;
-
-dev->rte_xstats_ids = NULL;
-dev->rte_xstats_ids_size = 0;
I know you are checking it in runtime against other variables, but still I
believe that such initialization should remain nevertheless.
>
> > There is one xcalloc pointless check, true.
> >
> > The last important thing is that counters configuration can change during
> > runtime, and I was facing a problem, where amount of
> counters was different during some configuration stages. That is why my
> initial implementation was looking for counter name based
> on given ID, not returned index in array => That was a reason to keep whole
> list of names, but only limited IDs(filtered out). Also I do
> think that returned array should be checked against IDs, as implementation
> can always change in the future as well.
>
> Hmm. OK. But I think, in this case we could even more simplify the code.
> As I understand, the 'netdev_dpdk_reconfigure' is the only function that
> could make influence on the stats counters in HW. Correct me, if I'm wrong.
> In this case we could call 'netdev_dpdk_configure_xstats()' only once at the
> end of reconfiguration. After that all the inconsistency with returned
> from DPDK values should be treated as HW or DPDK error and we should not
> handle this case in OVS and just do not return any stats.
> This will simplify 'netdev_dpdk_get_custom_stats()' too.
I'm not sure if what you are stating is true to be honest -
"'netdev_dpdk_reconfigure' is the only function that
could make influence on the stats counters in HW" - also I'm not sure if it
can be called just once .
And I'm not sure what gain do you really have in mind. Currently If counters
configuration will change, all IDs
and Names will be queried again - as I said this is what I've encountered,
however
It was 3 months ago so I don't recall all the details. Anyhow, I still think
that we can't assume that counters will
remain the same, And we have to guarantee that cached data is up to date.
>
> We also could refactor 'netdev_dpdk_get_stats' in a same way and reuse
> already configured xstats.
> What do you think?
Yes Ilya, to be honest I even had it in my backlog, but didn't manage to do
Re: [ovs-dev] [PATCH] netdev-dpdk: Refactor custom stats.
Hello,
I've went through the patch quite carefully. Main change refactors creation
cached IDs and Names from IF-like code block to "Goto" code block.
There are also some initializations removal, which I don't personally agree
with - even if those seems to be not needed, code may always evolve in the
future. There is one xcalloc pointless check, true.
The last important thing is that counters configuration can change during
runtime, and I was facing a problem, where amount of counters was different
during some configuration stages. That is why my initial implementation was
looking for counter name based on given ID, not returned index in array => That
was a reason to keep whole list of names, but only limited IDs(filtered out).
Also I do think that returned array should be checked against IDs, as
implementation can always change in the future as well.
Br,
Michal.
> -Original Message-
> From: Ilya Maximets [mailto:i.maxim...@samsung.com]
> Sent: Tuesday, January 23, 2018 2:14 PM
> To: ovs-dev@openvswitch.org
> Cc: Heetae Ahn ; Ben Pfaff ; Stokes,
> Ian ; Weglicki, MichalX
> ; Ilya Maximets
> Subject: [PATCH] netdev-dpdk: Refactor custom stats.
>
> This code is overcomplicated and completely unreadable. And a
> bunch of recently fixed memory leaks confirms that statement.
>
> Main concerns that were fixed:
> * Too big nesting level.
> * Useless checks like pointer checking after xmalloc.
> * Misleading comments.
> * Bad names of the variables.
>
> As a bonus, size of the code significantly reduced.
>
> Signed-off-by: Ilya Maximets
> ---
>
> This patch made on top of memory leaks' fixes:
> * https://mail.openvswitch.org/pipermail/ovs-dev/2018-January/343537.html
> * https://mail.openvswitch.org/pipermail/ovs-dev/2018-January/343538.html
>
> lib/netdev-dpdk.c | 215
> --
> 1 file changed, 80 insertions(+), 135 deletions(-)
>
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index 50a94d1..e834c7e 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -437,10 +437,9 @@ struct netdev_dpdk {
>
> PADDED_MEMBERS(CACHE_LINE_SIZE,
> /* Names of all XSTATS counters */
> -struct rte_eth_xstat_name *rte_xstats_names;
> -int rte_xstats_names_size;
> -int rte_xstats_ids_size;
> -uint64_t *rte_xstats_ids;
> +struct rte_eth_xstat_name *xstats_names;
> +uint64_t *xstats_ids;
> +int xstats_count;
> );
> };
>
> @@ -901,6 +900,8 @@ common_construct(struct netdev *netdev, dpdk_port_t
> port_no,
> dev->vhost_reconfigured = false;
> dev->attached = false;
>
> +dev->xstats_count = 0;
> +
> ovsrcu_init(&dev->qos_conf, NULL);
>
> ovsrcu_init(&dev->ingress_policer, NULL);
> @@ -925,13 +926,6 @@ common_construct(struct netdev *netdev, dpdk_port_t
> port_no,
> ovs_list_push_back(&dpdk_list, &dev->list_node);
>
> netdev_request_reconfigure(netdev);
> -
> -dev->rte_xstats_names = NULL;
> -dev->rte_xstats_names_size = 0;
> -
> -dev->rte_xstats_ids = NULL;
> -dev->rte_xstats_ids_size = 0;
> -
> return 0;
> }
>
> @@ -1174,123 +1168,83 @@ netdev_dpdk_dealloc(struct netdev *netdev)
> static void
> netdev_dpdk_clear_xstats(struct netdev_dpdk *dev)
> {
> -/* If statistics are already allocated, we have to
> - * reconfigure, as port_id could have been changed. */
> -if (dev->rte_xstats_names) {
> -free(dev->rte_xstats_names);
> -dev->rte_xstats_names = NULL;
> -dev->rte_xstats_names_size = 0;
> -}
> -if (dev->rte_xstats_ids) {
> -free(dev->rte_xstats_ids);
> -dev->rte_xstats_ids = NULL;
> -dev->rte_xstats_ids_size = 0;
> -}
> -}
> -
> -static const char*
> -netdev_dpdk_get_xstat_name(struct netdev_dpdk *dev, uint64_t id)
> -{
> -if (id >= dev->rte_xstats_names_size) {
> -return "UNKNOWN";
> +if (dev->xstats_count) {
> +free(dev->xstats_ids);
> +free(dev->xstats_names);
> +dev->xstats_count = 0;
> }
> -return dev->rte_xstats_names[id].name;
> }
>
> static bool
> netdev_dpdk_configure_xstats(struct netdev_dpdk *dev)
> OVS_REQUIRES(dev->mutex)
> {
> -int rte_xstats_len;
> -bool ret;
> -struct rte_eth_xstat *rte_xstats;
> -uint64_t id;
> -int xstats_no;
> -const char *name;
> -
> -/* Retrieving all XSTATS names. If something will go wrong
> - * or amount of co
Re: [ovs-dev] [PATCH v2] ofproto-dpif-ipfix: Fix an issue in flow key part
Hi Daniel, Ben,
Looks good to me!
Br,
Michal.
> -Original Message-
> From: Daniel Benli Ye [mailto:dani...@vmware.com]
> Sent: Thursday, February 15, 2018 2:52 AM
> To: b...@ovn.com; wen...@vmware.com; Weglicki, MichalX
> ; d...@openvswitch.org
> Cc: Benli Ye
> Subject: [PATCH v2] ofproto-dpif-ipfix: Fix an issue in flow key part
>
> From: Benli Ye
>
> As struct ipfix_data_record_flow_key_iface didn't calculate
> its length in flow key part, it may cause problem when flow
> key part length is not enough. Use MAX_IF_LEN and MAX_IF_DESCR
> to pre-allocate memory for ipfix_data_record_flow_key_iface.
>
> v1 -> v2: Use strnlen() to limit the max length.
>
> Signed-off-by: Daniel Benli Ye
> ---
> ofproto/ofproto-dpif-ipfix.c | 24 +---
> 1 file changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
> index c7ddeb9..4d9fe78 100644
> --- a/ofproto/ofproto-dpif-ipfix.c
> +++ b/ofproto/ofproto-dpif-ipfix.c
> @@ -314,6 +314,7 @@ BUILD_ASSERT_DECL(sizeof(struct
> ipfix_data_record_flow_key_common) == 20);
> /* Part of data record flow key for interface information. Since some of the
> * elements have variable length, members of this structure should be
> appended
> * to the 'struct dp_packet' one by one. */
> +OVS_PACKED(
> struct ipfix_data_record_flow_key_iface {
> ovs_be32 if_index; /* (INGRESS | EGRESS)_INTERFACE */
> ovs_be32 if_type; /* (INGRESS | EGRESS)_INTERFACE_TYPE */
> @@ -321,7 +322,9 @@ struct ipfix_data_record_flow_key_iface {
> char *if_name;
> uint8_t if_descr_len; /* Variable length element: INTERFACE_DESCRIPTION
> */
> char *if_descr;
> -};
> +});
> +BUILD_ASSERT_DECL(sizeof(struct ipfix_data_record_flow_key_iface) ==
> + 10 + 2 * sizeof(char *));
>
> /* Part of data record flow key for VLAN entities. */
> OVS_PACKED(
> @@ -511,8 +514,21 @@ BUILD_ASSERT_DECL(sizeof(struct
> ipfix_data_record_aggregated_tcp) == 48);
> */
> #define MAX_TUNNEL_KEY_LEN 8
>
> +#define MAX_IF_NAME_LEN 64
> +#define MAX_IF_DESCR_LEN 128
> +
> +/*
> + * Calculate interface information length in flow key.
> + * This is used to calculate max flow key length.
> + */
> +#define FLOW_KEY_IFACE_LEN \
> +(sizeof(struct ipfix_data_record_flow_key_iface)\
> + - 2 * sizeof(char *) \
> + + MAX_IF_NAME_LEN + MAX_IF_DESCR_LEN)
> +
> #define MAX_FLOW_KEY_LEN\
> (sizeof(struct ipfix_data_record_flow_key_common) \
> + + FLOW_KEY_IFACE_LEN \
> + sizeof(struct ipfix_data_record_flow_key_vlan) \
> + sizeof(struct ipfix_data_record_flow_key_ip) \
> + MAX(sizeof(struct ipfix_data_record_flow_key_ipv4), \
> @@ -2030,9 +2046,11 @@ ipfix_get_iface_data_record(const struct dpif_ipfix
> *di, odp_port_t port_no,
>
> smap_destroy(&netdev_status);
> data->if_index = htonl(port->ifindex);
> -data->if_descr_len = data->if_descr ? strlen(data->if_descr) : 0;
> +data->if_descr_len = data->if_descr ? strnlen(data->if_descr,
> + MAX_IF_DESCR_LEN) : 0;
> data->if_name = nullable_xstrdup(netdev_get_name(port->ofport->netdev));
> -data->if_name_len = data->if_name ? strlen(data->if_name) : 0;
> +data->if_name_len = data->if_name ? strnlen(data->if_name,
> +MAX_IF_NAME_LEN) : 0;
>
> return 0;
> }
> --
> 2.7.4
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] netdev-dummy: Lock mutex when retrieving custom stats.
Hi Ben,
You are right, I totally forgot about this mutex.
- Michal.
> -Original Message-
> From: Ben Pfaff [mailto:b...@ovn.org]
> Sent: Thursday, January 11, 2018 12:47 AM
> To: d...@openvswitch.org
> Cc: Ben Pfaff ; Weglicki, MichalX
> Subject: [PATCH] netdev-dummy: Lock mutex when retrieving custom stats.
>
> Found by Clang.
>
> CC: Michal Weglicki
> Fixes: 971f4b394c6e ("netdev: Custom statistics.")
> Signed-off-by: Ben Pfaff
> ---
> Somehow I didn't notice this before I applied the previous patch. Oops,
> sorry.
>
> lib/netdev-dummy.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c
> index 57da19c48fcb..4246af3b9c86 100644
> --- a/lib/netdev-dummy.c
> +++ b/lib/netdev-dummy.c
> @@ -1239,12 +1239,14 @@ netdev_dummy_get_custom_stats(const struct netdev
> *netdev,
> (struct netdev_custom_counter *) xcalloc(C_STATS_SIZE,
> sizeof(struct netdev_custom_counter));
>
> +ovs_mutex_lock(&dev->mutex);
> for (i = 0 ; i < C_STATS_SIZE ; i++) {
> custom_stats->counters[i].value = dev->custom_stats[i].value;
> ovs_strlcpy(custom_stats->counters[i].name,
> dev->custom_stats[i].name,
> NETDEV_CUSTOM_STATS_NAME_SIZE);
> }
> +ovs_mutex_unlock(&dev->mutex);
>
> return 0;
> }
> --
> 2.10.2
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] netdev: Custom statistics.
> -Original Message- > From: Kevin Traynor [mailto:ktray...@redhat.com] > Sent: Monday, January 8, 2018 7:08 PM > To: Weglicki, MichalX ; d...@openvswitch.org > Subject: Re: [ovs-dev] [PATCH v2] netdev: Custom statistics. > > On 12/19/2017 03:00 PM, Kevin Traynor wrote: > > On 12/19/2017 02:35 PM, Weglicki, MichalX wrote: > >>> -Original Message- > >>> From: Kevin Traynor [mailto:ktray...@redhat.com] > >>> Sent: Tuesday, December 19, 2017 3:07 PM > >>> To: Weglicki, MichalX ; d...@openvswitch.org > >>> Subject: Re: [ovs-dev] [PATCH v2] netdev: Custom statistics. > >>> > >>> On 12/05/2017 02:55 PM, Michal Weglicki wrote: > >>>> - New get_custom_stats interface function is added to netdev. It > >>>> allows particular netdev implementation to expose custom > >>>> counters in dictionary format (counter name/counter value). > >>>> - New statistics are retrieved using experimenter code and > >>>> are printed as a result to ofctl dump-ports. > >>>> - New counters are available for OpenFlow 1.4+. > >>>> - New statistics are printed to output via ofctl only if those > >>>> are present in reply message. > >>>> - New statistics definition is added to include/openflow/intel-ext.h. > >>>> - Custom statistics are implemented only for dpdk-physical > >>>> port type. > >>>> - DPDK-physical implementation uses xstats to collect statistics. > >>>> Only dropped and error counters are exposed. > >>>> > >>> Hi Michal - why only dropped and error counters? why not just expose > >>> them all. For example, IIUC this would report management dropped packets > >>> but there would not be a stat for management rx/tx successful packets. > >>> > >>> Kevin. > >> Hi Kevin - those counters were of biggest value to us at the point of > >> making this > >> patch, sending all counters (where for IXGBE is about 150) will produce > >> some movement on the network. I think that biggest advantage of this > >> particular patch is that it introduces a mechanism to expose > >> any counters, counters list can be extended in the future > >> if necessary. However I'm not sure if sending all counters > >> is good idea, as there could be thousands of it in the future - in this > >> solution, we have some kind of control over the data size. > >> > > Ok thanks, that makes sense. I would like to suggest that *_management_* > > be added as part of this as I think it's only 2 additional stats and > > I've seen at least one user saying they needed this information. > > > > Hi Michal, this does not look to be in v3, do you think it should be > added as a separate patch? or they should not be reported? So sorry Kevin, I've simply forgot to add it, I've just sent v4 where those counters are included. > > thanks, > Kevin. > > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] netdev: Custom statistics.
Hi Ben, I've just sent V3 of the patch, but please find my comments inline. > -Original Message- > From: Ben Pfaff [mailto:b...@ovn.org] > Sent: Wednesday, December 20, 2017 12:42 AM > To: Weglicki, MichalX > Cc: d...@openvswitch.org > Subject: Re: [ovs-dev] [PATCH v2] netdev: Custom statistics. > > On Tue, Dec 05, 2017 at 02:55:20PM +, Michal Weglicki wrote: > > - New get_custom_stats interface function is added to netdev. It > > allows particular netdev implementation to expose custom > > counters in dictionary format (counter name/counter value). > > - New statistics are retrieved using experimenter code and > > are printed as a result to ofctl dump-ports. > > - New counters are available for OpenFlow 1.4+. > > - New statistics are printed to output via ofctl only if those > > are present in reply message. > > - New statistics definition is added to include/openflow/intel-ext.h. > > - Custom statistics are implemented only for dpdk-physical > > port type. > > - DPDK-physical implementation uses xstats to collect statistics. > > Only dropped and error counters are exposed. > > > > v1->v2: > > - Buffer overrun check in parse_intel_port_custom_property. > > - ofputil_append_ofp14_port_stats uses "postappend" instead > > of "reserve" during message creation. > > - NEWS update. > > - DPDK documentation update. > > - Compilation and sparse warnings corrections. > > > > Signed-off-by: Michal Weglicki > > Thank you for the updated patch. > > I still see some "sparse" warnings (there's an ovs-dev thread about > trouble with "sparse"--maybe you should join it): > > ../lib/ofp-util.c:8105:27: warning: incorrect type in assignment > (different base types) > ../lib/ofp-util.c:8105:27:expected unsigned long long [unsigned] > [usertype] counter_value > ../lib/ofp-util.c:8105:27:got restricted ovs_be64 > ../lib/ofp-util.c:8333:54: warning: incorrect type in argument 1 > (different base types) > ../lib/ofp-util.c:8333:54:expected restricted ovs_be64 [usertype] > > ../lib/ofp-util.c:8333:54:got unsigned long long [unsigned] > [addressable] [usertype] counter_value > It is corrected. > I don't think that the new ofproto_class function port_get_custom_stats > is needed. The generic ofproto code already knows that every port is > associated with a netdev. I think that append_port_stats() in ofproto.c > can just call netdev_get_custom_stats() directly rather than through > this additional level of indirection. It is corrected. > > I don't like the idea implied in the code in a few places that name[] in > struct netdev_custom_counter might not be null-terminated. I think that > we should ensure that it is always null terminated. Otherwise there is > a pitfall for carelessly written code. To be honest I'm not sure what I could do here, each time statistics are Requested from netdev, whole buffer is set to "0", so even it Particular netdev implementation would return string which Is not null-terminated, it will be as all other characters in counter name field, will be "\0". When message is decoded from open flow buffer, proper check is done, so this part of the code is safe. If there is anything else you would like me to do, just let me know. > > I would like to see some tests. The most obvious need is a new test for > ofp-print.at that exercises parsing and printing a stats message that > includes some of these custom stats. I've added custom counters to current test case. > > I have a few other minor suggestions. I'm appending them as an > incremental patch. Many of these are minor style points. I think that > most of them will be self-explanatory. Please let me know if they make > sense. [MW] Those changes are applied. > > I'll look forward to v3. (After I'm happy with this, I'll probably ask > Ian to review it to be added to his dpdk merge branch.) > > --8<--cut here-->8-- > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > index 07be4a21cd2c..3ccbec11f592 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -1205,7 +1205,6 @@ netdev_dpdk_configure_xstats(struct netdev_dpdk *dev) > int xstats_no; > const char *name; > > - > /* Retrieving all XSTATS names. If something will go wrong > * or amount of counters will be equal 0, rte_xstats_names > * buffer will be marked as NULL, and any further xstats > @@ -1216,7 +1215,6 @@ netdev_dpdk_configure_xstats(struct netdev_dpdk *dev) >
Re: [ovs-dev] [PATCH v2] netdev: Custom statistics.
> -Original Message- > From: Kevin Traynor [mailto:ktray...@redhat.com] > Sent: Tuesday, December 19, 2017 3:07 PM > To: Weglicki, MichalX ; d...@openvswitch.org > Subject: Re: [ovs-dev] [PATCH v2] netdev: Custom statistics. > > On 12/05/2017 02:55 PM, Michal Weglicki wrote: > > - New get_custom_stats interface function is added to netdev. It > > allows particular netdev implementation to expose custom > > counters in dictionary format (counter name/counter value). > > - New statistics are retrieved using experimenter code and > > are printed as a result to ofctl dump-ports. > > - New counters are available for OpenFlow 1.4+. > > - New statistics are printed to output via ofctl only if those > > are present in reply message. > > - New statistics definition is added to include/openflow/intel-ext.h. > > - Custom statistics are implemented only for dpdk-physical > > port type. > > - DPDK-physical implementation uses xstats to collect statistics. > > Only dropped and error counters are exposed. > > > > Hi Michal - why only dropped and error counters? why not just expose > them all. For example, IIUC this would report management dropped packets > but there would not be a stat for management rx/tx successful packets. > > Kevin. Hi Kevin - those counters were of biggest value to us at the point of making this patch, sending all counters (where for IXGBE is about 150) will produce some movement on the network. I think that biggest advantage of this particular patch is that it introduces a mechanism to expose any counters, counters list can be extended in the future if necessary. However I'm not sure if sending all counters is good idea, as there could be thousands of it in the future - in this solution, we have some kind of control over the data size. Michal. > > > v1->v2: > > - Buffer overrun check in parse_intel_port_custom_property. > > - ofputil_append_ofp14_port_stats uses "postappend" instead > > of "reserve" during message creation. > > - NEWS update. > > - DPDK documentation update. > > - Compilation and sparse warnings corrections. > > > > Signed-off-by: Michal Weglicki ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] netdev: Custom statistics.
Hi Ben, Did you have a chance to look at this patch re-work? Thank you in advance! Br, Michal. > -Original Message- > From: Weglicki, MichalX > Sent: Tuesday, December 5, 2017 3:55 PM > To: d...@openvswitch.org > Cc: Weglicki, MichalX > Subject: [PATCH v2] netdev: Custom statistics. > > - New get_custom_stats interface function is added to netdev. It > allows particular netdev implementation to expose custom > counters in dictionary format (counter name/counter value). > - New statistics are retrieved using experimenter code and > are printed as a result to ofctl dump-ports. > - New counters are available for OpenFlow 1.4+. > - New statistics are printed to output via ofctl only if those > are present in reply message. > - New statistics definition is added to include/openflow/intel-ext.h. > - Custom statistics are implemented only for dpdk-physical > port type. > - DPDK-physical implementation uses xstats to collect statistics. > Only dropped and error counters are exposed. > > v1->v2: > - Buffer overrun check in parse_intel_port_custom_property. > - ofputil_append_ofp14_port_stats uses "postappend" instead > of "reserve" during message creation. > - NEWS update. > - DPDK documentation update. > - Compilation and sparse warnings corrections. > > Signed-off-by: Michal Weglicki > --- > Documentation/howto/dpdk.rst | 15 ++-- > NEWS | 6 ++ > include/openflow/intel-ext.h | 28 ++ > include/openvswitch/netdev.h | 17 > include/openvswitch/ofp-util.h | 1 + > lib/netdev-dpdk.c | 195 > - > lib/netdev-dummy.c | 1 + > lib/netdev-linux.c | 1 + > lib/netdev-provider.h | 13 +++ > lib/netdev-vport.c | 1 + > lib/netdev.c | 27 ++ > lib/netdev.h | 2 + > lib/ofp-print.c| 18 > lib/ofp-util.c | 119 +++-- > lib/util.c | 13 +++ > lib/util.h | 2 + > ofproto/ofproto-dpif.c | 13 +++ > ofproto/ofproto-provider.h | 4 + > ofproto/ofproto.c | 21 + > ofproto/ofproto.h | 3 + > vswitchd/bridge.c | 24 + > 21 files changed, 512 insertions(+), 12 deletions(-) > > diff --git a/Documentation/howto/dpdk.rst b/Documentation/howto/dpdk.rst > index d123819..c99ec29 100644 > --- a/Documentation/howto/dpdk.rst > +++ b/Documentation/howto/dpdk.rst > @@ -311,12 +311,16 @@ performance of non-tunnel traffic, specifically for > smaller size packet. > > .. _extended-statistics: > > -Extended Statistics > > +Extended & Custom Statistics > + > > DPDK Extended Statistics API allows PMD to expose unique set of statistics. > The Extended statistics are implemented and supported only for DPDK physical > -and vHost ports. > +and vHost ports. Custom statistics are dynamic set of counters which can > +vary depenend on a driver. Those statistics are implemented > +for DPDK physical ports and contain all "dropped" and "error" > +counters from XSTATS. XSTATS counters list can be found here: > +<https://wiki.opnfv.org/display/fastpath/Collectd+Metrics+and+Events>`__. > > To enable statistics, you have to enable OpenFlow 1.4 support for OVS. > Configure bridge br0 to support OpenFlow version 1.4:: > @@ -333,8 +337,9 @@ Query the port statistics by explicitly specifying -O > OpenFlow14 option:: > > $ ovs-ofctl -O OpenFlow14 dump-ports br0 > > -Note: vHost ports supports only partial statistics. RX packet size based > -counter are only supported and doesn't include TX packet size counters. > +Note about "Extended Statistics": vHost ports supports only partial > +statistics. RX packet size based counter are only supported and > +doesn't include TX packet size counters. > > .. _port-hotplug: > > diff --git a/NEWS b/NEWS > index 427c8f8..727142c 100644 > --- a/NEWS > +++ b/NEWS > @@ -13,6 +13,12 @@ Post-v2.8.0 > * ovn-ctl: New commands run_nb_ovsdb and run_sb_ovsdb. > - Linux kernel 4.13 > * Add support for compiling OVS with the latest Linux 4.13 kernel > + - DPDK: > + * Custom statistics: > +- DPDK physical ports now return custom set of "dropped" and "error" > + statistics. > +- ovs-ofctl dump-ports command now prints new of set custom > statistics > + if available (for OpenFlow 1.4+). > > v2.8.0 - 31 Aug 2017 >
Re: [ovs-dev] [PATCH] netdev: Custom statistics.
> -Original Message- > From: Ben Pfaff [mailto:b...@ovn.org] > Sent: Wednesday, November 29, 2017 6:35 PM > To: Weglicki, MichalX > Cc: d...@openvswitch.org > Subject: Re: [ovs-dev] [PATCH] netdev: Custom statistics. > > On Tue, Nov 28, 2017 at 01:46:05PM +, Michal Weglicki wrote: > > - New get_custom_stats interface function is added to netdev. It > > allows particular netdev implementation to expose custom > > counters in dictionary format (counter name/counter value). > > - New statistics are retrieved using experimenter code and > > are printed as a result to ofctl dump-ports. > > - New counters are available for OpenFlow 1.4+. > > - New statistics are printed to output via ofctl only if those > > are present in reply message. > > - New statistics definition is added to include/openflow/intel-ext.h. > > - Custom statistics are implemented only for dpdk-physical > > port type. > > - DPDK-physical implementation uses xstats to collect statistics. > > Only dropped and error counters are exposed. > > > > Signed-off-by: Michal Weglicki > > This is pretty cool. I like it. > > There is one new compiler warning: > > ../lib/string.c:49:1: error: no previous prototype for function > 'string_ends_with' [-Werror,-Wmissing-prototypes] [MW] I was quite confused how this happened and now I see that string.h is on git ignore list. I was trying to find good spot for string helper function, and didn't find anything more suitable. If this is on ignore list on purpose could you point me to some kind of string helper file where I can include my function? > > and new "sparse" warnings: > > ../lib/ofp-util.c:8100:40: warning: incorrect type in assignment > (different base types) > ../lib/ofp-util.c:8100:40:expected restricted ovs_be16 [usertype] > stats_array_size > ../lib/ofp-util.c:8100:40:got unsigned int const [unsigned] > [usertype] size > ../lib/ofp-util.c:8288:28: warning: incorrect type in assignment > (different base types) > ../lib/ofp-util.c:8288:28:expected unsigned int [unsigned] [usertype] > size > ../lib/ofp-util.c:8288:28:got restricted ovs_be16 const [usertype] > stats_array_size > ../lib/ofp-util.c:8297:34: warning: restricted ovs_be16 degrades to > integer [MW] Sorry about that, will do. > > It is inconvenient, in ofputil_append_ofp14_port_stats(), to make the > custom stats code know the size of the stats to be appended before > actually appending them. I think that it would lead to nicer code if we > dropped netdev_get_custom_stats_size(), etc., and just used > ofpmp_postappend() instead of ofpmp_reserve(). The latter is slightly > more efficient but it requires knowing the size in advance. Before this > patch that was easy, after this patch it is not, so a change in strategy > makes sense. > > ofputil_append_ofp14_port_stats() and parse_intel_port_custom_property() > don't seem to be doing network byte order conversion for the counters. > > parse_intel_port_custom_property() doesn't seem to be careful not to > overrun the data buffer, or to make sure that the strings are of > reasonable lengths. > > Please add an item to NEWS. > > What's an appropriate place to document the new statistics? Is the set > of extended statistics fairly stable? If so, then maybe they could be > documented directly in OVS, otherwise I'd hope at least for a pointer to > wherever the real documentation is, from an appropriate place in the OVS > documentation. > > Thanks, > > Ben. Br, Michal. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v6 1/2] netdev-dpdk: extend netdev_dpdk_get_status to include if_type and if_descr
Hello Ben,
I've added missing missing documentation as suggested in your comment. If there
is something else you would like me to do, just let me know.
Br,
Michal.
> -Original Message-
> From: Weglicki, MichalX
> Sent: Tuesday, November 14, 2017 12:00 PM
> To: d...@openvswitch.org
> Cc: Weglicki, MichalX ; Przemyslaw Szczerbik
>
> Subject: [PATCH v6 1/2] netdev-dpdk: extend netdev_dpdk_get_status to include
> if_type and if_descr
>
> This commit extends netdev_dpdk_get_status API to include additional
> driver-related information: if_type and if_descr.
>
> v2->v3: Code rebase.
> v3->v4: Minor comments applied.
> v5->v6: Adds DPDK port specific description in documentation.
>
> Co-authored-by: Michal Weglicki
> Signed-off-by: Michal Weglicki
> Signed-off-by: Przemyslaw Szczerbik
> ---
> lib/netdev-dpdk.c| 9
> vswitchd/vswitch.xml | 64
>
> 2 files changed, 73 insertions(+)
>
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index 76e79be..3560bdd 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -36,6 +36,7 @@
> #include
> #include
> #include
> +#include
>
> #include "dirs.h"
> #include "dp-packet.h"
> @@ -2509,6 +2510,14 @@ netdev_dpdk_get_status(const struct netdev *netdev,
> struct smap *args)
> smap_add_format(args, "max_vfs", "%u", dev_info.max_vfs);
> smap_add_format(args, "max_vmdq_pools", "%u", dev_info.max_vmdq_pools);
>
> +/* Querying the DPDK library for iftype may be done in future, pending
> + * support; cf. RFC 3635 Section 3.2.4. */
> +enum { IF_TYPE_ETHERNETCSMACD = 6 };
> +
> +smap_add_format(args, "if_type", "%"PRIu32, IF_TYPE_ETHERNETCSMACD);
> +smap_add_format(args, "if_descr", "%s %s", rte_version(),
> + dev_info.driver_name);
> +
> if (dev_info.pci_dev) {
> smap_add_format(args, "pci-vendor_id", "0x%u",
> dev_info.pci_dev->id.vendor_id);
> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> index c145e1a..72cf353 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -2815,6 +2815,70 @@ ovs-vsctl add-port br0 p0 -- set Interface p0
> type=patch options:peer=p1 \
> Whether carrier is detected on key="tunnel_egress_iface"/>.
>
> +
> +
> +
> + DPDK specific interface status options.
> +
> +
> +
> +DPDK port ID.
> +
> +
> +
> +NUMA socket ID to which an Ethernet device is connected.
> +
> +
> +
> +Minimum size of RX buffer.
> +
> +
> +
> +Maximum configurable length of RX pkt.
> +
> +
> +
> +Maximum number of RX queues.
> +
> +
> +
> +Maximum number of TX queues.
> +
> +
> +
> +Maximum number of MAC addresses.
> +
> +
> +
> +Maximum number of hash MAC addresses for MTA and UTA.
> +
> +
> +
> +Maximum number of hash MAC addresses for MTA and UTA.
> +Maximum number of VFs.
> +
> +
> +
> +Maximum number of VMDq pools.
> +
> +
> +
> +Interface type ID according to IANA ifTYPE MIB definitions.
> +
> +
> +
> +Interface description string.
> +
> +
> +
> +Vendor ID of PCI device.
> +
> +
> +
> +Device ID of PCI device.
> +
> +
> +
>
>
>
> --
> 1.8.3.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v5 2/2] ofproto-dpif-ipfix: add interface Information Elements to flow key
Hello,
Can we proceed further with this patch if there are no more comments?
Thank you in advance.
Br,
Michal.
> -Original Message-
> From: Weglicki, MichalX
> Sent: Thursday, October 12, 2017 10:03 AM
> To: d...@openvswitch.org
> Cc: Greg Rose ; Weglicki, MichalX
>
> Subject: RE: [ovs-dev] [PATCH v5 2/2] ofproto-dpif-ipfix: add interface
> Information Elements to flow key
>
> Hello,
>
> Are there are any other comments?
>
> Thank you all in advance.
>
> Br,
> Michal.
>
> > -Original Message-
> > From: ovs-dev-boun...@openvswitch.org
> > [mailto:ovs-dev-boun...@openvswitch.org] On Behalf Of Weglicki, MichalX
> > Sent: Wednesday, October 4, 2017 10:19 AM
> > To: Greg Rose ; d...@openvswitch.org
> > Subject: Re: [ovs-dev] [PATCH v5 2/2] ofproto-dpif-ipfix: add interface
> > Information Elements to flow key
> >
> > > -Original Message-
> > > From: Greg Rose [mailto:gvrose8...@gmail.com]
> > > Sent: Wednesday, October 4, 2017 12:21 AM
> > > To: Weglicki, MichalX ; d...@openvswitch.org
> > > Subject: Re: [ovs-dev] [PATCH v5 2/2] ofproto-dpif-ipfix: add interface
> > > Information Elements to flow key
> > >
> > > On 10/02/2017 07:49 AM, Michal Weglicki wrote:
> > > > Extend flow key part of data record to include following Information
> > > > Elements:
> > > > - ingressInterface
> > > > - ingressInterfaceType
> > > > - egressInterface
> > > > - egressInterfaceType
> > > > - interfaceName
> > > > - interfaceDescription
> > > >
> > > > In case of input sampling we don't have information about egress port.
> > > > Define templates depending not only on protocol types, but also on flow
> > > > direction. Only egress flow will include egress information elements.
> > > >
> > > > With this change, dpif_ipfix_exporter stores every port in hmap rather
> > > > than only tunnel ports. It allows to easily retrieve required
> > > > information about interfaces during sampling upcalls.
> > > >
> > > > v1->v2
> > > > * Add interfaceType and interfaceDescription
> > > > * Rework ipfix_get_iface_data_record function
> > > > v2->v3: Code rebase.
> > > > v3->v4: Minor comments applied.
> > > > v4->v5: Clang complation problem fix.
> > > >
> > > > Co-authored-by: Michal Weglicki
> > > > Signed-off-by: Michal Weglicki
> > > > Signed-off-by: Przemyslaw Szczerbik
> > >
> > > Michal and Przemyslaw,
> > >
> > > There is one more small nit pick that I came across noted below.
> > >
> > > Other than that
> > >
> > > Tested-by: Greg Rose
> > > Reviewed-by: Greg Rose
> > >
> > > > ---
> > > > ofproto/ofproto-dpif-ipfix.c | 358
> > > > +++
> > > > ofproto/ofproto-dpif-ipfix.h | 6 +-
> > > > ofproto/ofproto-dpif-xlate.c | 4 +-
> > > > ofproto/ofproto-dpif.c | 19 +--
> > > > 4 files changed, 277 insertions(+), 110 deletions(-)
> > > >
> > > > diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
> > > > index 472c272..138c325 100644
> > > > --- a/ofproto/ofproto-dpif-ipfix.c
> > > > +++ b/ofproto/ofproto-dpif-ipfix.c
> > > > @@ -115,11 +115,12 @@ struct dpif_ipfix_global_stats {
> > > > };
> > > >
> > > > struct dpif_ipfix_port {
> > > > -struct hmap_node hmap_node; /* In struct dpif_ipfix's
> > > > "tunnel_ports" hmap. */
> > > > +struct hmap_node hmap_node; /* In struct dpif_ipfix's "ports"
> > > > hmap. */
> > > > struct ofport *ofport; /* To retrieve port stats. */
> > > > odp_port_t odp_port;
> > > > enum dpif_ipfix_tunnel_type tunnel_type;
> > > > uint8_t tunnel_key_length;
> > > > +uint32_t ifindex;
> > > > };
> > > >
> > > > struct dpif_ipfix_exporter {
> > > > @@ -157,9 +158,9 @@ struct dpif_ipfix_flow_exporter_map_node {
> > > > struct dpif_ipfix {
> > > > struct dpif_ipfix_bridge_exporter bridge_exporter;
> > > > struct hmap flow_exporter_map; /*
> > > > dpif_ipfix_flow_exporter_map_node. */
> > > > -stru
Re: [ovs-dev] [PATCH v5 2/2] ofproto-dpif-ipfix: add interface Information Elements to flow key
Hello,
Are there are any other comments?
Thank you all in advance.
Br,
Michal.
> -Original Message-
> From: ovs-dev-boun...@openvswitch.org
> [mailto:ovs-dev-boun...@openvswitch.org] On Behalf Of Weglicki, MichalX
> Sent: Wednesday, October 4, 2017 10:19 AM
> To: Greg Rose ; d...@openvswitch.org
> Subject: Re: [ovs-dev] [PATCH v5 2/2] ofproto-dpif-ipfix: add interface
> Information Elements to flow key
>
> > -Original Message-
> > From: Greg Rose [mailto:gvrose8...@gmail.com]
> > Sent: Wednesday, October 4, 2017 12:21 AM
> > To: Weglicki, MichalX ; d...@openvswitch.org
> > Subject: Re: [ovs-dev] [PATCH v5 2/2] ofproto-dpif-ipfix: add interface
> > Information Elements to flow key
> >
> > On 10/02/2017 07:49 AM, Michal Weglicki wrote:
> > > Extend flow key part of data record to include following Information
> > > Elements:
> > > - ingressInterface
> > > - ingressInterfaceType
> > > - egressInterface
> > > - egressInterfaceType
> > > - interfaceName
> > > - interfaceDescription
> > >
> > > In case of input sampling we don't have information about egress port.
> > > Define templates depending not only on protocol types, but also on flow
> > > direction. Only egress flow will include egress information elements.
> > >
> > > With this change, dpif_ipfix_exporter stores every port in hmap rather
> > > than only tunnel ports. It allows to easily retrieve required
> > > information about interfaces during sampling upcalls.
> > >
> > > v1->v2
> > > * Add interfaceType and interfaceDescription
> > > * Rework ipfix_get_iface_data_record function
> > > v2->v3: Code rebase.
> > > v3->v4: Minor comments applied.
> > > v4->v5: Clang complation problem fix.
> > >
> > > Co-authored-by: Michal Weglicki
> > > Signed-off-by: Michal Weglicki
> > > Signed-off-by: Przemyslaw Szczerbik
> >
> > Michal and Przemyslaw,
> >
> > There is one more small nit pick that I came across noted below.
> >
> > Other than that
> >
> > Tested-by: Greg Rose
> > Reviewed-by: Greg Rose
> >
> > > ---
> > > ofproto/ofproto-dpif-ipfix.c | 358
> > > +++
> > > ofproto/ofproto-dpif-ipfix.h | 6 +-
> > > ofproto/ofproto-dpif-xlate.c | 4 +-
> > > ofproto/ofproto-dpif.c | 19 +--
> > > 4 files changed, 277 insertions(+), 110 deletions(-)
> > >
> > > diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
> > > index 472c272..138c325 100644
> > > --- a/ofproto/ofproto-dpif-ipfix.c
> > > +++ b/ofproto/ofproto-dpif-ipfix.c
> > > @@ -115,11 +115,12 @@ struct dpif_ipfix_global_stats {
> > > };
> > >
> > > struct dpif_ipfix_port {
> > > -struct hmap_node hmap_node; /* In struct dpif_ipfix's "tunnel_ports"
> > > hmap. */
> > > +struct hmap_node hmap_node; /* In struct dpif_ipfix's "ports" hmap.
> > > */
> > > struct ofport *ofport; /* To retrieve port stats. */
> > > odp_port_t odp_port;
> > > enum dpif_ipfix_tunnel_type tunnel_type;
> > > uint8_t tunnel_key_length;
> > > +uint32_t ifindex;
> > > };
> > >
> > > struct dpif_ipfix_exporter {
> > > @@ -157,9 +158,9 @@ struct dpif_ipfix_flow_exporter_map_node {
> > > struct dpif_ipfix {
> > > struct dpif_ipfix_bridge_exporter bridge_exporter;
> > > struct hmap flow_exporter_map; /*
> > > dpif_ipfix_flow_exporter_map_node. */
> > > -struct hmap tunnel_ports; /* Contains "struct
> > > dpif_ipfix_port"s.
> > > - * It makes tunnel port lookups
> > > faster in
> > > - * sampling upcalls. */
> > > +struct hmap ports; /* Contains "struct
> > > dpif_ipfix_port"s.
> > > + * It makes port lookups faster in
> > > sampling
> > > + * upcalls. */
> > > struct ovs_refcount ref_cnt;
> > > };
> > >
> > > @@ -293,7 +294,8 @@ BUILD_ASSERT_DECL(sizeof(struct
> > > ipfix_template_field_specifier) == 8);
> > > /* Cf. IETF RFC 5102 Section 5.11.6. */
> > > enum ipfix_flow_direction {
> > &g
Re: [ovs-dev] [PATCH v5 2/2] ofproto-dpif-ipfix: add interface Information Elements to flow key
> -Original Message-
> From: Greg Rose [mailto:gvrose8...@gmail.com]
> Sent: Wednesday, October 4, 2017 12:21 AM
> To: Weglicki, MichalX ; d...@openvswitch.org
> Subject: Re: [ovs-dev] [PATCH v5 2/2] ofproto-dpif-ipfix: add interface
> Information Elements to flow key
>
> On 10/02/2017 07:49 AM, Michal Weglicki wrote:
> > Extend flow key part of data record to include following Information
> > Elements:
> > - ingressInterface
> > - ingressInterfaceType
> > - egressInterface
> > - egressInterfaceType
> > - interfaceName
> > - interfaceDescription
> >
> > In case of input sampling we don't have information about egress port.
> > Define templates depending not only on protocol types, but also on flow
> > direction. Only egress flow will include egress information elements.
> >
> > With this change, dpif_ipfix_exporter stores every port in hmap rather
> > than only tunnel ports. It allows to easily retrieve required
> > information about interfaces during sampling upcalls.
> >
> > v1->v2
> > * Add interfaceType and interfaceDescription
> > * Rework ipfix_get_iface_data_record function
> > v2->v3: Code rebase.
> > v3->v4: Minor comments applied.
> > v4->v5: Clang complation problem fix.
> >
> > Co-authored-by: Michal Weglicki
> > Signed-off-by: Michal Weglicki
> > Signed-off-by: Przemyslaw Szczerbik
>
> Michal and Przemyslaw,
>
> There is one more small nit pick that I came across noted below.
>
> Other than that
>
> Tested-by: Greg Rose
> Reviewed-by: Greg Rose
>
> > ---
> > ofproto/ofproto-dpif-ipfix.c | 358
> > +++
> > ofproto/ofproto-dpif-ipfix.h | 6 +-
> > ofproto/ofproto-dpif-xlate.c | 4 +-
> > ofproto/ofproto-dpif.c | 19 +--
> > 4 files changed, 277 insertions(+), 110 deletions(-)
> >
> > diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
> > index 472c272..138c325 100644
> > --- a/ofproto/ofproto-dpif-ipfix.c
> > +++ b/ofproto/ofproto-dpif-ipfix.c
> > @@ -115,11 +115,12 @@ struct dpif_ipfix_global_stats {
> > };
> >
> > struct dpif_ipfix_port {
> > -struct hmap_node hmap_node; /* In struct dpif_ipfix's "tunnel_ports"
> > hmap. */
> > +struct hmap_node hmap_node; /* In struct dpif_ipfix's "ports" hmap. */
> > struct ofport *ofport; /* To retrieve port stats. */
> > odp_port_t odp_port;
> > enum dpif_ipfix_tunnel_type tunnel_type;
> > uint8_t tunnel_key_length;
> > +uint32_t ifindex;
> > };
> >
> > struct dpif_ipfix_exporter {
> > @@ -157,9 +158,9 @@ struct dpif_ipfix_flow_exporter_map_node {
> > struct dpif_ipfix {
> > struct dpif_ipfix_bridge_exporter bridge_exporter;
> > struct hmap flow_exporter_map; /* dpif_ipfix_flow_exporter_map_node.
> > */
> > -struct hmap tunnel_ports; /* Contains "struct dpif_ipfix_port"s.
> > - * It makes tunnel port lookups faster
> > in
> > - * sampling upcalls. */
> > +struct hmap ports; /* Contains "struct dpif_ipfix_port"s.
> > + * It makes port lookups faster in
> > sampling
> > + * upcalls. */
> > struct ovs_refcount ref_cnt;
> > };
> >
> > @@ -293,7 +294,8 @@ BUILD_ASSERT_DECL(sizeof(struct
> > ipfix_template_field_specifier) == 8);
> > /* Cf. IETF RFC 5102 Section 5.11.6. */
> > enum ipfix_flow_direction {
> > INGRESS_FLOW = 0x00,
> > -EGRESS_FLOW = 0x01
> > +EGRESS_FLOW = 0x01,
> > +NUM_IPFIX_FLOW_DIRECTION
> > };
> >
> > /* Part of data record flow key for common metadata and Ethernet
> > entities. */
> > @@ -308,6 +310,18 @@ struct ipfix_data_record_flow_key_common {
> > });
> > BUILD_ASSERT_DECL(sizeof(struct ipfix_data_record_flow_key_common) == 20);
> >
> > +/* Part of data record flow key for interface information. Since some of
> > the
> > + * elements have variable length, members of this structure should be
> > appended
> > + * to the 'struct dp_packet' one by one. */
> > +struct ipfix_data_record_flow_key_iface {
> > +ovs_be32 if_index; /* (INGRESS | EGRESS)_INTERFACE */
> > +ovs_be32 if_type; /* (INGRESS | EGRESS)_INTERFACE_TYPE */
> > +uint8_t if_name_len; /* V
Re: [ovs-dev] [PATCH v4 2/2] ofproto-dpif-ipfix: add interface Information Elements to flow key
Greg,
Seems that Przemek used C statement which is a compiler extension and it is not
standardized - thus compatibility problems with clang. I will apply V5 patch
shortly.
Br,
Michal.
> -Original Message-
> From: Greg Rose [mailto:gvrose8...@gmail.com]
> Sent: Friday, September 29, 2017 12:32 AM
> To: Weglicki, MichalX ; d...@openvswitch.org
> Subject: Re: [ovs-dev] [PATCH v4 2/2] ofproto-dpif-ipfix: add interface
> Information Elements to flow key
>
> On 09/26/2017 04:53 AM, Michal Weglicki wrote:
> > Extend flow key part of data record to include following Information
> > Elements:
> > - ingressInterface
> > - ingressInterfaceType
> > - egressInterface
> > - egressInterfaceType
> > - interfaceName
> > - interfaceDescription
>
> Michal,
>
> I'm testing this patch and so far it looks good testing wise but there is a
> clang
> error. Can you look into it? I noted it below.
>
> Here's a link to the travis build with the clang errors:
>
> https://travis-ci.org/gvrose8192/ovs-experimental
>
> Thanks,
>
> - Greg
>
> >
> > In case of input sampling we don't have information about egress port.
> > Define templates depending not only on protocol types, but also on flow
> > direction. Only egress flow will include egress information elements.
> >
> > With this change, dpif_ipfix_exporter stores every port in hmap rather
> > than only tunnel ports. It allows to easily retrieve required
> > information about interfaces during sampling upcalls.
> >
> > v1->v2
> > * Add interfaceType and interfaceDescription
> > * Rework ipfix_get_iface_data_record function
> > v2->v3: Code rebase.
> > v3->v4: Minor comments applied.
> >
> > Co-authored-by: Michal Weglicki
> > Signed-off-by: Michal Weglicki
> > Signed-off-by: Przemyslaw Szczerbik
> > ---
> > ofproto/ofproto-dpif-ipfix.c | 358
> > +++
> > ofproto/ofproto-dpif-ipfix.h | 6 +-
> > ofproto/ofproto-dpif-xlate.c | 4 +-
> > ofproto/ofproto-dpif.c | 19 +--
> > 4 files changed, 277 insertions(+), 110 deletions(-)
> >
> > diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
> > index 472c272..ab9192b 100644
> > --- a/ofproto/ofproto-dpif-ipfix.c
> > +++ b/ofproto/ofproto-dpif-ipfix.c
> > @@ -115,11 +115,12 @@ struct dpif_ipfix_global_stats {
> > };
> >
> > struct dpif_ipfix_port {
> > -struct hmap_node hmap_node; /* In struct dpif_ipfix's "tunnel_ports"
> > hmap. */
> > +struct hmap_node hmap_node; /* In struct dpif_ipfix's "ports" hmap. */
> > struct ofport *ofport; /* To retrieve port stats. */
> > odp_port_t odp_port;
> > enum dpif_ipfix_tunnel_type tunnel_type;
> > uint8_t tunnel_key_length;
> > +uint32_t ifindex;
> > };
> >
> > struct dpif_ipfix_exporter {
> > @@ -157,9 +158,9 @@ struct dpif_ipfix_flow_exporter_map_node {
> > struct dpif_ipfix {
> > struct dpif_ipfix_bridge_exporter bridge_exporter;
> > struct hmap flow_exporter_map; /* dpif_ipfix_flow_exporter_map_node.
> > */
> > -struct hmap tunnel_ports; /* Contains "struct dpif_ipfix_port"s.
> > - * It makes tunnel port lookups faster
> > in
> > - * sampling upcalls. */
> > +struct hmap ports; /* Contains "struct dpif_ipfix_port"s.
> > + * It makes port lookups faster in
> > sampling
> > + * upcalls. */
> > struct ovs_refcount ref_cnt;
> > };
> >
> > @@ -293,7 +294,8 @@ BUILD_ASSERT_DECL(sizeof(struct
> > ipfix_template_field_specifier) == 8);
> > /* Cf. IETF RFC 5102 Section 5.11.6. */
> > enum ipfix_flow_direction {
> > INGRESS_FLOW = 0x00,
> > -EGRESS_FLOW = 0x01
> > +EGRESS_FLOW = 0x01,
> > +NUM_IPFIX_FLOW_DIRECTION
> > };
> >
> > /* Part of data record flow key for common metadata and Ethernet
> > entities. */
> > @@ -308,6 +310,18 @@ struct ipfix_data_record_flow_key_common {
> > });
> > BUILD_ASSERT_DECL(sizeof(struct ipfix_data_record_flow_key_common) == 20);
> >
> > +/* Part of data record flow key for interface information. Since some of
> > the
> > + * elements have variable length, members of this structure should be
> > appended
> > + * to the &#
Re: [ovs-dev] [PATCH 2/2] ofproto-dpif-ipfix: add interface Information Elements to flow key
Hi Greg - comments inline marked [MW]. > -Original Message- > From: Greg Rose [mailto:gvrose8...@gmail.com] > Sent: Saturday, September 16, 2017 12:45 AM > To: Weglicki, MichalX > Cc: d...@openvswitch.org; Darrell Ball > Subject: Re: [ovs-dev] [PATCH 2/2] ofproto-dpif-ipfix: add interface > Information Elements to flow key > > On 09/08/2017 06:35 AM, Weglicki, MichalX wrote: > > Greg, > > > > Patch is rebased and sent to mailing list as V3 (Last patch was supposed to > > be V2 - Przemek by accident sent it again as V1). > > > > Br, > > Michal. > > Michal, > > I'm not sure what has happened but I can't find your V3 patches in my mail > but they are in patchwork. > > I tested the patches and they seem to work fine, or at least the code > executes and I didn't see any serious > regression. My ntopng/nprobe setup accepted the new template. > > However, I am seeing this now: > > 5/Sep/2017 15:11:16 [Lua.cpp:74] ERROR: ntop_get_interface_host_info : > expected string, got nil > 15/Sep/2017 15:11:16 [Lua.cpp:6658] WARNING: Script failure > [/usr/share/ntopng/scripts/lua/iface_ndpi_stats.lua][/usr/share/ntopng/scripts/lua/iface_ndpi_stats.lua:31: > attempt to index > global > 'stats' (a nil value)] > 15/Sep/2017 15:11:16 [Lua.cpp:74] ERROR: ntop_get_interface_host_info : > expected string, got nil > 15/Sep/2017 15:11:15 [Lua.cpp:6658] WARNING: Script failure > [/usr/share/ntopng/scripts/lua/iface_ndpi_stats.lua][/usr/share/ntopng/scripts/lua/iface_ndpi_stats.lua:31: > attempt to index > global > 'stats' (a nil value)] > 15/Sep/2017 15:11:15 [Lua.cpp:74] ERROR: ntop_get_interface_host_info : > expected string, got nil > 15/Sep/2017 15:11:12 [Lua.cpp:6658] WARNING: Script failure > [/usr/share/ntopng/scripts/lua/iface_ndpi_stats.lua][/usr/share/ntopng/scripts/lua/iface_ndpi_stats.lua:31: > attempt to index > global > 'stats' (a nil value)] > 15/Sep/2017 15:11:12 [Lua.cpp:74] ERROR: ntop_get_interface_host_info : > expected string, got nil > 15/Sep/2017 15:11:09 [Lua.cpp:6658] WARNING: Script failure > [/usr/share/ntopng/scripts/lua/iface_ndpi_stats.lua][/usr/share/ntopng/scripts/lua/iface_ndpi_stats.lua:31: > attempt to index > global > 'stats' (a nil value)] > 15/Sep/2017 15:11:09 [Lua.cpp:74] ERROR: ntop_get_interface_host_info : > expected string, got nil > 15/Sep/2017 15:11:09 [Lua.cpp:6658] WARNING: Script failure > [/usr/share/ntopng/scripts/lua/iface_ndpi_stats.lua][/usr/share/ntopng/scripts/lua/iface_ndpi_stats.lua:31: > attempt to index > global > 'stats' (a nil value)] > 15/Sep/2017 15:11:09 [Lua.cpp:74] ERROR: ntop_get_interface_host_info : > expected string, got nil [MW] Well, are you sure that this patch is causing this error message? I mean, you don't see It without this patch, do you? If yes, could you explain your setup a little bit so I can reproduce it here? > > Also, in patch 1/2 though the interface is hard coded to Ethernet in this bit > of the patch: > > +/* Once DPDK library supports retrieving ifType we should get this value > + * directly from DPDK rather than hardcoding it. */ > +smap_add_format(args, "if_type", "%"PRIu32, IF_TYPE_ETHERNETCSMACD); > +smap_add_format(args, "if_descr", "%s %s", rte_version(), > + dev_info.driver_name); > > I'd like to get Darrel Ball's take on this so I've CC'd him. > > In patch 2/2 I have some other comments. [MW] There are some DPDK patches which expose this information for the driver, nevertheless I think that anyway value "6" Will be returned anyway - as this is very general category for Ethernet devices. I could be improved in the future of course When patch will be available. > > Here you change the ports to point to all ports rather than tunnel ports > > -struct hmap_node hmap_node; /* In struct dpif_ipfix's "tunnel_ports" > hmap. */ > +struct hmap_node hmap_node; /* In struct dpif_ipfix's "ports" hmap. */ > struct ofport *ofport; /* To retrieve port stats. */ > odp_port_t odp_port; > enum dpif_ipfix_tunnel_type tunnel_type; > uint8_t tunnel_key_length; > +uint32_t ifindex; > > I didn't see any reason this can't be done but I'm worried about side > effects. Are we sure that > there are no other assumptions in the code that depend on that hmap pointing > to only tunnel ports? > I realize that patch 2/2 seems to fix that up and I didn't see any particular > reason to > doubt but I
Re: [ovs-dev] [PATCH 2/2] ofproto-dpif-ipfix: add interface Information Elements to flow key
Greg, Patch is rebased and sent to mailing list as V3 (Last patch was supposed to be V2 - Przemek by accident sent it again as V1). Br, Michal. > -Original Message- > From: Greg Rose [mailto:gvrose8...@gmail.com] > Sent: Thursday, September 7, 2017 12:02 AM > To: Weglicki, MichalX > Cc: d...@openvswitch.org; Szczerbik, PrzemyslawX > > Subject: Re: [ovs-dev] [PATCH 2/2] ofproto-dpif-ipfix: add interface > Information Elements to flow key > > On 09/06/2017 02:53 AM, Weglicki, MichalX wrote: > > Hey Greg, > > > > Do you have any schedule for checking this patch? > > > > Thank you in advance! > > > > Br, > > Michal. > > Michal, > > The good news is that I have my ntopng/nprobe ipfix collector properly > configured now and can see > flows, hosts, etc. when I enable IPFIX. So I'm ready to test these two > patches from Przemyslaw: > > https://patchwork.ozlabs.org/patch/793773/ > https://patchwork.ozlabs.org/patch/793772/ > > Unfortunately the patches no longer apply to master. > > Can you rebase and resubmit a V3 series? > > Thanks, > > - Greg > > > > >> -Original Message- > >> From: Greg Rose [mailto:gvrose8...@gmail.com] > >> Sent: Tuesday, August 29, 2017 5:15 PM > >> To: Weglicki, MichalX > >> Cc: d...@openvswitch.org; Szczerbik, PrzemyslawX > >> > >> Subject: Re: [ovs-dev] [PATCH 2/2] ofproto-dpif-ipfix: add interface > >> Information Elements to flow key > >> > >> On 08/29/2017 04:48 AM, Weglicki, MichalX wrote: > >>> Hello Greg, > >>> > >>> Unfortunately Przemek is not available, but I'm more or less familiar > >>> with this patch. > >>> > >>> Did you manage to fix this issue? I think that your problems lies on > >>> connection level, which wasn't modified by this patch. Patch > >> added additional counters on reporting level, and based on your > >> description, ovs just can't connect to collector. > >>> > >>> Br, > >>> Michal. > >> > >> Correct - it is not an issue with the patch. However, I need to test the > >> patch and ran into problems while trying to do so. Since > then > >> I've been pulled into some other work and will get back to this > >> afterwards. I'll just need to debug the connection issue to the > >> collector. > >> > >> Thanks, > >> > >> - Greg > >> > >>> > >>>> -Original Message- > >>>> From: ovs-dev-boun...@openvswitch.org > >>>> [mailto:ovs-dev-boun...@openvswitch.org] On Behalf Of Greg Rose > >>>> Sent: Saturday, August 19, 2017 12:51 AM > >>>> To: Szczerbik, PrzemyslawX > >>>> Cc: d...@openvswitch.org > >>>> Subject: Re: [ovs-dev] [PATCH 2/2] ofproto-dpif-ipfix: add interface > >>>> Information Elements to flow key > >>>> > >>>> On 08/16/2017 01:54 AM, Szczerbik, PrzemyslawX wrote: > >>>>> Hi, > >>>>> > >>>>> I haven't received any feedback on this patch for quite some time. > >>>>> Is there anything that I can do to expedite review process? > >>>>> > >>>>> Regards, > >>>>> Przemek > >>>>> > >>>> > >>>> Przemek, > >>>> > >>>> I'm in the process of looking into this patch but I'm running into an > >>>> issue with the vswitch not sending flows to the > >> ntopng/nprobe > >>>> collector I have set up. I see this in the vswitchd log: > >>>> > >>>> 2017-08-18T21:48:57.917Z|00058|collectors|WARN|couldn't open connection > >>>> to collector 192.168.0.21:2055 (Network is > >>>> unreachable) > >>>> 2017-08-18T21:48:57.917Z|00059|ipfix|WARN|no collectors could be > >>>> initialized, IPFIX exporter disabled > >>>> > >>>> However, 192.168.0.21 is reachable, at least from the br0 bridge that > >>>> has the IPFIX flows enabled. > >>>> > >>>> ntopng/nprobe on the collector machine has the right ports open: > >>>> > >>>> netstat -tulpen | grep 2055 > >>>> udp0 0 0.0.0.0:20550.0.0.0:* > >>>>99 27671 3038/nprobe > >>>> udp6
Re: [ovs-dev] [PATCH 2/2] ofproto-dpif-ipfix: add interface Information Elements to flow key
Hey Greg, Do you have any schedule for checking this patch? Thank you in advance! Br, Michal. > -Original Message- > From: Greg Rose [mailto:gvrose8...@gmail.com] > Sent: Tuesday, August 29, 2017 5:15 PM > To: Weglicki, MichalX > Cc: d...@openvswitch.org; Szczerbik, PrzemyslawX > > Subject: Re: [ovs-dev] [PATCH 2/2] ofproto-dpif-ipfix: add interface > Information Elements to flow key > > On 08/29/2017 04:48 AM, Weglicki, MichalX wrote: > > Hello Greg, > > > > Unfortunately Przemek is not available, but I'm more or less familiar with > > this patch. > > > > Did you manage to fix this issue? I think that your problems lies on > > connection level, which wasn't modified by this patch. Patch > added additional counters on reporting level, and based on your description, > ovs just can't connect to collector. > > > > Br, > > Michal. > > Correct - it is not an issue with the patch. However, I need to test the > patch and ran into problems while trying to do so. Since then > I've been pulled into some other work and will get back to this afterwards. > I'll just need to debug the connection issue to the > collector. > > Thanks, > > - Greg > > > > > > -Original Message- > > > From: ovs-dev-boun...@openvswitch.org > > > [mailto:ovs-dev-boun...@openvswitch.org] On Behalf Of Greg Rose > > > Sent: Saturday, August 19, 2017 12:51 AM > > > To: Szczerbik, PrzemyslawX > > > Cc: d...@openvswitch.org > > > Subject: Re: [ovs-dev] [PATCH 2/2] ofproto-dpif-ipfix: add interface > > > Information Elements to flow key > > > > > > On 08/16/2017 01:54 AM, Szczerbik, PrzemyslawX wrote: > > >> Hi, > > >> > > >> I haven't received any feedback on this patch for quite some time. > > >> Is there anything that I can do to expedite review process? > > >> > > >> Regards, > > >> Przemek > > >> > > > > > > Przemek, > > > > > > I'm in the process of looking into this patch but I'm running into an > > > issue with the vswitch not sending flows to the > ntopng/nprobe > > > collector I have set up. I see this in the vswitchd log: > > > > > > 2017-08-18T21:48:57.917Z|00058|collectors|WARN|couldn't open connection > > > to collector 192.168.0.21:2055 (Network is > > > unreachable) > > > 2017-08-18T21:48:57.917Z|00059|ipfix|WARN|no collectors could be > > > initialized, IPFIX exporter disabled > > > > > > However, 192.168.0.21 is reachable, at least from the br0 bridge that has > > > the IPFIX flows enabled. > > > > > > ntopng/nprobe on the collector machine has the right ports open: > > > > > > netstat -tulpen | grep 2055 > > > udp0 0 0.0.0.0:20550.0.0.0:* > > > 99 27671 3038/nprobe > > > udp6 0 0 :::2055 :::* > > > 99 27672 3038/nprobe > > > > > > netstat -tulpen | grep 5556 > > > tcp0 0 0.0.0.0:55560.0.0.0:* > > > LISTEN 0 27666 3038/nprobe > > > > > > I'm not sure exactly what the problem is but I'm debugging that. Until I > > > can get past this issue I can't really test and review your > > > patch. I am actively working on getting this issue fixed though. > > > > > > Regards, > > > > > > - Greg > > >>> -Original Message- > > >>> From: ovs-dev-boun...@openvswitch.org [mailto:ovs-dev- > > >>> boun...@openvswitch.org] On Behalf Of Szczerbik, PrzemyslawX > > >>> Sent: Wednesday, July 26, 2017 12:01 PM > > >>> To: d...@openvswitch.org > > >>> Subject: Re: [ovs-dev] [PATCH 2/2] ofproto-dpif-ipfix: add interface > > >>> Information > > >>> Elements to flow key > > >>> > > >>> Hi, > > >>> > > >>> This patch was supposed to be v2, but I forgot to mention that in the > > >>> subject. > > >>> Previous version: https://patchwork.ozlabs.org/patch/792730/ > > >>> > > >>> Let me know if you want me to re-sent it with a proper version. > > >>> > > >>> Regards, > > >>> Przemek > > >>> > > >>>> -Origina
Re: [ovs-dev] [RFC PATCH 1/2] sflow: introduce egress flow sampling
Hello Neil, NAT can be configured as range of addresses, so it is impossible to Get this information before actual translation happens. In general I don't see any problem with creating egress action as it works in ipfix as default configuration along ingress action. When it comes to caching packet, I don't know yet exactly how it should implemented, but as long as this is optional sFLOW feature which would enable only during specific NAT case, I don't really see big impact as well. Also it gives us possible improvements in the future, as there could be other cases where some of the data fields can't be read on ingress. As you confirmed that it would work according to sFLOW specs, I really think it is worth it. Br, Michal. > -Original Message- > From: Neil McKee [mailto:neil.mc...@inmon.com] > Sent: Thursday, August 31, 2017 8:10 PM > To: Weglicki, MichalX > Cc: d...@openvswitch.org > Subject: Re: [ovs-dev] [RFC PATCH 1/2] sflow: introduce egress flow sampling > > Yes. Any solution that samples the original packet and annotates it > with accurate information about its forwarding will conform to the > spec. But anything you do that touches the chain of actions in more > than one place is likely to be problematic... > > For example, one possible approach that OVS *might* have taken from > the start was to (1) mark the packet for sampling at ingress (2) > accumulate forwarding information in a buffer as it goes through each > action, then (3) send an upcall with original-packet+forwarding-info > at the point when all the actions were completed. At first glance > this seems clean because there is only one upcall and no awkward > rendez-vous, but actually it is making assumptions that the datapath > is happy to buffer information at any stage, and that it will process > each packet to completion. Suppose there were a datapath > implementation (in hardware or software) where it made sense to > pipeline the datapath with batches of packets? It's not hard to > imagine scenarios where those assumptions would turn into serious > constraints. > > I completely agree with you that the NAT detail is a high-value > measurement, but the radical simplicity of the current implementation > is important, so making changes to the datapath should be weighed > against that. One question is: would it help if the sFlow feed > included at least the outline of the NAT translation that is known > from the actions-list? Or is there some other way to look up the NAT > translation table from user-space? Perhaps similar to the way the > host-sflow agent annotates samples with TCP performance metrics here: > https://github.com/sflow/host-sflow/blob/v2.0.11/src/Linux/mod_tcp.c#L512-L631 > > Neil > > -- > Neil McKee > InMon Corp. > http://www.inmon.com > > > On Thu, Aug 31, 2017 at 5:38 AM, Weglicki, MichalX > wrote: > > Hello Neil, > > > > The problem is that to fill NAT translation correctly through > > extended_nat we need sample packet before and after the translation. > > I understand that such information (possibly) could be analyzed by > > collector based on information from two switches, however I think that > > correctly getting this information at one point is beneficial. > > > > The approach which Przemek took is similar to ipfix, where default > > configuration per bridge is to have packet sampled on ingress and egress. > > This allows to support all the middlebox functions (e.g. NAT) that ipfix > > defines. I think it should also be supported in OVS-sFLOW. > > I wasn't aware that same packet can't be sampled on ingreess > > and egress, according to specification it can have to be sampled on > > igress OR egress - I didn't find any clear statement forbidding it, > > but I'm sure it is there, as you said. > > > > What if I would go into some hybrid approach, when all sampling > > would happen on ingress, but there would be optional egress action > > which fills additional counters (like extended_nat) and sent it when needed. > > So the logic would be: > > - When ingress action is executed, all counters are calculated, if any > > of the counters can't be retrieved at this point, packet is buffered, and > > marked for egress sampling with additional information what information > > is missing. > > - Then on egress when action is executed, requested function is > > calculated, filled, and then sent to collector. > > - In all other cases, egress action will just ignore packet, as it would be > > already sampled on ingress action. > > > > Do you think such approach would work? > > > > Br, > &
Re: [ovs-dev] [RFC PATCH 1/2] sflow: introduce egress flow sampling
Hello Neil, The problem is that to fill NAT translation correctly through extended_nat we need sample packet before and after the translation. I understand that such information (possibly) could be analyzed by collector based on information from two switches, however I think that correctly getting this information at one point is beneficial. The approach which Przemek took is similar to ipfix, where default configuration per bridge is to have packet sampled on ingress and egress. This allows to support all the middlebox functions (e.g. NAT) that ipfix defines. I think it should also be supported in OVS-sFLOW. I wasn't aware that same packet can't be sampled on ingreess and egress, according to specification it can have to be sampled on igress OR egress - I didn't find any clear statement forbidding it, but I'm sure it is there, as you said. What if I would go into some hybrid approach, when all sampling would happen on ingress, but there would be optional egress action which fills additional counters (like extended_nat) and sent it when needed. So the logic would be: - When ingress action is executed, all counters are calculated, if any of the counters can't be retrieved at this point, packet is buffered, and marked for egress sampling with additional information what information is missing. - Then on egress when action is executed, requested function is calculated, filled, and then sent to collector. - In all other cases, egress action will just ignore packet, as it would be already sampled on ingress action. Do you think such approach would work? Br, Michal. > -Original Message- > From: Neil McKee [mailto:neil.mc...@inmon.com] > Sent: Monday, August 28, 2017 9:47 PM > To: Weglicki, MichalX > Cc: d...@openvswitch.org > Subject: Re: [ovs-dev] [RFC PATCH 1/2] sflow: introduce egress flow sampling > > If you add an egress sampling action like this it ends up violating > the sFlow spec. To understand why, first see this thread back in > 2013 when we switched from per-interface datasources to per-bridge > datasources: > https://mail.openvswitch.org/pipermail/ovs-dev/2013-April/270575.html > > Having the packet-sampling datasource be the bridge was very helpful. > It meant we didn't have to add a new datasource for every new input > port, and didn't have to worry about input ports that had no ifIndex. > However a single datasource cannot sample the same "Packet Flow" twice > (see sFlow spec) so the same bridge datasource cannot sample on both > ingress and egress. > > If we took the radical step of going back to per-interface > datasources, then it would be allowable, but that is not tempting. > For one thing we would have to make sure that the sampled packet > header was fully decided and knowable at that point. If, for > example, the MAC source address of the egress packet is not yet > determined then inserting a made-up layer-2 header would be > problematic (this is actually a known problem with existing > egress-sampling implementations -- when packet samples are reported > that never actually existed in the network it can pollute the analysis > of address-mappings, host-location, traffic matrix de-duplication and > more). > > A guiding principle of sFlow is that the agent should be radically > simple and implemented everywhere. The NAT addressing should be > visible at the next downstream switch port. I concede that it can be > very helpful when the NAT info is included by the device doing the > translation, but if you examine the packet samples from each > observation point you should still be able to associate the internal > and external flows at the sFlow collector. Does that work for your > use-case? > > Neil > > > -- > Neil McKee > InMon Corp. > http://www.inmon.com > > > On Thu, Aug 24, 2017 at 5:33 AM, Michal Weglicki > wrote: > > Currently, sFlow implementation in Open vSwitch samples packets only on > > igress. > > While it works great for majority of cases, it can be troublesome to expose > > some of extended data structures. Ingress sampling is done on initial > > reception > > of packet, which means that at this stage required information might not be > > available yet. > > > > For instance, let's consider many-to-many source NAT translation > > configuration. > > During ingress sampling, only range of IP addresses that were specified by > > the > > user are known, but not the IP address that will be eventually selected for > > translation. This is an issue for sFlow, which will prevent us from > > exporting > > important data structure - extended NAT. > > > > To address this issue, sFlow
Re: [ovs-dev] [PATCH 2/2] ofproto-dpif-ipfix: add interface Information Elements to flow key
Hello Greg,
Unfortunately Przemek is not available, but I'm more or less familiar with this
patch.
Did you manage to fix this issue? I think that your problems lies on connection
level, which wasn't modified by this patch. Patch added additional counters on
reporting level, and based on your description, ovs just can't connect to
collector.
Br,
Michal.
> -Original Message-
> From: ovs-dev-boun...@openvswitch.org
> [mailto:ovs-dev-boun...@openvswitch.org] On Behalf Of Greg Rose
> Sent: Saturday, August 19, 2017 12:51 AM
> To: Szczerbik, PrzemyslawX
> Cc: d...@openvswitch.org
> Subject: Re: [ovs-dev] [PATCH 2/2] ofproto-dpif-ipfix: add interface
> Information Elements to flow key
>
> On 08/16/2017 01:54 AM, Szczerbik, PrzemyslawX wrote:
> > Hi,
> >
> > I haven't received any feedback on this patch for quite some time.
> > Is there anything that I can do to expedite review process?
> >
> > Regards,
> > Przemek
> >
>
> Przemek,
>
> I'm in the process of looking into this patch but I'm running into an issue
> with the vswitch not sending flows to the ntopng/nprobe
> collector I have set up. I see this in the vswitchd log:
>
> 2017-08-18T21:48:57.917Z|00058|collectors|WARN|couldn't open connection to
> collector 192.168.0.21:2055 (Network is
> unreachable)
> 2017-08-18T21:48:57.917Z|00059|ipfix|WARN|no collectors could be initialized,
> IPFIX exporter disabled
>
> However, 192.168.0.21 is reachable, at least from the br0 bridge that has the
> IPFIX flows enabled.
>
> ntopng/nprobe on the collector machine has the right ports open:
>
> netstat -tulpen | grep 2055
> udp0 0 0.0.0.0:20550.0.0.0:*
> 99 27671 3038/nprobe
> udp6 0 0 :::2055 :::*
> 99 27672 3038/nprobe
>
> netstat -tulpen | grep 5556
> tcp0 0 0.0.0.0:55560.0.0.0:* LISTEN
> 0 27666 3038/nprobe
>
> I'm not sure exactly what the problem is but I'm debugging that. Until I can
> get past this issue I can't really test and review your
> patch. I am actively working on getting this issue fixed though.
>
> Regards,
>
> - Greg
> > > -Original Message-
> > > From: ovs-dev-boun...@openvswitch.org [mailto:ovs-dev-
> > > boun...@openvswitch.org] On Behalf Of Szczerbik, PrzemyslawX
> > > Sent: Wednesday, July 26, 2017 12:01 PM
> > > To: d...@openvswitch.org
> > > Subject: Re: [ovs-dev] [PATCH 2/2] ofproto-dpif-ipfix: add interface
> > > Information
> > > Elements to flow key
> > >
> > > Hi,
> > >
> > > This patch was supposed to be v2, but I forgot to mention that in the
> > > subject.
> > > Previous version: https://patchwork.ozlabs.org/patch/792730/
> > >
> > > Let me know if you want me to re-sent it with a proper version.
> > >
> > > Regards,
> > > Przemek
> > >
> > >> -Original Message-
> > >> From: Szczerbik, PrzemyslawX
> > >> Sent: Wednesday, July 26, 2017 10:44 AM
> > >> To: d...@openvswitch.org
> > >> Cc: Szczerbik, PrzemyslawX
> > >> Subject: [PATCH 2/2] ofproto-dpif-ipfix: add interface Information
> > >> Elements to
> > >> flow key
> > >>
> > >> Extend flow key part of data record to include following Information
> > >> Elements:
> > >> - ingressInterface
> > >> - ingressInterfaceType
> > >> - egressInterface
> > >> - egressInterfaceType
> > >> - interfaceName
> > >> - interfaceDescription
> > >>
> > >> In case of input sampling we don't have information about egress port.
> > >> Define templates depending not only on protocol types, but also on flow
> > >> direction. Only egress flow will include egress information elements.
> > >>
> > >> With this change, dpif_ipfix_exporter stores every port in hmap rather
> > >> than only tunnel ports. It allows to easily retrieve required
> > >> information about interfaces during sampling upcalls.
> > >>
> > >> Signed-off-by: Przemyslaw Szczerbik
> > >> ---
> > >> v1->v2
> > >> * Add interfaceType and interfaceDescription
> > >> * Rework ipfix_get_iface_data_record function
> > >>
> > >> ofproto/ofproto-dpif-ipfix.c | 356 +++-
> > > --
> > >> -
> > >> ofproto/ofproto-dpif-ipfix.h | 6 +-
> > >> ofproto/ofproto-dpif-xlate.c | 4 +-
> > >> ofproto/ofproto-dpif.c | 19 +--
> > >> 4 files changed, 275 insertions(+), 110 deletions(-)
> > >>
> > >> diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
> > >> index 13ff426..e7ce279 100644
> > >> --- a/ofproto/ofproto-dpif-ipfix.c
> > >> +++ b/ofproto/ofproto-dpif-ipfix.c
> > >> @@ -113,11 +113,12 @@ struct dpif_ipfix_global_stats {
> > >> };
> > >>
> > >> struct dpif_ipfix_port {
> > >> -struct hmap_node hmap_node; /* In struct dpif_ipfix's "tunnel_ports"
> > > hmap.
> > >> */
> > >> +struct hmap_node hmap_node; /* In struct dpif_ipfix's "ports" hmap.
> > >> */
> > >> struct ofport *ofport; /* To retrieve port stats. */
Re: [ovs-dev] [PATCH v4] Update relevant artifacts to add support for DPDK 17.05.1.
Hey Mark, Sorry, I had some problems with rebase, and somehow I just simply forgot t to do so. Do you want me to re-apply this patch as v5? > -Original Message- > From: Kavanagh, Mark B > Sent: Wednesday, July 26, 2017 10:04 AM > To: Weglicki, MichalX ; d...@openvswitch.org > Subject: RE: [ovs-dev] [PATCH v4] Update relevant artifacts to add support > for DPDK 17.05.1. > > > > >-Original Message- > >From: ovs-dev-boun...@openvswitch.org > >[mailto:ovs-dev-boun...@openvswitch.org] > >On Behalf Of Michal Weglicki > >Sent: Tuesday, July 25, 2017 1:35 PM > >To: d...@openvswitch.org > >Subject: [ovs-dev] [PATCH v4] Update relevant artifacts to add support for > >DPDK 17.05.1. > > > >Upgrading to DPDK 17.05.1 stable release adds new > >significant features relevant to OVS, including, > >but not limited to: > >- tun/tap PMD, > >- VFIO hotplug support, > >- Generic flow API. > > > >Following changes are applied: > >- netdev-dpdk: Changes required by DPDK API modifications. > >- doc: Because of DPDK API changes, backward compatibility > > with previous DPDK releases will be broken, thus all > > relevant documentation entries are updated. > >- .travis: DPDK version change from 16.11.1 to 17.05.1. > >- rhel/openvswitch-fedora.spec.in: DPDK version change > > from 16.11 to 17.05.1 > > > >v1->v2: Patch rebase. > >v2->v3: Fixed wrong formating after v2 patch rebase. > >v3->v4: Minor documentation changes. > > > >Signed-off-by: Michal Weglicki > >Reviewed-by: Aaron Conole > > > Hi Michal, > > Is there a reason why you haven't added my acked-by to this latest patch? > https://mail.openvswitch.org/pipermail/ovs-dev/2017- > July/334893.html > > Thanks, > Mark > > >--- > > .travis/linux-build.sh | 2 +- > > Documentation/faq/releases.rst | 1 + > > Documentation/howto/dpdk.rst | 6 +- > > Documentation/intro/install/dpdk.rst | 14 +-- > > Documentation/topics/dpdk/vhost-user.rst | 12 +-- > > NEWS | 1 + > > lib/netdev-dpdk.c| 144 > > +++--- > >- > > rhel/openvswitch-fedora.spec.in | 2 +- > > tests/dpdk/ring_client.c | 6 +- > > 9 files changed, 114 insertions(+), 74 deletions(-) > > > >diff --git a/.travis/linux-build.sh b/.travis/linux-build.sh > >index f66b534..efccdf1 100755 > >--- a/.travis/linux-build.sh > >+++ b/.travis/linux-build.sh > >@@ -80,7 +80,7 @@ fi > > > > if [ "$DPDK" ]; then > > if [ -z "$DPDK_VER" ]; then > >-DPDK_VER="16.11.2" > >+DPDK_VER="17.05.1" > > fi > > install_dpdk $DPDK_VER > > if [ "$CC" = "clang" ]; then > >diff --git a/Documentation/faq/releases.rst b/Documentation/faq/releases.rst > >index 707834b..2ecc24c 100644 > >--- a/Documentation/faq/releases.rst > >+++ b/Documentation/faq/releases.rst > >@@ -161,6 +161,7 @@ Q: What DPDK version does each Open vSwitch release work > >with? > > 2.5.x2.2 > > 2.6.x16.07.2 > > 2.7.x16.11.2 > >+2.8.x17.05.1 > > === > > > > Q: I get an error like this when I configure Open vSwitch: > >diff --git a/Documentation/howto/dpdk.rst b/Documentation/howto/dpdk.rst > >index af01d3e..1756c8c 100644 > >--- a/Documentation/howto/dpdk.rst > >+++ b/Documentation/howto/dpdk.rst > >@@ -528,15 +528,15 @@ described in :ref:`dpdk-testpmd`. Once compiled, run > >the > >application:: > > > > When you finish testing, bind the vNICs back to kernel:: > > > >-$ $DPDK_DIR/tools/dpdk-devbind.py --bind=virtio-pci :00:03.0 > >-$ $DPDK_DIR/tools/dpdk-devbind.py --bind=virtio-pci :00:04.0 > >+$ $DPDK_DIR/usertools/dpdk-devbind.py --bind=virtio-pci :00:03.0 > >+$ $DPDK_DIR/usertools/dpdk-devbind.py --bind=virtio-pci :00:04.0 > > > > .. note:: > > > > Valid PCI IDs must be passed in above example. The PCI IDs can be > > retrieved > > like so:: > > > >- $ $DPDK_DIR/tools/dpdk-devbind.py --status > >+ $ $DPDK_DIR/usertools/dpdk-devbind.py --status > > > > More information on the dpdkvhostuser ports can be found in > > :doc:`/topics/dpdk/vhost-user`. > >diff --git a/Documentation/intro/install/dpdk.rst > >b/Documentation/intro/install/dp
Re: [ovs-dev] [PATCH v3] Update relevant artifacts to add support for DPDK 17.05.1.
Hi All, VHOST Client reconnection also works without any problems. Suggested documentation changes will be part of v4 patch which will be available shortly. Br, Michal. > -Original Message- > From: Stokes, Ian > Sent: Monday, July 24, 2017 7:15 PM > To: Darrell Ball ; Kevin Traynor ; > Weglicki, MichalX ; > d...@openvswitch.org > Subject: RE: [ovs-dev] [PATCH v3] Update relevant artifacts to add support > for DPDK 17.05.1. > > > On 7/19/17, 9:40 AM, "ovs-dev-boun...@openvswitch.org on behalf of Kevin > > Traynor" > ktray...@redhat.com> wrote: > > > > On 07/19/2017 10:30 AM, Michal Weglicki wrote: > > > Upgrading to DPDK 17.05.1 stable release adds new > > > significant features relevant to OVS, including, > > > but not limited to: > > > - tun/tap PMD, > > > - VFIO hotplug support, > > > - Generic flow API. > > > > > > Following changes are applied: > > > - netdev-dpdk: Changes required by DPDK API modifications. > > > - doc: Because of DPDK API changes, backward compatibility > > > with previous DPDK releases will be broken, thus all > > > relevant documentation entries are updated. > > > - .travis: DPDK version change from 16.11.1 to 17.05.1. > > > - rhel/openvswitch-fedora.spec.in: DPDK version change > > > from 16.11 to 17.05.1 > > > > > > > Hi Michal, were you able to check vhost features like multi-queue and > > vhostclient reconnect are still working ok with this patch? > Hi All, > > I've tested the vhost multi queue aspect of this patch today and I found no > issues with it. > > Thanks > Ian > > > > Few comments on the docs below. > > > > > v1->v2: Patch rebase. > > > v2->v3: Fixed wrong formating after v2 patch rebase. > > > > > > Signed-off-by: Michal Weglicki > > > Reviewed-by: Aaron Conole > > > --- > > > .travis/linux-build.sh | 2 +- > > > Documentation/intro/install/dpdk.rst | 8 +- > > > Documentation/topics/dpdk/vhost-user.rst | 8 +- > > > NEWS | 1 + > > > lib/netdev-dpdk.c| 144 +++- > > --- > > > rhel/openvswitch-fedora.spec.in | 2 +- > > > tests/dpdk/ring_client.c | 6 +- > > > 7 files changed, 105 insertions(+), 66 deletions(-) > > > > > > diff --git a/.travis/linux-build.sh b/.travis/linux-build.sh > > > index f66b534..efccdf1 100755 > > > --- a/.travis/linux-build.sh > > > +++ b/.travis/linux-build.sh > > > @@ -80,7 +80,7 @@ fi > > > > > > if [ "$DPDK" ]; then > > > if [ -z "$DPDK_VER" ]; then > > > -DPDK_VER="16.11.2" > > > +DPDK_VER="17.05.1" > > > fi > > > install_dpdk $DPDK_VER > > > if [ "$CC" = "clang" ]; then > > > diff --git a/Documentation/intro/install/dpdk.rst > > b/Documentation/intro/install/dpdk.rst > > > index a05aa1a..4a178f3 100644 > > > --- a/Documentation/intro/install/dpdk.rst > > > +++ b/Documentation/intro/install/dpdk.rst > > > @@ -40,7 +40,7 @@ Build requirements > > > In addition to the requirements described in :doc:`general`, > > building Open > > > vSwitch with DPDK will require the following: > > > > > > -- DPDK 16.11 > > > +- DPDK 17.05.1 > > > > > > - A `DPDK supported NIC`_ > > > > > > @@ -69,9 +69,9 @@ Install DPDK > > > #. Download the `DPDK sources`_, extract the file and set > > ``DPDK_DIR``:: > > > > > > $ cd /usr/src/ > > > - $ wget https://urldefense.proofpoint.com/v2/url?u=http- > > 3A__fast.dpdk.org_rel_dpdk- > > 2D16.11.2.tar.xz&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih- > > uZnsw&m=k97jBbskm5qtu47B5YEDKLa6LCgJq7IpV9fRXIASszk&s=VXewRbmVuHTeOAdaJZA9 > > ivHiJCZFh617b1ay74Fjhqs&e= > > > - $ tar xf dpdk-16.11.2.tar.xz > > > - $ export DPDK_DIR=/usr/src/dpdk-stable-16.11.2 > > > + $ wget https://urldefense.proofpoint.com/v2/url?u=http- > > 3A__fast.dpdk.org_rel_dpdk- >
Re: [ovs-dev] [PATCH v2] Update relevant artifacts to add support for DPDK 17.05.
Hello, Thank all for your comments, my answers inline. Br, Michal. > -Original Message- > From: Darrell Ball [mailto:db...@vmware.com] > Sent: Thursday, June 1, 2017 4:41 AM > To: Kavanagh, Mark B ; Kevin Traynor > ; Weglicki, MichalX > ; d...@openvswitch.org > Subject: Re: [ovs-dev] [PATCH v2] Update relevant artifacts to add support > for DPDK 17.05. > > > > On 5/31/17, 4:36 PM, "ovs-dev-boun...@openvswitch.org on behalf of Darrell > Ball" of db...@vmware.com> wrote: > > I did a pass through the changes. > Few comments > > On 5/31/17, 6:09 AM, "ovs-dev-boun...@openvswitch.org on behalf of > Kavanagh, Mark B" on behalf of mark.b.kavan...@intel.com> wrote: > > > >From: ovs-dev-boun...@openvswitch.org > [mailto:ovs-dev-boun...@openvswitch.org] On Behalf Of > >Kevin Traynor > >Sent: Wednesday, May 31, 2017 1:53 PM > >To: Weglicki, MichalX ; > d...@openvswitch.org > >Subject: Re: [ovs-dev] [PATCH v2] Update relevant artifacts to add > support for DPDK 17.05. > > > >On 05/31/2017 10:47 AM, mweglicx wrote: > >> Following changes are applied: > >> - netdev-dpdk: Changes required by DPDK API modifications. > >> - doc: Because of DPDK API changes, backward compatibility > >> with previous DPDK releases will be broken, thus all > >> relevant documentation entries are updated. > >> - .travis: DPDK version change from 16.11.1 to 17.05. > >> - rhel/openvswitch-fedora.spec.in: DPDK version change > >> from 16.11 to 17.05. > >> > >> v1->v2: Patch rework based on minor review comments. > > > Hi Michal, > > Apart from the changes suggested by Kevin, LGTM. > > Thanks, > Mark > > > >> > >> Signed-off-by: Michal Weglicki > >> --- > >> .travis/linux-build.sh | 2 +- > >> Documentation/faq/releases.rst | 3 +- > >> Documentation/intro/install/dpdk.rst | 8 +-- > >> Documentation/topics/dpdk/vhost-user.rst | 8 +-- > >> lib/netdev-dpdk.c| 114 > --- > >> rhel/openvswitch-fedora.spec.in | 2 +- > >> tests/dpdk/ring_client.c | 6 +- > >> 7 files changed, 75 insertions(+), 68 deletions(-) > >> > >> diff --git a/.travis/linux-build.sh b/.travis/linux-build.sh > >> index 8750d68..ec15fd8 100755 > >> --- a/.travis/linux-build.sh > >> +++ b/.travis/linux-build.sh > >> @@ -80,7 +80,7 @@ fi > >> > >> if [ "$DPDK" ]; then > >> if [ -z "$DPDK_VER" ]; then > >> -DPDK_VER="16.11.1" > >> +DPDK_VER="17.05" > >> fi > >> install_dpdk $DPDK_VER > >> if [ "$CC" = "clang" ]; then > >> diff --git a/Documentation/faq/releases.rst > b/Documentation/faq/releases.rst > >> index c85eff8..0455a43 100644 > >> --- a/Documentation/faq/releases.rst > >> +++ b/Documentation/faq/releases.rst > >> @@ -160,7 +160,8 @@ Q: What DPDK version does each Open vSwitch > release work with? > >> 2.4.x2.0 > >> 2.5.x2.2 > >> 2.6.x16.07.2 > >> -2.7.x16.11.1 > >> +2.7.016.11.1 > >> +2.7.0+ 17.05 > >> === > > > >This is about OVS releases. I think you should revert back to the > >original text for 2.7.x. Any OVS 2.7.x release from branch-2.7 would > >likely not use DPDK 17.05. > > That's a good point - +1 on this. > > Yep [MW] Will do. > > > > > >When OVS 2.8 is close to release it can be updated with it's DPDK > >version, or you could set it now and it can be changed later if > required. > > > >> > >> Q: I get an error like this when I configure Open vSwitch: > >> diff --git a/Documentation/intro/install/dpdk
Re: [ovs-dev] [PATCH v2] ofproto-dpif-ipfix: total counters.
> -Original Message- > From: Ben Pfaff [mailto:b...@ovn.org] > Sent: Monday, May 1, 2017 7:36 PM > To: Weglicki, MichalX > Cc: d...@openvswitch.org > Subject: Re: [ovs-dev] [PATCH v2] ofproto-dpif-ipfix: total counters. > > On Thu, Apr 20, 2017 at 03:25:13PM +0100, mweglicx wrote: > > Implementation of IPFix counters which hold > > total values measured since metering process startup. > > > > v2: Patch is reapplied for a review because > > it hasn't been correctly marked in patchwork. > > > > Signed-off-by: Michal Weglicki > > Thanks a lot for working on IPFix! I don't have IPFix expertise, and I > don't think the other OVS committers have much either, so I need to ask > some dumb questions to better understand the implications of this patch. > > Actually, maybe I just need to ask one. This changes the wire format of > the IPFix reports that go out. Will this require changes to IPFix > collectors that understood the previous format, that is, is the new > format backward compatible with collectors that expected the previous > format? I understand that IPFix uses a template format to tell > collectors the format of what it's sending, but it's not clear to me > whether or not that prevents this kind of compatibility issue. > > Thanks, > > Ben. Hello Ben, So counters which just got exposed through my patch are part of RFC which is already partially implemented in OVS. I've just added few missing ones, and I'm going to implement also others from same category (https://tools.ietf.org/html/rfc5102#section-5.10) However to answer your question directly those counters are part of standard group of IPFix counters, so no changes on Collector side are required. IPFix sends counters in kind of dictionary format, so adding any counter shouldn't break backward compatibility - IPFix even defines something like enterprise counters (vendor specific) where any agent can define own counters in the template and send those over (not every collector supports it, but data-wise it shouldn't break anything on collector side). My patch was also tested against libipfix/ipfix_collector and no additional changes were required to see new counters on collector side. Br, Michal. -- Intel Research and Development Ireland Limited Registered in Ireland Registered Office: Collinstown Industrial Park, Leixlip, County Kildare Registered Number: 308263 This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] Expose sFLOW "Processor information" counters.
Hello Neil,
Definitely I’m not sFLOW expert, as I didn’t even know anything about hsflowd.
Nevertheless before declining my proposal, maybe it is good to think about
other benefits. I do think that such basic statistics which are available by
default without any third party tool can simplify configuration for some users.
Also I can’t find in sflow_host cpu_utilization, load average is little
different statistic type, I know this is minor. I also thing that keeping
simple API to get most common system stats on OVS level, could be also
beneficial for some other functionalities in the future.
Br,
Michal.
From: Neil McKee [mailto:neil.mc...@inmon.com]
Sent: Tuesday, November 22, 2016 12:14 AM
To: Weglicki, MichalX
Cc: d...@openvswitch.org
Subject: Re: [ovs-dev] [PATCH] Expose sFLOW "Processor information" counters.
Hello Michal,
Most systems that run Open vSwitch can also run hsflowd to export a wide range
of standard host performance statistics:
http://sflow.org/sflow_host.txt
http://sflow.org/sflow_host_ip.txt
That's a large superset of the "struct processor" export you are filling in
here.
This "struct processor" was originally aimed at providing some visibility into
the CPU/memory status of closed-platform physical switches, but it has really
been superseded in places where sflow_host is available. Newer physical
switches tend to run Linux, and usually run hsflowd (Cumulus Linux, Dell
OS10, ...).
The hsflowd daemon integrates with OVS (just add "ovs{}" to /etc/hsflowd.conf
and it will configure OVS sFlow): http://sflow.net/host-sflow-linux-config.php
So I'm wondering if this patch is worthwhile?
But perhaps I missed something. Is there a use-case where it's unlikely that
hsflowd can be layered on top?
Neil
--
Neil McKee
InMon Corp.
http://www.inmon.com
On Mon, Nov 21, 2016 at 5:34 AM, mweglicx
mailto:michalx.wegli...@intel.com>> wrote:
Exports PROCESSOR structure with sFLOW counters.
Refactors vswitchd/system-stats.* functionality to support
complex system counters which are enabled on request, and basic stats
enabled by default.
Adjust unit tests with very basic sanity check for PROCESSOR counters.
Signed-off-by: Michal Weglicki
mailto:michalx.wegli...@intel.com>>
---
lib/sflow.h | 12 ++
lib/sflow_receiver.c | 8 ++
ofproto/ofproto-dpif-sflow.c | 17 ++-
tests/ofproto-dpif.at<http://ofproto-dpif.at>| 28 +++-
tests/test-sflow.c | 15 +++
vswitchd/bridge.c| 1 +
vswitchd/system-stats.c | 308 ---
vswitchd/system-stats.h | 16 +++
8 files changed, 352 insertions(+), 53 deletions(-)
diff --git a/lib/sflow.h b/lib/sflow.h
index 95bedd9..53ba4e0 100644
--- a/lib/sflow.h
+++ b/lib/sflow.h
@@ -502,6 +502,16 @@ typedef struct _SFLVlan_counters {
u_int32_t discards;
} SFLVlan_counters;
+#define SFL_CTR_PROCESSOR_XDR_SIZE 28
+
+typedef struct _SFLProcessor_counters {
+int32_t cpu_util_5s; /* 5 second average CPU utilization */
+int32_t cpu_util_1m; /* 1 minute average CPU utilization */
+int32_t cpu_util_5m; /* 5 minute average CPU utilization */
+u_int64_t total_memory; /* total memory (in bytes) */
+u_int64_t free_memory; /* free memory (in bytes) */
+} SFLProcessor_counters;
+
/* OpenFlow port */
typedef struct {
u_int64_t datapath_id;
@@ -587,6 +597,7 @@ enum SFLCounters_type_tag {
SFLCOUNTERS_VG = 4,
SFLCOUNTERS_VLAN = 5,
SFLCOUNTERS_LACP = 7,
+SFLCOUNTERS_PROCESSOR= 1001,
SFLCOUNTERS_OPENFLOWPORT = 1004,
SFLCOUNTERS_PORTNAME = 1005,
SFLCOUNTERS_APP_RESOURCES = 2203,
@@ -604,6 +615,7 @@ typedef union _SFLCounters_type {
SFLPortName portName;
SFLAPPResources_counters appResources;
SFLOVSDP_counters ovsdp;
+SFLProcessor_counters processor;
} SFLCounters_type;
typedef struct _SFLCounters_sample_element {
diff --git a/lib/sflow_receiver.c b/lib/sflow_receiver.c
index cde1359..23468f1 100644
--- a/lib/sflow_receiver.c
+++ b/lib/sflow_receiver.c
@@ -655,6 +655,7 @@ 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_PROCESSOR: elemSiz = SFL_CTR_PROCESSOR_XDR_SIZE; break;
default:
sflError(receiver, "unexpected counters_tag");
return -1;
@@ -795,6 +796,13 @@ int sfl_receiver_writeCountersSample(SFLReceiver
*receiver, SFL_COUNTERS_SAMPLE_
putNet32(receiver, elem->counterBlock.ovsdp.n_flows);
putNet32(receiver, ele
