[ovs-dev] [PATCH 5/8] bridge: Remove the 'Instant' stats.

Alex Wang alexw at nicira.com
Wed Apr 9 23:31:46 UTC 2014


Thx for the review Ben, please see my reply inline


On Wed, Apr 9, 2014 at 4:13 PM, Ben Pfaff <blp at nicira.com> wrote:

> On Fri, Apr 04, 2014 at 03:00:40PM -0700, Alex Wang wrote:
> > This commit removes the "Instant" stats related logic in bridge.c
> > by moving the logic into iface_refresh_ofproto_status() and using
> > the global sequence number to check whether or not to call it.
> >
> > Also, the 'netdev''s 'change_seq' is used to skip the update if the
> > interface's netdev status is not changed since last update.
> >
> > Signed-off-by: Alex Wang <alexw at nicira.com>
>
> The commit message is too low-level, in my opinion, because it doesn't
> explain the user-visible effects.  Does this mean, for example, that
> the user will see slower database updates?  I'm not really sure, since
> it seems to talk more about moving code around.
>


Thanks for pointing out it,
It will not cause a slower update.  And I will fix the message to explain
the
user-visible effects.




> There's a comment in iface_refresh_netdev_status() that I don't quite
> understand:
>
> > +    if (iface->change_seq == netdev_get_change_seq(iface->netdev)) {
> > +        return;
> > +    }
> > +
> > +    /* Should not worry any race.  Since only main thread can change the
> > +     * 'netdev''s change_seq. */
> > +    iface->change_seq = netdev_get_change_seq(iface->netdev);
>
> It might be true (I did not check) that currently only actions taken
> by the main thread can cause netdev change sequence numbers to change.
> But that's not a requirement.  One can imagine, for example, some type
> of netdev for which the best implementation is to have a thread
> specialized for waiting for status changes and, when they occur,
> incrementing the netdev's change sequence number.
>
> But I don't think that this code even cares whether that's the case:
> it only actually reads the netdev's status *after* it reads the
> sequence number, which means that any change after this code should,
> at worst, get picked up on the next call to the function.  So I don't
> think there's a race here at all, even if other threads can change the
> change_seq.
>


Thx for the explanation, I agree, the comment is invalid.  Will remove it.



> I also don't quite follow this comment in
> iface_refresh_ofproto_status(), because it talks about a change to
> status but actually checks for an error fetching status:
>
>

I'll change the variable name.



>  +    smap_init(&smap);
> +    error = ofproto_port_get_bfd_status(iface->port->bridge->ofproto,
> +                                        iface->ofp_port, &smap);
> +    /* No need to do the following work if there is no change to status.
> */
> +    if (error < 0) {
> +        return;
> +    }
> +    ovsrec_interface_set_bfd_status(iface->cfg, &smap);
> +    smap_destroy(&smap);
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20140409/456831ad/attachment-0005.html>


More information about the dev mailing list