Re: [PATCH v3 3/7] support UFFD write fault processing in ram_save_iterate()

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Andrey Gruzdev (andrey.gruzdev@virtuozzo.com) wrote:
> On 25.11.2020 16:08, Dr. David Alan Gilbert wrote:
> > * Andrey Gruzdev (andrey.gruzdev@virtuozzo.com) wrote:
> > > In this particular implementation the same single migration
> > > thread is responsible for both normal linear dirty page
> > > migration and procesing UFFD page fault events.
> > > 
> > > Processing write faults includes reading UFFD file descriptor,
> > > finding respective RAM block and saving faulting page to
> > > the migration stream. After page has been saved, write protection
> > > can be removed. Since asynchronous version of qemu_put_buffer()
> > > is expected to be used to save pages, we also have to flush
> > > migraion stream prior to un-protecting saved memory range.
> > > 
> > > Write protection is being removed for any previously protected
> > > memory chunk that has hit the migration stream. That's valid
> > > for pages from linear page scan along with write fault pages.
> > > 
> > > Signed-off-by: Andrey Gruzdev <andrey.gruzdev@virtuozzo.com>
> > > ---
> > >   migration/ram.c | 124 ++++++++++++++++++++++++++++++++++++++++++++----
> > >   1 file changed, 115 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/migration/ram.c b/migration/ram.c
> > > index 7f273c9996..08a1d7a252 100644
> > > --- a/migration/ram.c
> > > +++ b/migration/ram.c
> > > @@ -314,6 +314,8 @@ struct RAMState {
> > >       ram_addr_t last_page;
> > >       /* last ram version we have seen */
> > >       uint32_t last_version;
> > > +    /* 'write-tracking' migration is enabled */
> > > +    bool ram_wt_enabled;
> > >       /* We are in the first round */
> > >       bool ram_bulk_stage;
> > >       /* The free page optimization is enabled */
> > > @@ -574,8 +576,6 @@ static int uffd_protect_memory(int uffd, hwaddr start, hwaddr length, bool wp)
> > >       return 0;
> > >   }
> > > -__attribute__ ((unused))
> > > -static int uffd_read_events(int uffd, struct uffd_msg *msgs, int count);
> > >   __attribute__ ((unused))
> > >   static bool uffd_poll_events(int uffd, int tmo);
> > > @@ -1929,6 +1929,86 @@ static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss,
> > >       return pages;
> > >   }
> > > +/**
> > > + * ram_find_block_by_host_address: find RAM block containing host page
> > > + *
> > > + * Returns true if RAM block is found and pss->block/page are
> > > + * pointing to the given host page, false in case of an error
> > > + *
> > > + * @rs: current RAM state
> > > + * @pss: page-search-status structure
> > > + */
> > > +static bool ram_find_block_by_host_address(RAMState *rs, PageSearchStatus *pss,
> > > +        hwaddr page_address)
> > > +{
> > > +    bool found = false;
> > > +
> > > +    pss->block = rs->last_seen_block;
> > > +    do {
> > > +        if (page_address >= (hwaddr) pss->block->host &&
> > > +            (page_address + TARGET_PAGE_SIZE) <=
> > > +                    ((hwaddr) pss->block->host + pss->block->used_length)) {
> > > +            pss->page = (unsigned long)
> > > +                    ((page_address - (hwaddr) pss->block->host) >> TARGET_PAGE_BITS);
> > > +            found = true;
> > > +            break;
> > > +        }
> > > +
> > > +        pss->block = QLIST_NEXT_RCU(pss->block, next);
> > > +        if (!pss->block) {
> > > +            /* Hit the end of the list */
> > > +            pss->block = QLIST_FIRST_RCU(&ram_list.blocks);
> > > +        }
> > > +    } while (pss->block != rs->last_seen_block);
> > > +
> > > +    rs->last_seen_block = pss->block;
> > > +    /*
> > > +     * Since we are in the same loop with ram_find_and_save_block(),
> > > +     * need to reset pss->complete_round after switching to
> > > +     * other block/page in pss.
> > > +     */
> > > +    pss->complete_round = false;
> > > +
> > > +    return found;
> > > +}
> > > +
> > > +/**
> > > + * get_fault_page: try to get next UFFD write fault page and, if pending fault
> > > + *   is found, put info about RAM block and block page into pss structure
> > > + *
> > > + * Returns true in case of UFFD write fault detected, false otherwise
> > > + *
> > > + * @rs: current RAM state
> > > + * @pss: page-search-status structure
> > > + *
> > > + */
> > > +static bool get_fault_page(RAMState *rs, PageSearchStatus *pss)
> > > +{
> > > +    struct uffd_msg uffd_msg;
> > > +    hwaddr page_address;
> > > +    int res;
> > > +
> > > +    if (!rs->ram_wt_enabled) {
> > > +        return false;
> > > +    }
> > > +
> > > +    res = uffd_read_events(rs->uffdio_fd, &uffd_msg, 1);
> > > +    if (res <= 0) {
> > > +        return false;
> > > +    }
> > > +
> > > +    page_address = uffd_msg.arg.pagefault.address;
> > > +    if (!ram_find_block_by_host_address(rs, pss, page_address)) {
> > > +        /* In case we couldn't find respective block, just unprotect faulting page */
> > > +        uffd_protect_memory(rs->uffdio_fd, page_address, TARGET_PAGE_SIZE, false);
> > > +        error_report("ram_find_block_by_host_address() failed: address=0x%0lx",
> > > +                     page_address);
> > > +        return false;
> > > +    }
> > > +
> > > +    return true;
> > > +}
> > > +
> > >   /**
> > >    * ram_find_and_save_block: finds a dirty page and sends it to f
> > >    *
> > > @@ -1955,25 +2035,50 @@ static int ram_find_and_save_block(RAMState *rs, bool last_stage)
> > >           return pages;
> > >       }
> > > +    if (!rs->last_seen_block) {
> > > +        rs->last_seen_block = QLIST_FIRST_RCU(&ram_list.blocks);
> > > +    }
> > >       pss.block = rs->last_seen_block;
> > >       pss.page = rs->last_page;
> > >       pss.complete_round = false;
> > > -    if (!pss.block) {
> > > -        pss.block = QLIST_FIRST_RCU(&ram_list.blocks);
> > > -    }
> > > -
> > >       do {
> > > +        ram_addr_t page;
> > > +        ram_addr_t page_to;
> > > +
> > >           again = true;
> > > -        found = get_queued_page(rs, &pss);
> > > -
> > > +        /* In case of 'write-tracking' migration we first try
> > > +         * to poll UFFD and get write page fault event */
> > > +        found = get_fault_page(rs, &pss);
> > > +        if (!found) {
> > > +            /* No trying to fetch something from the priority queue */
> > > +            found = get_queued_page(rs, &pss);
> > > +        }
> > >           if (!found) {
> > >               /* priority queue empty, so just search for something dirty */
> > >               found = find_dirty_block(rs, &pss, &again);
> > >           }
> > >           if (found) {
> > > +            page = pss.page;
> > >               pages = ram_save_host_page(rs, &pss, last_stage);
> > > +            page_to = pss.page;
> > > +
> > > +            /* Check if page is from UFFD-managed region */
> > > +            if (pss.block->flags & RAM_UF_WRITEPROTECT) {
> > > +                hwaddr page_address = (hwaddr) pss.block->host +
> > > +                        ((hwaddr) page << TARGET_PAGE_BITS);
> > > +                hwaddr run_length = (hwaddr) (page_to - page + 1) << TARGET_PAGE_BITS;
> > > +                int res;
> > > +
> > > +                /* Flush async buffers before un-protect */
> > > +                qemu_fflush(rs->f);
> > > +                /* Un-protect memory range */
> > > +                res = uffd_protect_memory(rs->uffdio_fd, page_address, run_length, false);
> > > +                if (res < 0) {
> > > +                    break;
> > > +                }
> > > +            }
> > 
> > Please separate that out into a separate function.
> > 
> > Dave
> > 
> 
> You mean separate implementation of ram_find_and_save_block()?

No, I mean take that if (...WRITEPROTECT) { ..} block and put it in a
function and call it from there, just to keep ram_find_and_save_block
clean.

Dave

> Andrey
> 
> > >           }
> > >       } while (!pages && again);
> > > @@ -2086,7 +2191,8 @@ static void ram_state_reset(RAMState *rs)
> > >       rs->last_sent_block = NULL;
> > >       rs->last_page = 0;
> > >       rs->last_version = ram_list.version;
> > > -    rs->ram_bulk_stage = true;
> > > +    rs->ram_wt_enabled = migrate_track_writes_ram();
> > > +    rs->ram_bulk_stage = !rs->ram_wt_enabled;
> > >       rs->fpo_enabled = false;
> > >   }
> > > -- 
> > > 2.25.1
> > > 
> 
> 
> -- 
> Andrey Gruzdev, Principal Engineer
> Virtuozzo GmbH  +7-903-247-6397
>                 virtuzzo.com
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK