Re: [Xen-devel] [PATCH v3 1/2] Interface for grant copy operation in libs.

[David Vrabel <david.vrabel@xxxxxxxxxx>]

On 22/06/16 09:38, Paulina Szubarczyk wrote:
> In a linux part an ioctl(gntdev, IOCTL_GNTDEV_GRANT_COPY, ..)
> system call is invoked. In mini-os the operation is yet not
> implemented. For other OSs there is a dummy implementation.
[...]
> --- a/tools/libs/gnttab/linux.c
> +++ b/tools/libs/gnttab/linux.c
> @@ -235,6 +235,51 @@ int osdep_gnttab_unmap(xengnttab_handle *xgt,
>      return 0;
>  }
>  
> +int osdep_gnttab_grant_copy(xengnttab_handle *xgt,
> +                            uint32_t count,
> +                            xengnttab_grant_copy_segment_t *segs)
> +{
> +    int i, rc;
> +    int fd = xgt->fd;
> +    struct ioctl_gntdev_grant_copy copy;
> +
> +    copy.segments = calloc(count, sizeof(struct 
> ioctl_gntdev_grant_copy_segment));
> +    copy.count = count;
> +    for (i = 0; i < count; i++)
> +    {
> +        copy.segments[i].flags = segs[i].flags;
> +        copy.segments[i].len = segs[i].len;
> +        if (segs[i].flags == GNTCOPY_dest_gref) 
> +        {
> +            copy.segments[i].dest.foreign.ref = segs[i].dest.foreign.ref;
> +            copy.segments[i].dest.foreign.domid = segs[i].dest.foreign.domid;
> +            copy.segments[i].dest.foreign.offset = 
> segs[i].dest.foreign.offset;
> +            copy.segments[i].source.virt = segs[i].source.virt;
> +        } 
> +        else 
> +        {
> +            copy.segments[i].source.foreign.ref = segs[i].source.foreign.ref;
> +            copy.segments[i].source.foreign.domid = 
> segs[i].source.foreign.domid;
> +            copy.segments[i].source.foreign.offset = 
> segs[i].source.foreign.offset;
> +            copy.segments[i].dest.virt = segs[i].dest.virt;
> +        }
> +    }
> +
> +    rc = ioctl(fd, IOCTL_GNTDEV_GRANT_COPY, &copy);
> +    if (rc) 
> +    {
> +        GTERROR(xgt->logger, "ioctl GRANT COPY failed %d ", errno);
> +    }
> +    else 
> +    {
> +        for (i = 0; i < count; i++)
> +            segs[i].status = copy.segments[i].status;
> +    }
> +
> +    free(copy.segments);
> +    return rc;
> +}

I know Wei asked for this but you've replaced what should be a single
pointer assignment with a memory allocation and two loops over all the
segments.

This is a hot path and the two structures (the libxengnttab one and the
Linux kernel one) are both part of their respective ABIs and won't
change so Wei's concern that they might change in the future is unfounded.

This change makes xengnttab_grant_copy() useless for our (XenServer's)
use case.

David

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