Thanks Ethan for the review,
On Tue, Aug 20, 2013 at 3:07 PM, Ethan Jackson <et...@nicira.com> wrote:
> Does this patch work while decay mode is on? Won't the fact that we
> share rx_packets between the two features, and that it's set so
> frequently by the forwarding_if_rx feature, break the decay mode
> feature's assumption that it's the only one setting it? I'm surprised
> this doesn't break the unit tests. Perhaps we should add one that
> runs decay mode and forwarding_if_rx in conjunctin?
I don't understand. I have used a separate decay_rx_packets to record the
rx_packets stats for bfd decay. So, rx_packets is not shared.
But I'll definitely add an unit test running the two together.
> > + uint64_t decay_rx_ctrl; /* Count bfd packets received within
> decay */
> > /* detect interval. */
> > + uint64_t decay_rx_packets; /* Packets received by 'netdev'. */
>
> decay_rx_packets is unused. Also, if changing the type of
> decay_rx_ctl is necessary, it should probably be in a separate patch.
>
>
This is a typo. It is not necessary to change the type of decay_rx_ctl.
Also, I think I use the decay_rx_packets in the bfd_try_decay() and
bfd_decay_update().
>
> > @@ -753,6 +776,7 @@ bfd_set_netdev(struct bfd *bfd, const struct netdev
> *netdev)
> > if (bfd->decay_min_rx) {
> > bfd_decay_update(bfd);
> > }
> > + bfd->rx_packets = bfd_rx_packets(bfd);
>
> we should probably reset the detect time here as well.
>
>
> > + if (diff < 0) {
> > + VLOG_WARN("rx_packets count is smaller than last time.");
>
> It's probably worth rate limiting this log message. I suspect if it
> happens it might happen frequently.
>
>
I'll adjust accordingly. Thanks,
>
I think the forwarding field in the database bfd_status column is far
> more important than that in the appctl output which is mostly for
> developers to debug things. I'd rewrite this and the commit message
> to mention that instead. Also both here and in the comment message, I
> would talk about "interfaces" instead of "tunnels". BFD can be run
> on any kind of interface, so there's no reason to restrict it in the
> documentation.
>
>
Thanks a lot, I agree (especially after I know how it will affect the
bundle ports). I'll redo them accordingly.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
