There's a few comments inline.
On 18 April 2014 08:32, Alex Wang <al...@nicira.com> wrote:
> This commit removes the 'Instant' stats related logic in bridge.c.
> Instead, the corresponding status is updated immediately after the
> global connectivity sequence number changes.
>
Could you update the commit message? It has changed a bit since this was
written. In particular, I don't think it "removes" the instant stats logic.
There is refactoring and listening for netdev change_seq.
This change brings the following effects:
>
> 1. There is no change to the database update speed, since both the master
> (in ofproto_wait) and this patch wait on the global connectivity
> sequence
> number.
>
If this is no longer changing, it could be omitted from the commit message.
I'd like to draw attention to the ordering of the if statements on the old
compared to new. This doesn't reflect only a refactoring, there's a minor
logical change too:
> - if (!instant_txn) {
...
> - seq = seq_read(connectivity_seq_get());
> - if (seq == connectivity_seqno) {
> - return;
> - }
>
(create new transaction)
> - }
- status = ovsdb_idl_txn_commit(instant_txn);
> - if (status != TXN_INCOMPLETE) {
> - ovsdb_idl_txn_destroy(instant_txn);
> - instant_txn = NULL;
> - }
> - if (status == TXN_UNCHANGED) {
> - instant_stats_could_have_changed = false;
> - }
> -}
>
So, if the transaction doesn't exist, we create it (if something changed).
But if there is a transaction later in instant_stats_run(), we will try to
commit it regardless of whether something changed recently.
> + /* Check the need to update status. */
> + seq = seq_read(connectivity_seq_get());
> + if (seq != connectivity_seqno) {
> + enum ovsdb_idl_txn_status status;
> +
> + if (!status_txn) {
> + connectivity_seqno = seq;
> + status_txn = ovsdb_idl_txn_create(idl);
> + HMAP_FOR_EACH (br, node, &all_bridges) {
> + struct port *port;
> +
> + br_refresh_stp_status(br);
> + HMAP_FOR_EACH (port, hmap_node, &br->ports) {
> + struct iface *iface;
> +
> + port_refresh_stp_status(port);
> + LIST_FOR_EACH (iface, port_elem, &port->ifaces) {
> + iface_refresh_netdev_status(iface);
> + iface_refresh_ofproto_status(iface);
> + }
> + }
> + }
> + }
> +
> + status = ovsdb_idl_txn_commit(status_txn);
> + /* Do not destroy "status_txn" if the transaction is
> + * "TXN_INCOMPLETE". */
> + if (status != TXN_INCOMPLETE) {
> + ovsdb_idl_txn_destroy(status_txn);
> + status_txn = NULL;
> + }
> + }
> +
>
The new logic checks the connectivity_seqno first. At first glance, this
seems like it will only execute the codeblock if something changes.
However, connectivity_seqno is only updated if a new transaction is made.
This seems a bit misleading, it would be easier to read (and more obviously
following previous behaviour) if the "status_txn" check was at the start of
the code block, and the connectivity_seqno check was nested as it was
previously. I can't see an obvious error with the code, but I think it
would be easier to read if it were the other way around.
Cheers,
Joe
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
