Thanks for the review, Joe
Replies 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.
>
Yeah, I will do that,
> 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'll discard it.
>
> 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.
>
Please see my next reply,
> + 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
>
Thanks for point this out. Your suggestions definitely improves the
comprehensibility.
As discussed offline, I'll make two adjustments:
1. check the instant_txn non-null first and then check the change_seq, as
was in the
instant_stats_run().
2. I'll move the ovsdb_idl_txn_commit() outside to the bridge_run(). And
as long as instant_txn
is non-null, we will call it to commit.
Thanks,
Alex Wang,
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
