| Message ID | 1493144468-22493-16-git-send-email-logang@deltatee.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Apr 25, 2017 at 12:21:02PM -0600, Logan Gunthorpe wrote: > Straightforward conversion to the new helper, except due to the lack > of error path, we have to use SG_MAP_MUST_NOT_FAIL which may BUG_ON in > certain cases in the future. > > Signed-off-by: Logan Gunthorpe <logang@deltatee.com> > Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com> > Cc: Juergen Gross <jgross@suse.com> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > Cc: "Roger Pau Monné" <roger.pau@citrix.com> > --- > drivers/block/xen-blkfront.c | 20 +++++++++++--------- > 1 file changed, 11 insertions(+), 9 deletions(-) > > diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c > index 3945963..ed62175 100644 > --- a/drivers/block/xen-blkfront.c > +++ b/drivers/block/xen-blkfront.c > @@ -816,8 +816,9 @@ static int blkif_queue_rw_req(struct request *req, struct blkfront_ring_info *ri > BUG_ON(sg->offset + sg->length > PAGE_SIZE); > > if (setup.need_copy) { > - setup.bvec_off = sg->offset; > - setup.bvec_data = kmap_atomic(sg_page(sg)); > + setup.bvec_off = 0; > + setup.bvec_data = sg_map(sg, 0, SG_KMAP_ATOMIC | > + SG_MAP_MUST_NOT_FAIL); I assume that sg_map already adds sg->offset to the address? Also wondering whether we can get rid of bvec_off and just increment bvec_data, adding Julien who IIRC added this code. > } > > gnttab_foreach_grant_in_range(sg_page(sg), > @@ -827,7 +828,7 @@ static int blkif_queue_rw_req(struct request *req, struct blkfront_ring_info *ri > &setup); > > if (setup.need_copy) > - kunmap_atomic(setup.bvec_data); > + sg_unmap(sg, setup.bvec_data, 0, SG_KMAP_ATOMIC); > } > if (setup.segments) > kunmap_atomic(setup.segments); > @@ -1053,7 +1054,7 @@ static int xen_translate_vdev(int vdevice, int *minor, unsigned int *offset) > case XEN_SCSI_DISK5_MAJOR: > case XEN_SCSI_DISK6_MAJOR: > case XEN_SCSI_DISK7_MAJOR: > - *offset = (*minor / PARTS_PER_DISK) + > + *offset = (*minor / PARTS_PER_DISK) + > ((major - XEN_SCSI_DISK1_MAJOR + 1) * 16) + > EMULATED_SD_DISK_NAME_OFFSET; > *minor = *minor + > @@ -1068,7 +1069,7 @@ static int xen_translate_vdev(int vdevice, int *minor, unsigned int *offset) > case XEN_SCSI_DISK13_MAJOR: > case XEN_SCSI_DISK14_MAJOR: > case XEN_SCSI_DISK15_MAJOR: > - *offset = (*minor / PARTS_PER_DISK) + > + *offset = (*minor / PARTS_PER_DISK) + > ((major - XEN_SCSI_DISK8_MAJOR + 8) * 16) + > EMULATED_SD_DISK_NAME_OFFSET; > *minor = *minor + > @@ -1119,7 +1120,7 @@ static int xlvbd_alloc_gendisk(blkif_sector_t capacity, > if (!VDEV_IS_EXTENDED(info->vdevice)) { > err = xen_translate_vdev(info->vdevice, &minor, &offset); > if (err) > - return err; > + return err; Cosmetic changes should go in a separate patch please. Roger. -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 26/04/17 01:37 AM, Roger Pau Monné wrote: > On Tue, Apr 25, 2017 at 12:21:02PM -0600, Logan Gunthorpe wrote: >> Straightforward conversion to the new helper, except due to the lack >> of error path, we have to use SG_MAP_MUST_NOT_FAIL which may BUG_ON in >> certain cases in the future. >> >> Signed-off-by: Logan Gunthorpe <logang@deltatee.com> >> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com> >> Cc: Juergen Gross <jgross@suse.com> >> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> >> Cc: "Roger Pau Monné" <roger.pau@citrix.com> >> --- >> drivers/block/xen-blkfront.c | 20 +++++++++++--------- >> 1 file changed, 11 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c >> index 3945963..ed62175 100644 >> --- a/drivers/block/xen-blkfront.c >> +++ b/drivers/block/xen-blkfront.c >> @@ -816,8 +816,9 @@ static int blkif_queue_rw_req(struct request *req, struct blkfront_ring_info *ri >> BUG_ON(sg->offset + sg->length > PAGE_SIZE); >> >> if (setup.need_copy) { >> - setup.bvec_off = sg->offset; >> - setup.bvec_data = kmap_atomic(sg_page(sg)); >> + setup.bvec_off = 0; >> + setup.bvec_data = sg_map(sg, 0, SG_KMAP_ATOMIC | >> + SG_MAP_MUST_NOT_FAIL); > > I assume that sg_map already adds sg->offset to the address? Correct. > Also wondering whether we can get rid of bvec_off and just increment bvec_data, > adding Julien who IIRC added this code. bvec_off is used to keep track of the offset within the current mapping so it's not a great idea given that you'd want to kunmap_atomic the original address and not something with an offset. It would be nice if this could be converted to use the sg_miter interface but that's a much more invasive change that would require someone who knows this code and can properly test it. I'd be very grateful if someone actually took that on. Logan -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 27, 2017 at 02:19:24PM -0600, Logan Gunthorpe wrote: > > > On 26/04/17 01:37 AM, Roger Pau Monné wrote: > > On Tue, Apr 25, 2017 at 12:21:02PM -0600, Logan Gunthorpe wrote: > >> Straightforward conversion to the new helper, except due to the lack > >> of error path, we have to use SG_MAP_MUST_NOT_FAIL which may BUG_ON in > >> certain cases in the future. > >> > >> Signed-off-by: Logan Gunthorpe <logang@deltatee.com> > >> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com> > >> Cc: Juergen Gross <jgross@suse.com> > >> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > >> Cc: "Roger Pau Monné" <roger.pau@citrix.com> > >> drivers/block/xen-blkfront.c | 20 +++++++++++--------- > >> 1 file changed, 11 insertions(+), 9 deletions(-) > >> > >> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c > >> index 3945963..ed62175 100644 > >> +++ b/drivers/block/xen-blkfront.c > >> @@ -816,8 +816,9 @@ static int blkif_queue_rw_req(struct request *req, struct blkfront_ring_info *ri > >> BUG_ON(sg->offset + sg->length > PAGE_SIZE); > >> > >> if (setup.need_copy) { > >> - setup.bvec_off = sg->offset; > >> - setup.bvec_data = kmap_atomic(sg_page(sg)); > >> + setup.bvec_off = 0; > >> + setup.bvec_data = sg_map(sg, 0, SG_KMAP_ATOMIC | > >> + SG_MAP_MUST_NOT_FAIL); > > > > I assume that sg_map already adds sg->offset to the address? > > Correct. > > > Also wondering whether we can get rid of bvec_off and just increment bvec_data, > > adding Julien who IIRC added this code. > > bvec_off is used to keep track of the offset within the current mapping > so it's not a great idea given that you'd want to kunmap_atomic the > original address and not something with an offset. It would be nice if > this could be converted to use the sg_miter interface but that's a much > more invasive change that would require someone who knows this code and > can properly test it. I'd be very grateful if someone actually took that on. blkfront is one of the drivers I looked at, and it appears to only be memcpying with the bvec_data pointer, so I wonder why it does not use sg_copy_X_buffer instead.. Jason -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 27/04/17 02:53 PM, Jason Gunthorpe wrote: > blkfront is one of the drivers I looked at, and it appears to only be > memcpying with the bvec_data pointer, so I wonder why it does not use > sg_copy_X_buffer instead.. Yes, sort of... But you'd potentially end up calling sg_copy_to_buffer multiple times per page within the sg (given that gnttab_foreach_grant_in_range might call blkif_copy_from_grant/blkif_setup_rw_req_grant multiple times). Even calling sg_copy_to_buffer once per page seems rather inefficient as it uses sg_miter internally. Switching the for_each_sg to sg_miter is probably the nicer solution as it takes care of the mapping and the offset/length accounting for you and will have similar performance. But, yes, if performance is not an issue, switching it to sg_copy_to_buffer would be a less invasive change than sg_miter. Which the same might be said about a lot of these cases. Unfortunately, changing from kmap_atomic (which is a null operation in a lot of cases) to sg_copy_X_buffer is a pretty big performance hit. Logan -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 27, 2017 at 03:53:37PM -0600, Logan Gunthorpe wrote: > On 27/04/17 02:53 PM, Jason Gunthorpe wrote: > > blkfront is one of the drivers I looked at, and it appears to only be > > memcpying with the bvec_data pointer, so I wonder why it does not use > > sg_copy_X_buffer instead.. > > But you'd potentially end up calling sg_copy_to_buffer multiple times > per page within the sg (given that gnttab_foreach_grant_in_range might > call blkif_copy_from_grant/blkif_setup_rw_req_grant multiple times). > Even calling sg_copy_to_buffer once per page seems rather inefficient as > it uses sg_miter internally. Well, that is in the current form, with more users it would make sense to optimize for the single page case, eg by providing the existing call, providing a faster single-page-only variant of the copy, perhaps even one that is inlined. > Switching the for_each_sg to sg_miter is probably the nicer solution as > it takes care of the mapping and the offset/length accounting for you > and will have similar performance. sg_miter will still fail when the sg contains __iomem, however I would expect that the sg_copy will work with iomem, by using the __iomem memcpy variant. So, sg_copy should always be preferred in this new world with mixed __iomem since it is the only primitive that can transparently handle it. Jason -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 27/04/17 04:11 PM, Jason Gunthorpe wrote: > On Thu, Apr 27, 2017 at 03:53:37PM -0600, Logan Gunthorpe wrote: > Well, that is in the current form, with more users it would make sense > to optimize for the single page case, eg by providing the existing > call, providing a faster single-page-only variant of the copy, perhaps > even one that is inlined. Ok, does it make sense then to have an sg_copy_page_to_buffer (or some such... I'm having trouble thinking of a sane name that isn't too long). That just does k(un)map_atomic and memcpy? I could try that if it makes sense to people. >> Switching the for_each_sg to sg_miter is probably the nicer solution as >> it takes care of the mapping and the offset/length accounting for you >> and will have similar performance. > > sg_miter will still fail when the sg contains __iomem, however I would > expect that the sg_copy will work with iomem, by using the __iomem > memcpy variant. Yes, that's true. Any sg_miters that ever see iomem will need to be converted to support it. This isn't much different than the other kmap(sg_page()) users I was converting that will also fail if they see iomem. Though, I suspect an sg_miter user would be easier to convert to iomem than a random kmap user. Logan -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 27, 2017 at 05:03:45PM -0600, Logan Gunthorpe wrote: > > > On 27/04/17 04:11 PM, Jason Gunthorpe wrote: > > On Thu, Apr 27, 2017 at 03:53:37PM -0600, Logan Gunthorpe wrote: > > Well, that is in the current form, with more users it would make sense > > to optimize for the single page case, eg by providing the existing > > call, providing a faster single-page-only variant of the copy, perhaps > > even one that is inlined. > > Ok, does it make sense then to have an sg_copy_page_to_buffer (or some > such... I'm having trouble thinking of a sane name that isn't too long). > That just does k(un)map_atomic and memcpy? I could try that if it makes > sense to people. It seems the most robust: test for iomem, and jump to a slow path copy, otherwise inline the kmap and memcpy Every place doing memcpy from sgl will need that pattern to be correct. > > sg_miter will still fail when the sg contains __iomem, however I would > > expect that the sg_copy will work with iomem, by using the __iomem > > memcpy variant. > > Yes, that's true. Any sg_miters that ever see iomem will need to be > converted to support it. This isn't much different than the other > kmap(sg_page()) users I was converting that will also fail if they see > iomem. Though, I suspect an sg_miter user would be easier to convert to > iomem than a random kmap user. How? sg_miter seems like the next nightmare down this path, what is sg_miter_next supposed to do when something hits an iomem sgl? miter.addr is supposed to be a kernel pointer that must not be __iomem.. Jason -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 27/04/17 05:20 PM, Jason Gunthorpe wrote: > It seems the most robust: test for iomem, and jump to a slow path > copy, otherwise inline the kmap and memcpy > > Every place doing memcpy from sgl will need that pattern to be > correct. Ok, sounds like a good place to start to me. I'll see what I can do for a v3 of this set. Though, I probably won't send anything until after the merge window. >>> sg_miter will still fail when the sg contains __iomem, however I would >>> expect that the sg_copy will work with iomem, by using the __iomem >>> memcpy variant. >> >> Yes, that's true. Any sg_miters that ever see iomem will need to be >> converted to support it. This isn't much different than the other >> kmap(sg_page()) users I was converting that will also fail if they see >> iomem. Though, I suspect an sg_miter user would be easier to convert to >> iomem than a random kmap user. > > How? sg_miter seems like the next nightmare down this path, what is > sg_miter_next supposed to do when something hits an iomem sgl? My proposal is roughly included in the draft I sent upthread. We add an sg_miter flag indicating the iteratee supports iomem and if miter finds iomem (with the support flag set) it sets ioaddr which is __iomem. The iteratee then just needs to null check addr and ioaddr and perform the appropriate action. Logan -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c index 3945963..ed62175 100644 --- a/drivers/block/xen-blkfront.c +++ b/drivers/block/xen-blkfront.c @@ -816,8 +816,9 @@ static int blkif_queue_rw_req(struct request *req, struct blkfront_ring_info *ri BUG_ON(sg->offset + sg->length > PAGE_SIZE); if (setup.need_copy) { - setup.bvec_off = sg->offset; - setup.bvec_data = kmap_atomic(sg_page(sg)); + setup.bvec_off = 0; + setup.bvec_data = sg_map(sg, 0, SG_KMAP_ATOMIC | + SG_MAP_MUST_NOT_FAIL); } gnttab_foreach_grant_in_range(sg_page(sg), @@ -827,7 +828,7 @@ static int blkif_queue_rw_req(struct request *req, struct blkfront_ring_info *ri &setup); if (setup.need_copy) - kunmap_atomic(setup.bvec_data); + sg_unmap(sg, setup.bvec_data, 0, SG_KMAP_ATOMIC); } if (setup.segments) kunmap_atomic(setup.segments); @@ -1053,7 +1054,7 @@ static int xen_translate_vdev(int vdevice, int *minor, unsigned int *offset) case XEN_SCSI_DISK5_MAJOR: case XEN_SCSI_DISK6_MAJOR: case XEN_SCSI_DISK7_MAJOR: - *offset = (*minor / PARTS_PER_DISK) + + *offset = (*minor / PARTS_PER_DISK) + ((major - XEN_SCSI_DISK1_MAJOR + 1) * 16) + EMULATED_SD_DISK_NAME_OFFSET; *minor = *minor + @@ -1068,7 +1069,7 @@ static int xen_translate_vdev(int vdevice, int *minor, unsigned int *offset) case XEN_SCSI_DISK13_MAJOR: case XEN_SCSI_DISK14_MAJOR: case XEN_SCSI_DISK15_MAJOR: - *offset = (*minor / PARTS_PER_DISK) + + *offset = (*minor / PARTS_PER_DISK) + ((major - XEN_SCSI_DISK8_MAJOR + 8) * 16) + EMULATED_SD_DISK_NAME_OFFSET; *minor = *minor + @@ -1119,7 +1120,7 @@ static int xlvbd_alloc_gendisk(blkif_sector_t capacity, if (!VDEV_IS_EXTENDED(info->vdevice)) { err = xen_translate_vdev(info->vdevice, &minor, &offset); if (err) - return err; + return err; nr_parts = PARTS_PER_DISK; } else { minor = BLKIF_MINOR_EXT(info->vdevice); @@ -1483,8 +1484,9 @@ static bool blkif_completion(unsigned long *id, for_each_sg(s->sg, sg, num_sg, i) { BUG_ON(sg->offset + sg->length > PAGE_SIZE); - data.bvec_offset = sg->offset; - data.bvec_data = kmap_atomic(sg_page(sg)); + data.bvec_offset = 0; + data.bvec_data = sg_map(sg, 0, SG_KMAP_ATOMIC | + SG_MAP_MUST_NOT_FAIL); gnttab_foreach_grant_in_range(sg_page(sg), sg->offset, @@ -1492,7 +1494,7 @@ static bool blkif_completion(unsigned long *id, blkif_copy_from_grant, &data); - kunmap_atomic(data.bvec_data); + sg_unmap(sg, data.bvec_data, 0, SG_KMAP_ATOMIC); } } /* Add the persistent grant into the list of free grants */
Straightforward conversion to the new helper, except due to the lack of error path, we have to use SG_MAP_MUST_NOT_FAIL which may BUG_ON in certain cases in the future. Signed-off-by: Logan Gunthorpe <logang@deltatee.com> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com> Cc: Juergen Gross <jgross@suse.com> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Cc: "Roger Pau Monné" <roger.pau@citrix.com> --- drivers/block/xen-blkfront.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-)