Re: [PATCH V4 07/11] tests/qtest: migration events

[Steven Sistare]

On 11/13/2023 1:33 PM, Steven Sistare wrote:
> On 8/30/2023 1:00 PM, Peter Xu wrote:
>> On Tue, Aug 29, 2023 at 11:18:02AM -0700, Steve Sistare wrote:
[...]
>>> +/*
>>> + * Wait for two changes in the migration pass count, but bail if we stop.
>>> + */
>>>  static void wait_for_migration_pass(QTestState *who)
>>>  {
>>> -    uint64_t initial_pass = get_migration_pass(who);
>>> -    uint64_t pass;
>>> +    uint64_t pass, prev_pass = 0, changes = 0;
>>>  
>>> -    /* Wait for the 1st sync */
>>> -    while (!got_src_stop && !initial_pass) {
>>> -        usleep(1000);
>>> -        initial_pass = get_migration_pass(who);
>>> -    }
>>> -
>>> -    do {
>>> +    while (changes < 2 && !src_state.stop_seen) {
>>>          usleep(1000);
>>>          pass = get_migration_pass(who);
>>> -    } while (pass == initial_pass && !got_src_stop);
>>> +        changes += (pass != prev_pass);
>>> +        prev_pass = pass;
>>> +    }
>>
>> Here won't it start to wait for 2 iterations every time instead of 1?
>>
>> Note that previously we only wait for 1 iteration as long as not the
>> initial pass.  
> 
> I don't think so.  Both the old and new code require at least a transition 
> from
> pass 0 to 1, and pass 1 to 2, to return.  With the old:
>   when initial_pass becomes non-zero, done with first loop
>   when pass changes again, done with 2nd loop

OK, you refer to count of iterations that call usleep(1000), not pass count.
Yes, the new code may call usleep(1000) twice instead of once.
args->iterations at the caller is at most 2, so this is a tiny difference.
However, I could improve it like so:

static void wait_for_migration_pass(QTestState *who)
{
    uint64_t pass = get_migration_pass(who);
    uint64_t changes = (pass > 0);
    uint64_t prev_pass;

    while (changes < 2 && !src_state.stop_seen && !src_state.suspend_seen) {
        usleep(1000);
        prev_pass = pass;
        pass = get_migration_pass(who);
        changes += (pass != prev_pass);
    }
}

- Steve

>> And I think the change will double the counts for below..
>>
>>             while (args->iterations > 1) {
>>                 wait_for_migration_pass(from);
>>                 args->iterations--;
>>             }