Thx for the review Ben, please see my reply inline
On Wed, Apr 9, 2014 at 4:13 PM, Ben Pfaff <b...@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 <al...@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);
>
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
