Nice results, reducing CPU usage by around 50% in that case :-)
Feedback below, mostly style.
On 12 April 2014 06:20, Alex Wang <al...@nicira.com> wrote:
> @@ -1460,15 +1460,17 @@ struct ofproto_class {
> * support CFM, as does a null pointer. */
> int (*set_cfm)(struct ofport *ofport, const struct cfm_settings *s);
>
> - /* Checks the status of CFM configured on 'ofport'. Returns true if
> the
> - * port's CFM status was successfully stored into '*status'. Returns
> false
> - * if the port did not have CFM configured, in which case '*status' is
> - * indeterminate.
> + /* Checks the status of CFM configured on 'ofport'. Returns 0 if the
> + * port's CFM status was successfully stored into '*status'. Returns
> + * negative number if there is no status change since last update.
> + * Returns positive errno otherwise. EOPNOTSUPP as a return value
> + * indicates that this ofproto_class does not support CFM, as does a
> + * null pointer.
>
Perhaps you could clarify that '*status' is indeterminate if this function
returns non-zero, just to be crystal clear?
Also, minor formatting thing: "EOPNOTSUPP..." could go on a new line like
other functions in the file.
(this comment goes for BFD in ofproto-provider.h too)
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index 436a745..86b4a02 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -1018,7 +1018,7 @@ ofproto_port_set_bfd(struct ofproto *ofproto,
> ofp_port_t ofp_port,
>
> /* Populates 'status' with key value pairs indicating the status of the
> BFD
> * session on 'ofp_port'. This information is intended to be populated
> in the
> - * OVS database. Has no effect if 'ofp_port' is not na OpenFlow port in
> + * OVS database. Has no effect if 'ofp_port' is not an OpenFlow port in
> * 'ofproto'. */
> int
> ofproto_port_get_bfd_status(struct ofproto *ofproto, ofp_port_t ofp_port,
>
This version of the comment seems to be missing a bunch of information
compared with the ofproto-provider.h one. Could you update it, too?
*
> * The caller must provide and owns '*status', and must free
> 'status->rmps'. */
> -bool
> +int
> ofproto_port_get_cfm_status(const struct ofproto *ofproto, ofp_port_t
> ofp_port,
> struct ofproto_cfm_status *status)
> {
> struct ofport *ofport = ofproto_get_port(ofproto, ofp_port);
> - return (ofport
> - && ofproto->ofproto_class->get_cfm_status
> - && ofproto->ofproto_class->get_cfm_status(ofport, status));
> + return (ofport && ofproto->ofproto_class->get_cfm_status ?
> + ofproto->ofproto_class->get_cfm_status(ofport, status)
> + : EOPNOTSUPP);
> }
>
I think the '?' usually goes at the start of the next line.
> @@ -1836,9 +1836,14 @@ iface_refresh_cfm_stats(struct iface *iface)
> {
> const struct ovsrec_interface *cfg = iface->cfg;
> struct ofproto_cfm_status status;
> + int error;
>
> - if (!ofproto_port_get_cfm_status(iface->port->bridge->ofproto,
> - iface->ofp_port, &status)) {
> + error = ofproto_port_get_cfm_status(iface->port->bridge->ofproto,
> + iface->ofp_port, &status);
> + /* Do nothing if there is no status change since last update. */
>
This comment could be placed inside the first if condition below to replace
that comment:
> + if (error < 0) {
> + // PASS.
> + } else if (error > 0) {
>
> @@ -2246,8 +2251,11 @@ instant_stats_run(void)
> iface_refresh_cfm_stats(iface);
>
> smap_init(&smap);
> - ofproto_port_get_bfd_status(br->ofproto, iface->ofp_port,
> - &smap);
> + error = ofproto_port_get_bfd_status(br->ofproto,
> iface->ofp_port,
> + &smap);
> + if (error < 0) {
> + continue;
>
Should smap_destroy() also be called in this case?
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
