Re: [PATCH RFC] migrate/ram: remove "ram_bulk_stage" and "fpo_enabled"

[David Hildenbrand <david@redhat.com>]

On 10.05.21 20:58, Dr. David Alan Gilbert wrote:
> * David Hildenbrand (david@redhat.com) wrote:
>> The bulk stage is kind of weird: migration_bitmap_find_dirty() will
>> indicate a dirty page, however, ram_save_host_page() will never save it, as
>> migration_bitmap_clear_dirty() detects that it is not dirty.
>>
>> We already fill the bitmap in ram_list_init_bitmaps() with ones, marking
>> everything dirty - it didn't used to be that way, which is why we needed
>> an explicit first bulk stage.
>>
>> Let's simplify: make the bitmap the single source of thuth. Explicitly
>> handle the "xbzrle_enabled after first round" case.
> 
> I think you're right here, so (at long last) queued.

Thanks!

> I did read the comments on 6eeb63f which added the FPO flag, and I still
> think you're right.
> 
>> Regarding XBZRLE (implicitly handled via "ram_bulk_stage = false" right
>> now), there is now a slight change in behavior:
>> - Colo: When starting, it will be disabled (was implicitly enabled)
>>    until the first round actually finishes.
> 
> Was it or did they see a bulk stage again?

  static void colo_init_ram_state(void)
  {
      ram_state_init(&ram_state);
-    ram_state->ram_bulk_stage = false;
  }

suggests that they wanted to things out of order (or at least onsider 
the bitmap with eventual holes) right away, which implicitly unlocked 
xbzrle. Not sure if that behavior was really intended (don't think so).

> I can imagine that XBZRLE might be useful for COLO if subsequent rounds
> of synchornisation finds pages that change but not much.

Right, I assume in the first round they don't really care.

> 
>> - Free page hinting: When starting, XBZRLE will be disabled (was implicitly
>>    enabled) until the first round actually finished.
> 
> But the XBZRLE cache would be empty anyway on that first round?

Yes; it's less of a concern just something that changed (most probably 
to the good :) ).

> 
>> - Snapshots: When starting, XBZRLE will be disabled. We essentially only
>>    do a single run, so I guess it will never actually get disabled.
>>
>> Postcopy seems to indirectly disable it in ram_save_page(), so there
>> shouldn't be really any change.
>>
>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>> Cc: Juan Quintela <quintela@redhat.com>
>> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>> Cc: Andrey Gruzdev <andrey.gruzdev@virtuozzo.com>
>> Cc: Peter Xu <peterx@redhat.com>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>
>> Our dirty bitmap handling is right now a little confusing due to the bulk
>> stage. Am i missing something important? Can someone comment on the
>> expected XBZRLE handling? It all is a little bit too intertwined for my
>> taste.
> 
> I think it's mostly due to there being no benefit (and a lot of cost) in
> doing xbzrle during the first round.

That makes sense, thanks!


-- 
Thanks,

David / dhildenb