I think cfm_get_status() deserves a comment in ofproto-provider.h and
ofproto.c, more out of convention than anything else.
Acked-by: Ethan Jackson <et...@nicira.com>
On Tue, Mar 5, 2013 at 4:28 PM, Ben Pfaff <b...@nicira.com> wrote:
> This eliminates several function calls and in particular several
> hash table lookups to find structures corresponding to a port
> number from iface_refresh_cfm_stats().
>
> This does not seem to reduce CPU use, but the code is shorter
> and more readable.
>
> Signed-off-by: Ben Pfaff <b...@nicira.com>
> ---
> ofproto/ofproto-dpif.c | 44 +++++++-------------------------
> ofproto/ofproto-provider.h | 42 ++----------------------------
> ofproto/ofproto.c | 60
> ++++---------------------------------------
> ofproto/ofproto.h | 22 +++++++++++-----
> vswitchd/bridge.c | 59
> ++++++++++++++----------------------------
> 5 files changed, 54 insertions(+), 173 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 7035530..625429c 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -1778,43 +1778,22 @@ set_cfm(struct ofport *ofport_, const struct
> cfm_settings *s)
> return error;
> }
>
> -static int
> -get_cfm_fault(const struct ofport *ofport_)
> -{
> - struct ofport_dpif *ofport = ofport_dpif_cast(ofport_);
> -
> - return ofport->cfm ? cfm_get_fault(ofport->cfm) : -1;
> -}
> -
> -static int
> -get_cfm_opup(const struct ofport *ofport_)
> -{
> - struct ofport_dpif *ofport = ofport_dpif_cast(ofport_);
> -
> - return ofport->cfm ? cfm_get_opup(ofport->cfm) : -1;
> -}
> -
> -static int
> -get_cfm_remote_mpids(const struct ofport *ofport_, const uint64_t **rmps,
> - size_t *n_rmps)
> +static bool
> +get_cfm_status(const struct ofport *ofport_,
> + struct ofproto_cfm_status *status)
> {
> struct ofport_dpif *ofport = ofport_dpif_cast(ofport_);
>
> if (ofport->cfm) {
> - cfm_get_remote_mpids(ofport->cfm, rmps, n_rmps);
> - return 0;
> + status->faults = cfm_get_fault(ofport->cfm);
> + status->remote_opstate = cfm_get_opup(ofport->cfm);
> + status->health = cfm_get_health(ofport->cfm);
> + cfm_get_remote_mpids(ofport->cfm, &status->rmps, &status->n_rmps);
> + return true;
> } else {
> - return -1;
> + return false;
> }
> }
> -
> -static int
> -get_cfm_health(const struct ofport *ofport_)
> -{
> - struct ofport_dpif *ofport = ofport_dpif_cast(ofport_);
> -
> - return ofport->cfm ? cfm_get_health(ofport->cfm) : -1;
> -}
>
> /* Spanning Tree. */
>
> @@ -8370,10 +8349,7 @@ const struct ofproto_class ofproto_dpif_class = {
> get_netflow_ids,
> set_sflow,
> set_cfm,
> - get_cfm_fault,
> - get_cfm_opup,
> - get_cfm_remote_mpids,
> - get_cfm_health,
> + get_cfm_status,
> set_stp,
> get_stp_status,
> set_stp_port,
> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
> index 95bda33..3b17a73 100644
> --- a/ofproto/ofproto-provider.h
> +++ b/ofproto/ofproto-provider.h
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2009, 2010, 2011, 2012 Nicira, Inc.
> + * Copyright (c) 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
> *
> * Licensed under the Apache License, Version 2.0 (the "License");
> * you may not use this file except in compliance with the License.
> @@ -1120,44 +1120,8 @@ struct ofproto_class {
> * support CFM, as does a null pointer. */
> int (*set_cfm)(struct ofport *ofport, const struct cfm_settings *s);
>
> - /* Checks the fault status of CFM configured on 'ofport'. Returns a
> - * bitmask of 'cfm_fault_reason's to indicate a CFM fault (generally
> - * indicating a connectivity problem). Returns zero if CFM is not
> faulted,
> - * and -1 if CFM is not enabled on 'port'.
> - *
> - * This function may be a null pointer if the ofproto implementation
> does
> - * not support CFM. */
> - int (*get_cfm_fault)(const struct ofport *ofport);
> -
> - /* Check the operational status reported by the remote CFM endpoint of
> - * 'ofp_port' Returns 1 if operationally up, 0 if operationally
> down, and
> - * -1 if CFM is not enabled on 'ofp_port' or does not support
> operational
> - * status.
> - *
> - * This function may be a null pointer if the ofproto implementation
> does
> - * not support CFM. */
> - int (*get_cfm_opup)(const struct ofport *ofport);
> -
> - /* Gets the MPIDs of the remote maintenance points broadcasting to
> - * 'ofport'. Populates 'rmps' with a provider owned array of MPIDs,
> and
> - * 'n_rmps' with the number of MPIDs in 'rmps'. Returns a number less
> than
> - * 0 if CFM is not enabled of 'ofport'.
> - *
> - * This function may be a null pointer if the ofproto implementation
> does
> - * not support CFM. */
> - int (*get_cfm_remote_mpids)(const struct ofport *ofport,
> - const uint64_t **rmps, size_t *n_rmps);
> -
> - /* Checks the health of CFM configured on 'ofport'. Returns an
> integer
> - * to indicate the health percentage of the 'ofport' which is an
> average of
> - * the health of all the remote_mps. Returns an integer between 0
> and 100
> - * where 0 means that the 'ofport' is very unhealthy and 100 means the
> - * 'ofport' is perfectly healthy. Returns -1 if CFM is not enabled on
> - * 'port' or if the number of remote_mpids is > 1.
> - *
> - * This function may be a null pointer if the ofproto implementation
> does
> - * not support CFM. */
> - int (*get_cfm_health)(const struct ofport *ofport);
> + bool (*get_cfm_status)(const struct ofport *ofport,
> + struct ofproto_cfm_status *status);
>
> /* Configures spanning tree protocol (STP) on 'ofproto' using the
> * settings defined in 's'.
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index a9c7e76..bf27179 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -2843,62 +2843,14 @@ ofproto_get_netflow_ids(const struct ofproto
> *ofproto,
> ofproto->ofproto_class->get_netflow_ids(ofproto, engine_type,
> engine_id);
> }
>
> -/* Checks the fault status of CFM for 'ofp_port' within 'ofproto'.
> Returns a
> - * bitmask of 'cfm_fault_reason's to indicate a CFM fault (generally
> - * indicating a connectivity problem). Returns zero if CFM is not
> faulted,
> - * and -1 if CFM is not enabled on 'ofp_port'. */
> -int
> -ofproto_port_get_cfm_fault(const struct ofproto *ofproto, uint16_t
> ofp_port)
> -{
> - struct ofport *ofport = ofproto_get_port(ofproto, ofp_port);
> - return (ofport && ofproto->ofproto_class->get_cfm_fault
> - ? ofproto->ofproto_class->get_cfm_fault(ofport)
> - : -1);
> -}
> -
> -/* Checks the operational status reported by the remote CFM endpoint of
> - * 'ofp_port' Returns 1 if operationally up, 0 if operationally down,
> and -1
> - * if CFM is not enabled on 'ofp_port' or does not support operational
> status.
> - */
> -int
> -ofproto_port_get_cfm_opup(const struct ofproto *ofproto, uint16_t
> ofp_port)
> -{
> - struct ofport *ofport = ofproto_get_port(ofproto, ofp_port);
> - return (ofport && ofproto->ofproto_class->get_cfm_opup
> - ? ofproto->ofproto_class->get_cfm_opup(ofport)
> - : -1);
> -}
> -
> -/* Gets the MPIDs of the remote maintenance points broadcasting to
> 'ofp_port'
> - * within 'ofproto'. Populates 'rmps' with an array of MPIDs owned by
> - * 'ofproto', and 'n_rmps' with the number of MPIDs in 'rmps'. Returns a
> - * number less than 0 if CFM is not enabled on 'ofp_port'. */
> -int
> -ofproto_port_get_cfm_remote_mpids(const struct ofproto *ofproto,
> - uint16_t ofp_port, const uint64_t
> **rmps,
> - size_t *n_rmps)
> -{
> - struct ofport *ofport = ofproto_get_port(ofproto, ofp_port);
> -
> - *rmps = NULL;
> - *n_rmps = 0;
> - return (ofport && ofproto->ofproto_class->get_cfm_remote_mpids
> - ? ofproto->ofproto_class->get_cfm_remote_mpids(ofport, rmps,
> - n_rmps)
> - : -1);
> -}
> -
> -/* Checks the health of the CFM for 'ofp_port' within 'ofproto'. Returns
> an
> - * integer value between 0 and 100 to indicate the health of the port as a
> - * percentage which is the average of cfm health of all the remote_mpids
> or
> - * returns -1 if CFM is not enabled on 'ofport'. */
> -int
> -ofproto_port_get_cfm_health(const struct ofproto *ofproto, uint16_t
> ofp_port)
> +bool
> +ofproto_port_get_cfm_status(const struct ofproto *ofproto, uint16_t
> ofp_port,
> + struct ofproto_cfm_status *status)
> {
> struct ofport *ofport = ofproto_get_port(ofproto, ofp_port);
> - return (ofport && ofproto->ofproto_class->get_cfm_health
> - ? ofproto->ofproto_class->get_cfm_health(ofport)
> - : -1);
> + return (ofport
> + && ofproto->ofproto_class->get_cfm_status
> + && ofproto->ofproto_class->get_cfm_status(ofport, status));
> }
>
> static enum ofperr
> diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
> index a217e0a..f164b3f 100644
> --- a/ofproto/ofproto.h
> +++ b/ofproto/ofproto.h
> @@ -355,15 +355,23 @@ void ofproto_get_snoops(const struct ofproto *,
> struct sset *);
> void ofproto_get_all_flows(struct ofproto *p, struct ds *);
> void ofproto_get_netflow_ids(const struct ofproto *,
> uint8_t *engine_type, uint8_t *engine_id);
> -int ofproto_port_get_cfm_fault(const struct ofproto *, uint16_t ofp_port);
> -int ofproto_port_get_cfm_opup(const struct ofproto *, uint16_t ofp_port);
> -int ofproto_port_get_cfm_remote_mpids(const struct ofproto *,
> - uint16_t ofp_port, const uint64_t
> **rmps,
> - size_t *n_rmps);
> -int ofproto_port_get_cfm_health(const struct ofproto *ofproto,
> - uint16_t ofp_port);
> +
> void ofproto_get_ofproto_controller_info(const struct ofproto *, struct
> shash *);
> void ofproto_free_ofproto_controller_info(struct shash *);
> +
> +/* CFM status query. */
> +struct ofproto_cfm_status {
> + enum cfm_fault_reason faults; /* 0 if not faulted. */
> + bool remote_opstate; /* True if remote CFM endpoint is up.
> */
> + int health; /* Health status in [0,100] range. */
> +
> + /* MPIDs of remote maintenance points whose CCMs have been received.
> */
> + const uint64_t *rmps;
> + size_t n_rmps;
> +};
> +
> +bool ofproto_port_get_cfm_status(const struct ofproto *, uint16_t
> ofp_port,
> + struct ofproto_cfm_status *);
>
> /* Linux VLAN device support (e.g. "eth0.10" for VLAN 10.)
> *
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index 993e4da..7bf169d 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -1742,57 +1742,38 @@ static void
> iface_refresh_cfm_stats(struct iface *iface)
> {
> const struct ovsrec_interface *cfg = iface->cfg;
> - int fault, opup, error;
> - const uint64_t *rmps;
> - size_t n_rmps;
> - int health;
> -
> - fault = ofproto_port_get_cfm_fault(iface->port->bridge->ofproto,
> - iface->ofp_port);
> - if (fault >= 0) {
> + struct ofproto_cfm_status status;
> +
> + if (!ofproto_port_get_cfm_status(iface->port->bridge->ofproto,
> + iface->ofp_port, &status)) {
> + ovsrec_interface_set_cfm_fault(cfg, NULL, 0);
> + ovsrec_interface_set_cfm_fault_status(cfg, NULL, 0);
> + ovsrec_interface_set_cfm_remote_opstate(cfg, NULL);
> + ovsrec_interface_set_cfm_health(cfg, NULL, 0);
> + ovsrec_interface_set_cfm_remote_mpids(cfg, NULL, 0);
> + } else {
> const char *reasons[CFM_FAULT_N_REASONS];
> - bool fault_bool = fault;
> + int64_t cfm_health = status.health;
> + bool faulted = status.faults != 0;
> size_t i, j;
>
> + ovsrec_interface_set_cfm_fault(cfg, &faulted, 1);
> +
> j = 0;
> for (i = 0; i < CFM_FAULT_N_REASONS; i++) {
> int reason = 1 << i;
> - if (fault & reason) {
> + if (status.faults & reason) {
> reasons[j++] = cfm_fault_reason_to_str(reason);
> }
> }
> -
> - ovsrec_interface_set_cfm_fault(cfg, &fault_bool, 1);
> ovsrec_interface_set_cfm_fault_status(cfg, (char **) reasons, j);
> - } else {
> - ovsrec_interface_set_cfm_fault(cfg, NULL, 0);
> - ovsrec_interface_set_cfm_fault_status(cfg, NULL, 0);
> - }
> -
> - opup = ofproto_port_get_cfm_opup(iface->port->bridge->ofproto,
> - iface->ofp_port);
> - if (opup >= 0) {
> - ovsrec_interface_set_cfm_remote_opstate(cfg, opup ? "up" :
> "down");
> - } else {
> - ovsrec_interface_set_cfm_remote_opstate(cfg, NULL);
> - }
>
> - error =
> ofproto_port_get_cfm_remote_mpids(iface->port->bridge->ofproto,
> - iface->ofp_port, &rmps,
> &n_rmps);
> - if (error >= 0) {
> - ovsrec_interface_set_cfm_remote_mpids(cfg, (const int64_t *)rmps,
> - n_rmps);
> - } else {
> - ovsrec_interface_set_cfm_remote_mpids(cfg, NULL, 0);
> - }
> -
> - health = ofproto_port_get_cfm_health(iface->port->bridge->ofproto,
> - iface->ofp_port);
> - if (health >= 0) {
> - int64_t cfm_health = health;
> + ovsrec_interface_set_cfm_remote_opstate(cfg,
> (status.remote_opstate
> + ? "up" : "down"));
> + ovsrec_interface_set_cfm_remote_mpids(cfg,
> + (const int64_t
> *)status.rmps,
> + status.n_rmps);
> ovsrec_interface_set_cfm_health(cfg, &cfm_health, 1);
> - } else {
> - ovsrec_interface_set_cfm_health(cfg, NULL, 0);
> }
> }
>
> --
> 1.7.2.5
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
