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?
> + /* When forward_if_rx is true, the bfd_forwarding() will
s/the bfd_forwarding()/bfd_forwarding()
> + * return true as long as there are incoming packets received.
> + * Note, forwarding_override still has higher priority. */
> + bool forwarding_if_rx;
> + long long int forwarding_if_rx_detect_time;
> +
> /* BFD decay related variables. */
> bool in_decay; /* True when bfd is in decay. */
> int decay_min_rx; /* min_rx is set to decay_min_rx when */
> /* in decay. */
> - int decay_rx_ctrl; /* Count bfd packets received within decay
> */
> + 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.
> @@ -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.
> + <column name="bfd" key="forwarding_if_rx" type='{"type": "boolean"}'>
> + When <code>forwarding_if_rx</code> is true the <code>forwarding
> + </code> field in <code>"ovs-appctl bfd/show"</code> output will
> + still be true as long as there are incoming packets received.
> + This option is for indicating the tunnel liveness when the tunnel
> + becomes congested and consecutive BFD control packets are lost.
> + </column>
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.
Ethan
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
