On Mon, Dec 9, 2013 at 1:40 PM, Alex Wang <al...@nicira.com> wrote:
> Currently, we updates the forwarding flag in bfd_set_state() and
> in bfd_forwarding_if_rx_update() if bfd_forwarding_if_rx is enabled.
> However, these are not the exact places where the forwarding flag
> needs to be updated. The exact places are in the bfd_process_packet()
> where bfd status are changed based on received control packet, and in
> the flow_push_stats() and compose_output_action__() where the rx_packet
> counter is updated.
s/updates/update
s/status are changed/status is changed
> +/* When forwarding_if_rx is enabled, if there are packets received,
> + * updates forwarding_if_rx_detect_time. */
> +void
> +bfd_check_rx(struct bfd *bfd, const struct dpif_flow_stats *stats)
I think it'd be best if we renamed bfd_check_rx() to something more
descriptive to outside callers. Perhaps bfd_account_rx() similar to
the bond_account() function? If you think of something else is better
that's fine too.
> + ovs_mutex_lock(&mutex);
> +
> + /* Checks if there is a forwarding flag flap since the last
> + * invocation. */
> + bfd_forwarding__(bfd);
> +
> + if (bfd->forwarding_if_rx) {
> + if (stats->n_packets > 0) {
> + bfd_forwarding_if_rx_update(bfd);
> + }
> + bfd_forwarding__(bfd);
> + }
> +
> + ovs_mutex_unlock(&mutex);
> +}
Silly micro optimization, but perhaps it'd be cleaner if we check if
stats->n_packets is zero before taking the mutex and returning if it
is?
Do we really still need the bfd_forwarding_if_rx_update() function?
It's so short and only called in this one place, could we just move
it's code there?
> + /* Checks if there is a forwarding flag flap since the last
> + * invocation. */
> + bfd_forwarding__(bfd);
Perhaps this comment should be removed from the various invocations of
bfd_forwarding__() and instead, we can add a comment to it's
definition explaining that it needs to called to update flaps.
> struct netdev;
> struct ofpbuf;
> struct smap;
> +struct dpif_flow_stats;
Could you add this under struct bfd so we're still in alphabetical order?
I'm happy with this, clean up the minor stuff, resubmit, and I'll merge it.
Thanks,
Ethan
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
