Re: [PATCH v3] dm_op: Add xendevicemodel_modified_memory_bulk.

["Jan Beulich" <JBeulich@suse.com>]

>>> On 28.03.17 at 15:18, <jennifer.herbert@citrix.com> wrote:
> --- a/xen/arch/x86/hvm/dm.c
> +++ b/xen/arch/x86/hvm/dm.c
> @@ -119,56 +119,96 @@ static int set_isa_irq_level(struct domain *d, uint8_t isa_irq,
>  }
>  
>  static int modified_memory(struct domain *d,
> -                           struct xen_dm_op_modified_memory *data)
> +                           struct xen_dm_op_modified_memory *header,
> +                           xen_dm_op_buf_t* buf)
>  {
> -    xen_pfn_t last_pfn = data->first_pfn + data->nr - 1;
> -    unsigned int iter = 0;
> -    int rc = 0;
> -
> -    if ( (data->first_pfn > last_pfn) ||
> -         (last_pfn > domain_get_maximum_gpfn(d)) )
> -        return -EINVAL;
> +    /* Process maximum of 256 pfns before checking for continuation */
> +    const unsigned int cont_check_interval = 0x100;
> +    unsigned int rem_extents =  header->nr_extents;
> +    unsigned int batch_rem_pfns = cont_check_interval;
>  
>      if ( !paging_mode_log_dirty(d) )
>          return 0;
>  
> -    while ( iter < data->nr )
> +    if ( (buf->size / sizeof(struct xen_dm_op_modified_memory_extent)) <
> +         rem_extents )
> +        return -EINVAL;

I'm sorry for noticing this only now, but I think this together with the
open coded copy below calls for a copy_buf_from_guest_offset()
helper.

> +    while ( rem_extents > 0)
>      {
> -        unsigned long pfn = data->first_pfn + iter;
> -        struct page_info *page;
> +        struct xen_dm_op_modified_memory_extent extent;
> +        unsigned int batch_nr;
> +        xen_pfn_t pfn;
> +        xen_pfn_t end_pfn;
> +        unsigned int *pfns_already_done;

Perhaps drop "already"? Personally I also wouldn't mind you
dropping the variable altogether and using header->opaque
directly, but I guess that's too "opaque" for your taste?

> -        page = get_page_from_gfn(d, pfn, NULL, P2M_UNSHARE);
> -        if ( page )
> +        if ( copy_from_guest_offset(&extent, buf->h, rem_extents - 1, 1) )
> +            return -EFAULT;
> +        /*
> +         * In the case of continuation, header->opaque contains the
> +         * number of pfns already processed for this extent
> +         */
> +        pfns_already_done = &header->opaque;

If you want to keep the variable, this should be moved out of the
loop (as being loop invariant).

> +        if (*pfns_already_done >= extent.nr || extent.pad)
> +            return -EINVAL;
> +
> +        pfn = extent.first_pfn + *pfns_already_done;
> +        batch_nr = extent.nr - *pfns_already_done;
> +
> +        if ( batch_nr > batch_rem_pfns )
>          {
> -            mfn_t gmfn = _mfn(page_to_mfn(page));
> -
> -            paging_mark_dirty(d, gmfn);
> -            /*
> -             * These are most probably not page tables any more
> -             * don't take a long time and don't die either.
> -             */
> -            sh_remove_shadows(d, gmfn, 1, 0);
> -            put_page(page);
> +           batch_nr = batch_rem_pfns;
> +           *pfns_already_done += batch_nr;
> +        }
> +        else
> +        {
> +            rem_extents--;
> +            *pfns_already_done = 0;
>          }
>  
> -        iter++;
> +        batch_rem_pfns -= batch_nr;
> +        end_pfn = pfn + batch_nr;
> +
> +        if ( (pfn >= end_pfn) ||
> +             (end_pfn > domain_get_maximum_gpfn(d)) )
> +            return -EINVAL;

I think these checks should be done of the extent as a whole, not
just the portion you mean to process in this batch.

> +        header->nr_extents = rem_extents;
> +
> +        while ( pfn < end_pfn )
> +        {
> +            struct page_info *page;
> +            page = get_page_from_gfn(d, pfn, NULL, P2M_UNSHARE);

Either make this the initializer, or have a blank line between
declaration and statements.

> +            if ( page )
> +            {
> +                mfn_t gmfn = _mfn(page_to_mfn(page));
> +
> +                paging_mark_dirty(d, gmfn);
> +                /*
> +                 * These are most probably not page tables any more
> +                 * don't take a long time and don't die either.
> +                 */
> +                sh_remove_shadows(d, gmfn, 1, 0);
> +                put_page(page);
> +            }
> +            pfn++;
> +        }

Would you mind bringing this in for() form?

>          /*
> -         * Check for continuation every 256th iteration and if the
> -         * iteration is not the last.
> +         * Check for continuation every 256th pfn and if the
> +         * pfn is not the last.
>           */
> -        if ( (iter < data->nr) && ((iter & 0xff) == 0) &&
> -             hypercall_preempt_check() )
> +        if ( (batch_rem_pfns == 0) && (rem_extents > 0) )

Looking at this again I think it would be best to drop the mention
of 256 here, instead talking about a fully handled batch or some
such.

> @@ -441,13 +481,8 @@ static int dm_op(domid_t domid,
>          struct xen_dm_op_modified_memory *data =
>              &op.u.modified_memory;
>  
> -        const_op = false;
> -
> -        rc = -EINVAL;
> -        if ( data->pad )
> -            break;
> -
> -        rc = modified_memory(d, data);
> +        rc = modified_memory(d, data, &bufs[1]);
> +        const_op = (rc != 0);

Isn't this wrong now, i.e. don't you need to copy back the
header now in all cases?

> --- a/xen/include/public/hvm/dm_op.h
> +++ b/xen/include/public/hvm/dm_op.h
> @@ -237,13 +237,29 @@ struct xen_dm_op_set_pci_link_route {
>   * XEN_DMOP_modified_memory: Notify that a set of pages were modified by
>   *                           an emulator.
>   *
> - * NOTE: In the event of a continuation, the @first_pfn is set to the
> - *       value of the pfn of the remaining set of pages and @nr reduced
> - *       to the size of the remaining set.
> + * DMOP buf 1 contains an array of xen_dm_op_modified_memory_extent with
> + * @nr_extents entries.
> + * @opaque must be initially set to 0.
> + *
> + * On error, @nr_extents will contain the index+1 of the extent that
> + * had the error.  It is not defined if or which pages may have been
> + * marked as dirty, in this event.
> + *
> + * @opaque must be initially set to 0.

Please say so just once.

>  struct xen_dm_op_modified_memory {
> +    /*
> +     * IN - Number of extents to be processed
> +     * OUT -returns n+1 for failing extent
> +     */
> +    uint32_t nr_extents;

This n+1 thing is somewhat odd, but I guess it can't be prevented
without going through extra hoops.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel