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 <[email protected]>
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.
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.
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:
+ 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
[email protected]
http://openvswitch.org/mailman/listinfo/dev
