Getting very close, mostly nits at this point.
Quite a few of the comments in the code refer to the old behavior.
Particularly we aren't calculating an average of remote MPs anymore,
and -1 doesn't necessarily mean CFM is disabled (in ofproto).
> +
> + int health; /* Average health over all remote_mps */
This comment needs a period.
> + int health_interval; /* Num of fault_intervals used to compute the
> + * health. */
This would easier to read if it wrote out Number fully instead of just
Num. I actually think this comment isn't accurate anyways. Shouldn't
it say something like "Number of fault intervals since health was
recomputed".
> cfm->remote_opup = true;
> + if (cfm->health_interval == CFM_HEALTH_INTERVAL) {
> + int cfm_health = 0;
> +
> + /* Calculate the cfm health of the interface. If the number of
> + * remote_mpids of a cfm interface is > 1, the cfm health is
> + * undefined. If the number of remote_mpids is 1, the cfm Health
> is
> + * simply the remote_mpid's health, else it is 0. */
> + if (hmap_count(&cfm->remote_mps) > 1) {
> + cfm->health = -1;
> + } else {
> + HMAP_FOR_EACH (rmp, node, &cfm->remote_mps) {
> + int lost, exp_ccm_recvd;
lost is now redundant.
> +
> + exp_ccm_recvd = (CFM_HEALTH_INTERVAL << 2) -
> + (CFM_HEALTH_INTERVAL >> 1);
This would be much easier to read if it's just CFM_HEALTH_INTERVAL * 7 / 2.
> +
> + /* Calculate the percentage of healthy ccm frames
> received.
> + * Since the 'fault_interval' is (3.5 * cfm_interval),
> and
> + * 1 CCM packet must be received every cfm_interval,
> + * the 'remote_mpid' health reports the percentage of
> + * healthy CCM frames received every
> + * 'CFM_HEALTH_INTERVAL'th 'fault_interval'. */
> + rmp->health = (rmp->num_health_ccm * 100) /
> exp_ccm_recvd;
> + rmp->health = MIN(rmp->health, 100);
> + assert(rmp->health >= 0 && rmp->health <= 100);
> + cfm_health += rmp->health;
> + rmp->num_health_ccm = 0;
> + }
> + /* Calculate the cfm health. */
> + cfm->health = hmap_is_empty(&cfm->remote_mps)
> + ? 0 :
> + cfm_health;
> + assert(cfm->health >= 0 && cfm->health <= 100);
> + }
Now that we only support cfm_health when there is one rmp, for
readability, it may make sense to restructure the above code a bit.
Perhaps something like the following (completely untested) diff:
if (cfm->health_interval == CFM_HEALTH_INTERVAL) {
- int cfm_health = 0;
-
/* Calculate the cfm health of the interface. If the number of
* remote_mpids of a cfm interface is > 1, the cfm health is
* undefined. If the number of remote_mpids is 1, the cfm Health is
* simply the remote_mpid's health, else it is 0. */
if (hmap_count(&cfm->remote_mps) > 1) {
cfm->health = -1;
+ } else if (hmap_is_empty(&cfm->remote_mps)) {
+ cfm->health = 0;
} else {
- HMAP_FOR_EACH (rmp, node, &cfm->remote_mps) {
- int lost, exp_ccm_recvd;
-
- exp_ccm_recvd = (CFM_HEALTH_INTERVAL << 2) -
- (CFM_HEALTH_INTERVAL >> 1);
-
- /* Calculate the percentage of healthy ccm frames received.
- * Since the 'fault_interval' is (3.5 * cfm_interval), and
- * 1 CCM packet must be received every cfm_interval,
- * the 'remote_mpid' health reports the percentage of
- * healthy CCM frames received every
- * 'CFM_HEALTH_INTERVAL'th 'fault_interval'. */
- rmp->health = (rmp->num_health_ccm * 100) / exp_ccm_recvd;
- rmp->health = MIN(rmp->health, 100);
- assert(rmp->health >= 0 && rmp->health <= 100);
- cfm_health += rmp->health;
- rmp->num_health_ccm = 0;
- }
- /* Calculate the cfm health. */
- cfm->health = hmap_is_empty(&cfm->remote_mps)
- ? 0 :
- cfm_health;
+ int lost, exp_ccm_recvd;
+
+ rmp = CONTAINER_OF(hmap_first(&cfm->remote_mps),
+ struct remote_mp, node);
+
+
+ exp_ccm_recvd = (CFM_HEALTH_INTERVAL << 2) -
+ (CFM_HEALTH_INTERVAL >> 1);
+
+ /* Calculate the percentage of healthy ccm frames received.
+ * Since the 'fault_interval' is (3.5 * cfm_interval), and
+ * 1 CCM packet must be received every cfm_interval,
+ * the 'remote_mpid' health reports the percentage of
+ * healthy CCM frames received every
+ * 'CFM_HEALTH_INTERVAL'th 'fault_interval'. */
+ rmp->health = (rmp->num_health_ccm * 100) / exp_ccm_recvd;
+ rmp->health = MIN(rmp->health, 100);
+ assert(rmp->health >= 0 && rmp->health <= 100);
+ cfm->health = rmp->health;
+ rmp->num_health_ccm = 0;
+
> + cfm->health_interval = 0;
> + }
> + cfm->health_interval++;
As a nit I'd probably add a new line here.
> HMAP_FOR_EACH_SAFE (rmp, rmp_next, node, &cfm->remote_mps) {
> -
And it's not clear to me why this newline was removed.
Ethan
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
