| Message ID | 20220706062759.24946-4-nicolinc@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Update vfio_pin/unpin_pages API | expand |
On Tue, Jul 05, 2022 at 11:27:53PM -0700, Nicolin Chen wrote: > This driver is the only caller of vfio_pin/unpin_pages that might pass > in a non-contiguous PFN list, but in many cases it has a contiguous PFN > list to process. So letting VFIO API handle a non-contiguous PFN list > is actually counterproductive. > > Add a pair of simple loops to pass in contiguous PFNs only, to have an > efficient implementation in VFIO. > > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> > --- > drivers/s390/cio/vfio_ccw_cp.c | 70 +++++++++++++++++++++++++++------- > 1 file changed, 56 insertions(+), 14 deletions(-) I think this is fine as-is for this series, but someone who knows and can test ccw should go in and fix things so that pfn_array_alloc() doesn't exist. Allocating memory and filling it with consecutive integers is kind of silly given we can just call vfio_pin_pages() with pa_nr directly. pa->pa_iova_pfn[0] = pa->pa_iova >> PAGE_SHIFT; pa->pa_pfn[0] = -1ULL; for (i = 1; i < pa->pa_nr; i++) { pa->pa_iova_pfn[i] = pa->pa_iova_pfn[i - 1] + 1; It looks like only the 'ccw_is_idal' flow can actually create non-continuities. Also the loop in copy_from_iova() should ideally be using the much faster 'rw' interface, and not a pin/unpin cycle just to memcpy. If I guess right these changes would significantly speed this driver up. Anyhow, Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> Jason
On Tue, 2022-07-05 at 23:27 -0700, Nicolin Chen wrote: > This driver is the only caller of vfio_pin/unpin_pages that might > pass > in a non-contiguous PFN list, but in many cases it has a contiguous > PFN > list to process. So letting VFIO API handle a non-contiguous PFN list > is actually counterproductive. > > Add a pair of simple loops to pass in contiguous PFNs only, to have > an > efficient implementation in VFIO. > > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> Reviewed-by: Eric Farman <farman@linux.ibm.com> > --- > drivers/s390/cio/vfio_ccw_cp.c | 70 +++++++++++++++++++++++++++----- > -- > 1 file changed, 56 insertions(+), 14 deletions(-) > > diff --git a/drivers/s390/cio/vfio_ccw_cp.c > b/drivers/s390/cio/vfio_ccw_cp.c > index 0c2be9421ab7..3b94863ad24e 100644 > --- a/drivers/s390/cio/vfio_ccw_cp.c > +++ b/drivers/s390/cio/vfio_ccw_cp.c > @@ -90,6 +90,38 @@ static int pfn_array_alloc(struct pfn_array *pa, > u64 iova, unsigned int len) > return 0; > } > > +/* > + * pfn_array_unpin() - Unpin user pages in memory > + * @pa: pfn_array on which to perform the operation > + * @vdev: the vfio device to perform the operation > + * @pa_nr: number of user pages to unpin > + * > + * Only unpin if any pages were pinned to begin with, i.e. pa_nr > > 0, > + * otherwise only clear pa->pa_nr > + */ > +static void pfn_array_unpin(struct pfn_array *pa, > + struct vfio_device *vdev, int pa_nr) > +{ > + int unpinned = 0, npage = 1; > + > + while (unpinned < pa_nr) { > + unsigned long *first = &pa->pa_iova_pfn[unpinned]; > + unsigned long *last = &first[npage]; > + > + if (unpinned + npage < pa_nr && > + *first + npage == *last) { > + npage++; > + continue; > + } > + > + vfio_unpin_pages(vdev, first, npage); > + unpinned += npage; > + npage = 1; > + } > + > + pa->pa_nr = 0; > +} > + > /* > * pfn_array_pin() - Pin user pages in memory > * @pa: pfn_array on which to perform the operation > @@ -101,34 +133,44 @@ static int pfn_array_alloc(struct pfn_array > *pa, u64 iova, unsigned int len) > */ > static int pfn_array_pin(struct pfn_array *pa, struct vfio_device > *vdev) > { > + int pinned = 0, npage = 1; > int ret = 0; > > - ret = vfio_pin_pages(vdev, pa->pa_iova_pfn, pa->pa_nr, > - IOMMU_READ | IOMMU_WRITE, pa->pa_pfn); > + while (pinned < pa->pa_nr) { > + unsigned long *first = &pa->pa_iova_pfn[pinned]; > + unsigned long *last = &first[npage]; > > - if (ret < 0) { > - goto err_out; > - } else if (ret > 0 && ret != pa->pa_nr) { > - vfio_unpin_pages(vdev, pa->pa_iova_pfn, ret); > - ret = -EINVAL; > - goto err_out; > + if (pinned + npage < pa->pa_nr && > + *first + npage == *last) { > + npage++; > + continue; > + } > + > + ret = vfio_pin_pages(vdev, first, npage, > + IOMMU_READ | IOMMU_WRITE, > + &pa->pa_pfn[pinned]); > + if (ret < 0) { > + goto err_out; > + } else if (ret > 0 && ret != npage) { > + pinned += ret; > + ret = -EINVAL; > + goto err_out; > + } > + pinned += npage; > + npage = 1; > } > > return ret; > > err_out: > - pa->pa_nr = 0; > - > + pfn_array_unpin(pa, vdev, pinned); > return ret; > } > > /* Unpin the pages before releasing the memory. */ > static void pfn_array_unpin_free(struct pfn_array *pa, struct > vfio_device *vdev) > { > - /* Only unpin if any pages were pinned to begin with */ > - if (pa->pa_nr) > - vfio_unpin_pages(vdev, pa->pa_iova_pfn, pa->pa_nr); > - pa->pa_nr = 0; > + pfn_array_unpin(pa, vdev, pa->pa_nr); > kfree(pa->pa_iova_pfn); > } >
On Wed, 2022-07-06 at 14:05 -0300, Jason Gunthorpe wrote: > On Tue, Jul 05, 2022 at 11:27:53PM -0700, Nicolin Chen wrote: > > This driver is the only caller of vfio_pin/unpin_pages that might > > pass > > in a non-contiguous PFN list, but in many cases it has a contiguous > > PFN > > list to process. So letting VFIO API handle a non-contiguous PFN > > list > > is actually counterproductive. > > > > Add a pair of simple loops to pass in contiguous PFNs only, to have > > an > > efficient implementation in VFIO. > > > > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> > > --- > > drivers/s390/cio/vfio_ccw_cp.c | 70 +++++++++++++++++++++++++++--- > > ---- > > 1 file changed, 56 insertions(+), 14 deletions(-) > > I think this is fine as-is for this series, but someone who knows and > can test ccw should go in and fix things so that pfn_array_alloc() > doesn't exist. Allocating memory and filling it with consecutive > integers is kind of silly given we can just call vfio_pin_pages() > with > pa_nr directly. > > pa->pa_iova_pfn[0] = pa->pa_iova >> PAGE_SHIFT; > pa->pa_pfn[0] = -1ULL; > for (i = 1; i < pa->pa_nr; i++) { > pa->pa_iova_pfn[i] = pa->pa_iova_pfn[i - 1] + 1; > > It looks like only the 'ccw_is_idal' flow can actually create > non-continuities. This code is certainly not my favorite, but you're right that it's the IDAL flow that generates the non-contiguous requests and the code you reference is simply an initialization for the !IDAL case. As I have a todo in this code anyway, I'll register your suggestion to see if they can be untangled. > Also the loop in copy_from_iova() should ideally be > using the much faster 'rw' interface, and not a pin/unpin cycle just > to memcpy. I guess I missed when that was added. This looks like low hanging fruit for some ollllld code regardless of the above. Will get to this once I'm back. Thank you! Eric > > If I guess right these changes would significantly speed this driver > up. > > Anyhow, > > Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> > > Jason
diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c index 0c2be9421ab7..3b94863ad24e 100644 --- a/drivers/s390/cio/vfio_ccw_cp.c +++ b/drivers/s390/cio/vfio_ccw_cp.c @@ -90,6 +90,38 @@ static int pfn_array_alloc(struct pfn_array *pa, u64 iova, unsigned int len) return 0; } +/* + * pfn_array_unpin() - Unpin user pages in memory + * @pa: pfn_array on which to perform the operation + * @vdev: the vfio device to perform the operation + * @pa_nr: number of user pages to unpin + * + * Only unpin if any pages were pinned to begin with, i.e. pa_nr > 0, + * otherwise only clear pa->pa_nr + */ +static void pfn_array_unpin(struct pfn_array *pa, + struct vfio_device *vdev, int pa_nr) +{ + int unpinned = 0, npage = 1; + + while (unpinned < pa_nr) { + unsigned long *first = &pa->pa_iova_pfn[unpinned]; + unsigned long *last = &first[npage]; + + if (unpinned + npage < pa_nr && + *first + npage == *last) { + npage++; + continue; + } + + vfio_unpin_pages(vdev, first, npage); + unpinned += npage; + npage = 1; + } + + pa->pa_nr = 0; +} + /* * pfn_array_pin() - Pin user pages in memory * @pa: pfn_array on which to perform the operation @@ -101,34 +133,44 @@ static int pfn_array_alloc(struct pfn_array *pa, u64 iova, unsigned int len) */ static int pfn_array_pin(struct pfn_array *pa, struct vfio_device *vdev) { + int pinned = 0, npage = 1; int ret = 0; - ret = vfio_pin_pages(vdev, pa->pa_iova_pfn, pa->pa_nr, - IOMMU_READ | IOMMU_WRITE, pa->pa_pfn); + while (pinned < pa->pa_nr) { + unsigned long *first = &pa->pa_iova_pfn[pinned]; + unsigned long *last = &first[npage]; - if (ret < 0) { - goto err_out; - } else if (ret > 0 && ret != pa->pa_nr) { - vfio_unpin_pages(vdev, pa->pa_iova_pfn, ret); - ret = -EINVAL; - goto err_out; + if (pinned + npage < pa->pa_nr && + *first + npage == *last) { + npage++; + continue; + } + + ret = vfio_pin_pages(vdev, first, npage, + IOMMU_READ | IOMMU_WRITE, + &pa->pa_pfn[pinned]); + if (ret < 0) { + goto err_out; + } else if (ret > 0 && ret != npage) { + pinned += ret; + ret = -EINVAL; + goto err_out; + } + pinned += npage; + npage = 1; } return ret; err_out: - pa->pa_nr = 0; - + pfn_array_unpin(pa, vdev, pinned); return ret; } /* Unpin the pages before releasing the memory. */ static void pfn_array_unpin_free(struct pfn_array *pa, struct vfio_device *vdev) { - /* Only unpin if any pages were pinned to begin with */ - if (pa->pa_nr) - vfio_unpin_pages(vdev, pa->pa_iova_pfn, pa->pa_nr); - pa->pa_nr = 0; + pfn_array_unpin(pa, vdev, pa->pa_nr); kfree(pa->pa_iova_pfn); }
This driver is the only caller of vfio_pin/unpin_pages that might pass in a non-contiguous PFN list, but in many cases it has a contiguous PFN list to process. So letting VFIO API handle a non-contiguous PFN list is actually counterproductive. Add a pair of simple loops to pass in contiguous PFNs only, to have an efficient implementation in VFIO. Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> --- drivers/s390/cio/vfio_ccw_cp.c | 70 +++++++++++++++++++++++++++------- 1 file changed, 56 insertions(+), 14 deletions(-)