[ovs-dev] [PATCH V2 2/4] bfd: Implements forwarding_if_rx

Alex Wang alexw at nicira.com
Fri Aug 16 21:24:02 UTC 2013


Thanks a lot for the comments,


On Tue, Aug 13, 2013 at 12:13 AM, Ethan Jackson <ethan at nicira.com> wrote:

> I think this would be a bit cleaner if we ditched the has_rx flag.
> Instead, each time we go through the run loop, we can check if we've
> received any packets, if we have, we reset to rx_detect_time to now +
> bfd_min_rx * mult.  Then in bfd_forwarding() when forwarding_if_rx is
> enabled, we only have to check if the rx_detect_time has passed or
> not.
>
>

I think there can be jitters. Since we do not update the rx_detect_time at
the exact time instant when it timeout, it is possible that "forwarding"
flag becomes false when "bfd/show" is called at "time_msec() >
rx_detect_time" time.



> > +    if (diff < 0) {
> > +        VLOG_WARN("rx_packets count is smaller than last time.");
> > +    }
> > +    bfd->rx_packets = rx_packets;
> > +    bfd->has_rx = (diff > 0);
> > +    incr = bfd_rx_interval(bfd) * bfd->mult;
> > +    bfd->rx_detect_time = (incr > 2000 ? incr : 2000) + time_msec();
>
> This would be easier to read as MAX(incr, 2000).  Also it deserves a
> comment explaining why we're setting 2000 as the minimum
> rx_detect_time.  That number seems awfully high to me at any rate, I'd
> feel better if we set it to 1000, and made (in a separate patch) made
> ofproto-dpif pull stats from the datapath at least once every 800ms so
> we have time to grab our stats and update the bfd module.



I'll add the comment.

I'll adjust accordingly and send another patch for changing the pull stats
rate in ofproto-dpif.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20130816/429a5ad0/attachment-0003.html>


More information about the dev mailing list