Thanks Ethan for the comments, I'll adjust accordingly,
On Mon, Dec 9, 2013 at 3:38 PM, Ethan Jackson <et...@nicira.com> wrote:
> 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
