On Friday, July 21, 2023 4:38 AM, Peter Xu wrote:
> Looks good to me, after addressing Isaku's comments.
>
> The current_active_state is very unfortunate, along with most of the calls to
> migrate_set_state() - I bet most of the code will definitely go wrong if that
> cmpxchg didn't succeed inside of migrate_set_state(), IOW in most cases we
> simply always want:
Can you share examples where it could be wrong?
(If it has bugs, we need to fix)
>
> migrate_set_state(&s->state, s->state, XXX);
>
> Not sure whether one pre-requisite patch is good to have so we can rename
> migrate_set_state() to something like __migrate_set_state(), then:
>
> migrate_set_state(s, XXX) {
> __migrate_set_state(&s->state, s->state, XXX);
> }
>
> I don't even know whether there's any call site that will need
> __migrate_set_state() for real..
>
Seems this would break the use of "MIGRATION_STATUS_CANCELLING".
For example,
- In migration_maybe_pause:
migrate_set_state(&s->state, MIGRATION_STATUS_PRE_SWITCHOVER,
new_state);
If the current s->state isn't MIGRATION_STATUS_PRE_SWITCHOVER (could
be MIGRATION_STATUS_CANCELLING), then s->state won’t be updated to
new_state.
- Then, in migration_completion, the following update to s->state won't succeed:
migrate_set_state(&s->state, current_active_state,
MIGRATION_STATUS_COMPLETED);
- Finally, when reaching migration_iteration_finish(), s->state is
MIGRATION_STATUS_CANCELLING, instead of MIGRATION_STATUS_COMPLETED.
