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

[Paulina Szubarczyk <paulinaszubarczyk@gmail.com>]

On Wed, 22 Jun 2016 10:37:24 +0100
David Vrabel <david.vrabel@citrix.com> wrote:

> 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

As Wei and Ian are maintainers of toolstack if they agree on the previous cast
that was here I will revert the changes.

Paulina

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