| Message ID | HK2PR0301MB1954F98E35DACCC193AF84689FBB0@HK2PR0301MB1954.apcprd03.prod.outlook.com |
|---|---|
| State | Rejected |
| Headers | show |
<ravali.burra@wipro.com> writes: > Hi All, > > Below are the configuration and patch details which provides the support > for clearing statistics at Interface level. > Description & Configuration: > OVS by default has the support for the command ovs-vsctl clear interface <name> statistics > But in OVS the command functionality is implemented in such a way that we are just > clearing the key and values and Number of statistics fields respective to the statistics > column of Interface table at the db level, which was not clearing the statistics at > Hardware level so we have extended the support to clear the statistics at Hardware level. > > Please find the below patch for implementation of clear statistics at > Interface level. Your patch is malformed - please use a different email server (something like gmail). Worst case - attach the patch. Please run against checkpatch - you have a few errors that could be caught there. You need to provide a method for this to work when built as a non-dpdk application. Currently, this will not link for non-dpdk applications. > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > index 7c1523e..71e2c7e 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -1866,6 +1866,15 @@ netdev_dpdk_convert_xstats(struct netdev_stats *stats, > } > } > +int > +get_stats_resets(const struct netdev *netdev) > +{ > + struct netdev_dpdk *dev = netdev_dpdk_cast(netdev); > + memset(&dev->stats, 0, sizeof(dev->stats)); > + rte_eth_stats_reset(dev->port_id); > + return 0; > +} > + > static int > netdev_dpdk_get_stats(const struct netdev *netdev, struct netdev_stats *stats) > { > diff --git a/lib/netdev.h b/lib/netdev.h > index bad28c4..72a25a6 100644 > --- a/lib/netdev.h > +++ b/lib/netdev.h > @@ -221,6 +221,7 @@ struct netdev *netdev_find_dev_by_in4(const struct in_addr *); > /* Statistics. */ > int netdev_get_stats(const struct netdev *, struct netdev_stats *); > +int get_stats_resets(const struct netdev *); > /* Quality of service. */ > struct netdev_qos_capabilities { > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c > index ff5d86f..535c37c 100644 > --- a/vswitchd/bridge.c > +++ b/vswitchd/bridge.c > @@ -657,6 +657,10 @@ bridge_reconfigure(const struct ovsrec_open_vswitch *ovs_cfg) > &iface->cfg->lldp); > ofproto_port_set_config(br->ofproto, iface->ofp_port, > &iface->cfg->other_config); > + if(iface->cfg->n_statistics == 0){ > + get_stats_resets(iface->netdev); > + } > + > } > } > bridge_configure_mirrors(br); > The information contained in this electronic message and any attachments to this message are intended for the exclusive use of the addressee(s) and may contain proprietary, confidential or privileged information. If you are not the intended recipient, you should not disseminate, distribute or copy this e-mail. Please notify the sender immediately and destroy all copies of this message and any attachments. WARNING: Computer viruses can be transmitted via email. The recipient should check this email and any attachments for the presence of viruses. The company accepts no liability for any damage caused by any virus transmitted by this email. www.wipro.com > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Hi Aaron, As you suggested I have attached the patch. Please find the attached patch and let me know your comments.. -----Original Message----- From: Aaron Conole [mailto:aconole@redhat.com] Sent: Monday, November 14, 2016 9:15 PM To: Ravali Burra <ravali.burra@wipro.com> Cc: dev@openvswitch.org Subject: Re: [ovs-dev] [PATCH 1/1] : Add Clear statistics support at interface level ** This mail has been sent from an external source ** <ravali.burra@wipro.com> writes: > Hi All, > > Below are the configuration and patch details which provides the > support for clearing statistics at Interface level. > Description & Configuration: > OVS by default has the support for the command ovs-vsctl clear > interface <name> statistics But in OVS the command functionality is > implemented in such a way that we are just clearing the key and values > and Number of statistics fields respective to the statistics column of > Interface table at the db level, which was not clearing the statistics at Hardware level so we have extended the support to clear the statistics at Hardware level. > > Please find the below patch for implementation of clear statistics at > Interface level. Your patch is malformed - please use a different email server (something like gmail). Worst case - attach the patch. Please run against checkpatch - you have a few errors that could be caught there. You need to provide a method for this to work when built as a non-dpdk application. Currently, this will not link for non-dpdk applications. > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index > 7c1523e..71e2c7e 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -1866,6 +1866,15 @@ netdev_dpdk_convert_xstats(struct netdev_stats *stats, > } > } > +int > +get_stats_resets(const struct netdev *netdev) { > + struct netdev_dpdk *dev = netdev_dpdk_cast(netdev); > + memset(&dev->stats, 0, sizeof(dev->stats)); > + rte_eth_stats_reset(dev->port_id); > + return 0; > +} > + > static int > netdev_dpdk_get_stats(const struct netdev *netdev, struct netdev_stats > *stats) { diff --git a/lib/netdev.h b/lib/netdev.h index > bad28c4..72a25a6 100644 > --- a/lib/netdev.h > +++ b/lib/netdev.h > @@ -221,6 +221,7 @@ struct netdev *netdev_find_dev_by_in4(const struct > in_addr *); > /* Statistics. */ > int netdev_get_stats(const struct netdev *, struct netdev_stats *); > +int get_stats_resets(const struct netdev *); > /* Quality of service. */ > struct netdev_qos_capabilities { > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c index > ff5d86f..535c37c 100644 > --- a/vswitchd/bridge.c > +++ b/vswitchd/bridge.c > @@ -657,6 +657,10 @@ bridge_reconfigure(const struct ovsrec_open_vswitch *ovs_cfg) > &iface->cfg->lldp); > ofproto_port_set_config(br->ofproto, iface->ofp_port, > &iface->cfg->other_config); > + if(iface->cfg->n_statistics == 0){ > + get_stats_resets(iface->netdev); > + } > + > } > } > bridge_configure_mirrors(br); The information contained in > this electronic message and any attachments to this message are > intended for the exclusive use of the addressee(s) and may contain > proprietary, confidential or privileged information. If you are not > the intended recipient, you should not disseminate, distribute or copy > this e-mail. Please notify the sender immediately and destroy all > copies of this message and any attachments. WARNING: Computer viruses > can be transmitted via email. The recipient should check this email > and any attachments for the presence of viruses. The company accepts > no liability for any damage caused by any virus transmitted by this > email. www.wipro.com _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev The information contained in this electronic message and any attachments to this message are intended for the exclusive use of the addressee(s) and may contain proprietary, confidential or privileged information. If you are not the intended recipient, you should not disseminate, distribute or copy this e-mail. Please notify the sender immediately and destroy all copies of this message and any attachments. WARNING: Computer viruses can be transmitted via email. The recipient should check this email and any attachments for the presence of viruses. The company accepts no liability for any damage caused by any virus transmitted by this email. www.wipro.com
<ravali.burra@wipro.com> writes: > Hi Aaron, > > As you suggested I have attached the patch. > Please find the attached patch and let me know your comments.. These are the errors from `checkpatch.py clear_statistics.patch`: E: No signatures found. In file b/lib/netdev-dpdk.c W(1872): Line has non-spaces leading whitespace + struct netdev_dpdk *dev = netdev_dpdk_cast(netdev); W(1873): Line has non-spaces leading whitespace + memset(&dev->stats, 0, sizeof(dev->stats)); In file b/vswitchd/bridge.c W(660): Line has non-spaces leading whitespace + if(iface->cfg->n_statistics == 0){ W(661): Line has non-spaces leading whitespace + get_stats_resets(iface->netdev); W(662): Line has non-spaces leading whitespace + } Warnings: 5, Errors: 1 Please fix these. You can find checkpatch in the utilities/ directory. You also need to address the other point (how will non-dpdk enabled openvswitch cope with this?) I don't know the statistics system enough to comment on the validity of the change you propose. Why is not clearing the database data good enough? > -----Original Message----- > From: Aaron Conole [mailto:aconole@redhat.com] > Sent: Monday, November 14, 2016 9:15 PM > To: Ravali Burra <ravali.burra@wipro.com> > Cc: dev@openvswitch.org > Subject: Re: [ovs-dev] [PATCH 1/1] : Add Clear statistics support at interface level > > ** This mail has been sent from an external source ** > > <ravali.burra@wipro.com> writes: > >> Hi All, >> >> Below are the configuration and patch details which provides the >> support for clearing statistics at Interface level. >> Description & Configuration: >> OVS by default has the support for the command ovs-vsctl clear >> interface <name> statistics But in OVS the command functionality is >> implemented in such a way that we are just clearing the key and values >> and Number of statistics fields respective to the statistics column of >> Interface table at the db level, which was not clearing the >> statistics at Hardware level so we have extended the support to >> clear the statistics at Hardware level. >> >> Please find the below patch for implementation of clear statistics at >> Interface level. > > Your patch is malformed - please use a different email server >> (something like gmail). Worst case - attach the patch. > > Please run against checkpatch - you have a few errors that could be caught there. > > You need to provide a method for this to work when built as a non-dpdk >> application. Currently, this will not link for non-dpdk >> applications. > >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index >> 7c1523e..71e2c7e 100644 >> --- a/lib/netdev-dpdk.c >> +++ b/lib/netdev-dpdk.c >> @@ -1866,6 +1866,15 @@ netdev_dpdk_convert_xstats(struct netdev_stats *stats, >> } >> } >> +int >> +get_stats_resets(const struct netdev *netdev) { >> + struct netdev_dpdk *dev = netdev_dpdk_cast(netdev); >> + memset(&dev->stats, 0, sizeof(dev->stats)); >> + rte_eth_stats_reset(dev->port_id); >> + return 0; >> +} >> + >> static int >> netdev_dpdk_get_stats(const struct netdev *netdev, struct netdev_stats >> *stats) { diff --git a/lib/netdev.h b/lib/netdev.h index >> bad28c4..72a25a6 100644 >> --- a/lib/netdev.h >> +++ b/lib/netdev.h >> @@ -221,6 +221,7 @@ struct netdev *netdev_find_dev_by_in4(const struct >> in_addr *); >> /* Statistics. */ >> int netdev_get_stats(const struct netdev *, struct netdev_stats *); >> +int get_stats_resets(const struct netdev *); >> /* Quality of service. */ >> struct netdev_qos_capabilities { >> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c index >> ff5d86f..535c37c 100644 >> --- a/vswitchd/bridge.c >> +++ b/vswitchd/bridge.c >> @@ -657,6 +657,10 @@ bridge_reconfigure(const struct ovsrec_open_vswitch *ovs_cfg) >> &iface->cfg->lldp); >> ofproto_port_set_config(br->ofproto, iface->ofp_port, >> &iface->cfg->other_config); >> + if(iface->cfg->n_statistics == 0){ >> + get_stats_resets(iface->netdev); >> + } >> + >> } >> } >> bridge_configure_mirrors(br); The information contained in >> this electronic message and any attachments to this message are >> intended for the exclusive use of the addressee(s) and may contain >> proprietary, confidential or privileged information. If you are not >> the intended recipient, you should not disseminate, distribute or copy >> this e-mail. Please notify the sender immediately and destroy all >> copies of this message and any attachments. WARNING: Computer viruses >> can be transmitted via email. The recipient should check this email >> and any attachments for the presence of viruses. The company accepts >> no liability for any damage caused by any virus transmitted by this >> email. www.wipro.com _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > The information contained in this electronic message and any attachments to this message are intended for the exclusive use of the addressee(s) and may contain proprietary, confidential or privileged information. If you are not the intended recipient, you should not disseminate, distribute or copy this e-mail. Please notify the sender immediately and destroy all copies of this message and any attachments. WARNING: Computer viruses can be transmitted via email. The recipient should check this email and any attachments for the presence of viruses. The company accepts no liability for any damage caused by any virus transmitted by this email. www.wipro.com
On Fri, Nov 11, 2016 at 12:36:26PM +0000, ravali.burra@wipro.com wrote: > Below are the configuration and patch details which provides the support > for clearing statistics at Interface level. I don't think that this is a valuable feature, and it's incompatible with the kernel datapath. I don't think we should take it. This isn't the right way to do it in any case. A database field getting cleared shouldn't affect statistics reporting.
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index 7c1523e..71e2c7e 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -1866,6 +1866,15 @@ netdev_dpdk_convert_xstats(struct netdev_stats *stats, } } +int +get_stats_resets(const struct netdev *netdev) +{ + struct netdev_dpdk *dev = netdev_dpdk_cast(netdev); + memset(&dev->stats, 0, sizeof(dev->stats)); + rte_eth_stats_reset(dev->port_id); + return 0; +} + static int netdev_dpdk_get_stats(const struct netdev *netdev, struct netdev_stats *stats) { diff --git a/lib/netdev.h b/lib/netdev.h index bad28c4..72a25a6 100644 --- a/lib/netdev.h +++ b/lib/netdev.h @@ -221,6 +221,7 @@ struct netdev *netdev_find_dev_by_in4(const struct in_addr *); /* Statistics. */ int netdev_get_stats(const struct netdev *, struct netdev_stats *); +int get_stats_resets(const struct netdev *); /* Quality of service. */ struct netdev_qos_capabilities { diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c index ff5d86f..535c37c 100644 --- a/vswitchd/bridge.c +++ b/vswitchd/bridge.c @@ -657,6 +657,10 @@ bridge_reconfigure(const struct ovsrec_open_vswitch *ovs_cfg) &iface->cfg->lldp); ofproto_port_set_config(br->ofproto, iface->ofp_port, &iface->cfg->other_config); + if(iface->cfg->n_statistics == 0){ + get_stats_resets(iface->netdev); + } + } } bridge_configure_mirrors(br);