Re: [PATCH v3 7/8] migration: Unexport migration_is_active()

[Avihai Horon <avihaih@nvidia.com>]

On 28/10/2024 21:06, Peter Xu wrote:
> External email: Use caution opening links or attachments
>
>
> On Mon, Oct 28, 2024 at 07:20:27PM +0200, Avihai Horon wrote:
>> On 28/10/2024 18:58, Peter Xu wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> On Mon, Oct 28, 2024 at 06:41:42PM +0200, Avihai Horon wrote:
>>>> On 28/10/2024 17:45, Peter Xu wrote:
>>>>> External email: Use caution opening links or attachments
>>>>>
>>>>>
>>>>> On Mon, Oct 28, 2024 at 09:43:16AM +0200, Avihai Horon wrote:
>>>>>> On 25/10/2024 0:30, Peter Xu wrote:
>>>>>>> External email: Use caution opening links or attachments
>>>>>>>
>>>>>>>
>>>>>>> We have two outside users of this API, so it's exported.
>>>>>>>
>>>>>>> Is it really necessary?  Does it matter whether it must be
>>>>>>> ACTIVE/POSTCOPY_ACTIVE/DEVICE?  I guess no.
>>>>>> Actually for VFIO it does matter, because we don't want VFIO to do DPT
>>>>>> log_sync in SETUP stage when DPT might not have been started yet.
>>>>>> See commit ff180c6bd7a8 ("vfio/migration: Skip log_sync during migration
>>>>>> SETUP state").
>>>>> This seems to be a known issue for migration in general, rather than VFIO
>>>>> specific.  Hyman has a patch for it, not yet reviewed..
>>>>>
>>>>> https://lore.kernel.org/r/cover.1729648695.git.yong.huang@smartx.com
>>>>>
>>>>> That corresponds to your comment here:
>>>>>
>>>>>        Redundant -- all RAM is marked dirty in migration SETUP state and is
>>>>>        transferred only after migration is set to ACTIVE state, so doing
>>>>>        log_sync during migration SETUP is pointless.
>>>>>
>>>>> So I wonder whether it's only VFIO that should skip it, or log_sync()
>>>>> simply shouldn't be called at all during SETUP, because of its redundancy.
>>>> Not sure why this sync was there in the first place, but if its only purpose
>>>> was to sync dirty pages then yes, I guess it be dropped.
>>>>
>>>>> The other thing you mentioned here:
>>>>>
>>>>>        Can fail -- there is a time window, between setting migration state to
>>>>>        SETUP and starting dirty tracking by RAM save_live_setup handler, during
>>>>>        which dirty tracking is still not started. Any VFIO log_sync call that
>>>>>        is issued during this time window will fail. For example, this error can
>>>>>        be triggered by migrating a VM when a GUI is active, which constantly
>>>>>        calls log_sync.
>>>>>
>>>>> This is VFIO specific.  Why this can fail even if global tracking is
>>>>> started already?
>>>> It can fail if global tracking is *not* started yet.
>>>> As mentioned in the commit message, there is a time window where migration
>>>> is in SETUP state but global tracking is not started yet.
>>> Hmm, I'm totally confused now..
>>>
>>> The only thing that can kickoff the sync during SETUP, AFAICT, is:
>>>
>>>               ret = memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION, errp);
>>>               if (!ret) {
>>>                   goto out_unlock;
>>>               }
>>>               migration_bitmap_sync_precopy(false);   <------------- here
>>>
>>> I need to confess this may not be the right place to invoke it in ram.c (I
>>> think we probably should move it out at some point.. into generic migration
>>> code).  However I don't yet see why log_start() is not called first in your
>>> case before log_sync().
>>>
>>> Could you elaborate?
>> Indeed, in the above code log_start is called before log_sync.
>>
>> I was referring to the case where some other code path triggers log_sync.
>> E.g., if you open a VNC to the guest then it constantly calls log_sync to
>> refresh the graphics. In that case, one of these log_syncs can happen
>> between "migration status is set to SETUP" and "global tracking is started".
> I see.  That's unfortunate..
>
> Though this is also the case where it shouldn't be VFIO's problem alone.
> See some other users of log_sync():
>
> vhost_sync_dirty_bitmap():
>      if (!dev->log_enabled || !dev->started) {
>          return 0;
>      }
>
> kvm_slot_get_dirty_log():
>      if (ret == -ENOENT) {
>          /* kernel does not have dirty bitmap in this slot */
>          ret = 0;
>      }
>
> And I didn't further look.
>
> In short, IMHO looks like VFIO still shouldn't be special on differeciating
> and make migration export the SETUP phase just for this..  as VFIO has
> log_start() like all the rest, so VFIO can also know whether tracking is
> enabled at all, then it can silently no-op the log_sync() like all the rest
> of the users.
>
> If you agree, I'd prefer we keep this patch - it'll be nice we only ever
> expose migration_is_running() for migration status checks, without exposing
> SETUP only for this VFIO use case even if it could have followed what other
> modules are doing.

Yes, I agree this could be a nice cleanup.

>
> If you would like to propose a patch for VFIO, I'd be happy to include your
> patch before this patch (just in case this patch could land some day) to
> make sure VFIO works as before.  Since I don't have VFIO HW to test, it'll
> be challenging for me to propose and test such patch otherwise.

I can do that, though it may be a bit involved because VFIO has multiple 
dirty tracking mechanisms (legacy, device DPT and IOMMU DPT). Plus, I 
don't have HW that supports IOMMU DPT at hand for testing.
I assume this is not an urgent cleanup, right?

Thanks.