Thanks very much for the review,
On Mon, Aug 12, 2013 at 11:47 PM, Ethan Jackson <et...@nicira.com> wrote:
>
> >>> + /* Always resets decay_min_rx when cfg_min_rx is updated. */
> >>> + bfd->decay_min_rx = 0;
> >>> + bfd_poll(bfd);
>
> I think the intention is right here, but it's a bit odd. For example,
> if in_decay is true, we don't drop out of it with this logic in all
> cases. How about we add a boolean to this function
> "cfm_min_rx_changed" and set it to true here. If that's true, then in
> the next if statement we can run through the decay_min_rx changed
> logic which we know is correct.
I think the reset here is necessary. Since we do not allow "decay_min_rx <
cfg_min_rx", we need to check the validity of "decay_min_rx" every time
"cfg_min_rx" is changed.
> >>> + if (bfd->state == STATE_UP && bfd->decay_min_rx > 0
> >>> + && now >= bfd->decay_detect_time) {
> >>> + bfd_try_decay(bfd);
> >>> + }
>
> I'm not sure the bfd_try_decay() function is giving us a lot. This is
> the only caller and it's fairly short. What do you think about simply
> moving it's logic into this if block? If you feel strongly about it,
> it's fine as is, just a style nit.
>
You point makes sense, but I still think that the current way makes it
cleaner. I'll also ask Ben about it.
> One thing, we should probably reset the decay_detect_time when we
> first enter STATE_UP. Otherwise our time in other states counts to
> the detect time.
>
>
Yes, I'll adjust accordingly,
> >>> - ovs_strlcpy(flag_str, ds_cstr(&ds), sizeof flag_str);
> >>> + /* Do not copy the trailing whitespace. */
> >>> + ovs_strlcpy(flag_str, ds_cstr(&ds), ds.length);
>
> This probably belongs in a different patch. Also have a look at the
> ds_chomp() function which I think is a bit cleaner for this sort of
> thing.
Already made it a separate patch and applied,
> >>> + measure = (bfd->decay_min_rx < 2000 ? 2000 : bfd->decay_min_rx)
> >>> + / bfd->min_rx + 5;
>
> This is awfully complicated. Any particular reason we don't simply do
> something like MAX(bfd->decay_min_rx, 2000)? It certainly would be
> easier to explain to people. If not, could we add a comment
> explaining this formula and how we arrived at it?
>
Thanks for pointing this out, I totally forgot using MAX() function.
Yeah, the logic here is awfully complicated, I'll add comment to explain
it. Basically, the "netdev->rx_packets" gets updated every 2 seconds by the
"type_run()" in ofproto-dpif.c, so "bfd_rx_packets()" can be invoked any
time in the 2-second interval and the resulting "rx_packets" can differ
from the actual value by "2 seconds / min_rx".
Also, I'll change the formula to:
measure = MAX(bfd->decay_min_rx, 2000) / bfd->min_rx
+ MAX(2000 / bfd->cfg_min_rx, 1)
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
