| Message ID | 1443013156-8612-1-git-send-email-vkuznets@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> -----Original Message----- > From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com] > Sent: Wednesday, September 23, 2015 5:59 AM > To: linux-scsi@vger.kernel.org > Cc: linux-kernel@vger.kernel.org; devel@linuxdriverproject.org; KY > Srinivasan <kys@microsoft.com>; Haiyang Zhang > <haiyangz@microsoft.com>; James E.J. Bottomley <JBottomley@odin.com>; > Radim Kr?má? <rkrcmar@redhat.com>; Christoph Hellwig > <hch@infradead.org> > Subject: [PATCH v2] storvsc: get rid of bounce buffer > > Storvsc driver needs to ensure there are no 'holes' in the presented > sg list (all segments in the middle of the list need to be of PAGE_SIZE). > When a hole is detected storvsc driver creates a 'bounce sgl' without > holes and copies data over with copy_{to,from}_bounce_buffer() functions. > Setting virt_boundary_mask to PAGE_SIZE - 1 guarantees we'll never see > such holes so we can significantly simplify the driver. This is also > supposed to bring us some performance improvement for certain workloads > as we eliminate copying. > > Reported-by: Radim Kr?má? <rkrcmar@redhat.com> > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> > --- > Changes since v1: > - This patch is a successor of 'storvsc: get rid of homegrown > copy_{to,from}_bounce_buffer()'. Use virt_boundary instead to > eliminate the need for bounce buffer completely [Christoph Hellwig]. > --- > drivers/scsi/storvsc_drv.c | 286 +-------------------------------------------- > 1 file changed, 5 insertions(+), 281 deletions(-) > > diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c > index 40c43ae..eeade40 100644 > --- a/drivers/scsi/storvsc_drv.c > +++ b/drivers/scsi/storvsc_drv.c > @@ -393,9 +393,6 @@ static void storvsc_on_channel_callback(void > *context); > struct storvsc_cmd_request { > struct scsi_cmnd *cmd; > > - unsigned int bounce_sgl_count; > - struct scatterlist *bounce_sgl; > - > struct hv_device *device; > > /* Synchronize the request/response if needed */ > @@ -586,241 +583,6 @@ get_in_err: > > } > > -static void destroy_bounce_buffer(struct scatterlist *sgl, > - unsigned int sg_count) > -{ > - int i; > - struct page *page_buf; > - > - for (i = 0; i < sg_count; i++) { > - page_buf = sg_page((&sgl[i])); > - if (page_buf != NULL) > - __free_page(page_buf); > - } > - > - kfree(sgl); > -} > - > -static int do_bounce_buffer(struct scatterlist *sgl, unsigned int sg_count) > -{ > - int i; > - > - /* No need to check */ > - if (sg_count < 2) > - return -1; > - > - /* We have at least 2 sg entries */ > - for (i = 0; i < sg_count; i++) { > - if (i == 0) { > - /* make sure 1st one does not have hole */ > - if (sgl[i].offset + sgl[i].length != PAGE_SIZE) > - return i; > - } else if (i == sg_count - 1) { > - /* make sure last one does not have hole */ > - if (sgl[i].offset != 0) > - return i; > - } else { > - /* make sure no hole in the middle */ > - if (sgl[i].length != PAGE_SIZE || sgl[i].offset != 0) > - return i; > - } > - } > - return -1; > -} > - > -static struct scatterlist *create_bounce_buffer(struct scatterlist *sgl, > - unsigned int sg_count, > - unsigned int len, > - int write) > -{ > - int i; > - int num_pages; > - struct scatterlist *bounce_sgl; > - struct page *page_buf; > - unsigned int buf_len = ((write == WRITE_TYPE) ? 0 : PAGE_SIZE); > - > - num_pages = ALIGN(len, PAGE_SIZE) >> PAGE_SHIFT; > - > - bounce_sgl = kcalloc(num_pages, sizeof(struct scatterlist), > GFP_ATOMIC); > - if (!bounce_sgl) > - return NULL; > - > - sg_init_table(bounce_sgl, num_pages); > - for (i = 0; i < num_pages; i++) { > - page_buf = alloc_page(GFP_ATOMIC); > - if (!page_buf) > - goto cleanup; > - sg_set_page(&bounce_sgl[i], page_buf, buf_len, 0); > - } > - > - return bounce_sgl; > - > -cleanup: > - destroy_bounce_buffer(bounce_sgl, num_pages); > - return NULL; > -} > - > -/* Assume the original sgl has enough room */ > -static unsigned int copy_from_bounce_buffer(struct scatterlist *orig_sgl, > - struct scatterlist *bounce_sgl, > - unsigned int orig_sgl_count, > - unsigned int bounce_sgl_count) > -{ > - int i; > - int j = 0; > - unsigned long src, dest; > - unsigned int srclen, destlen, copylen; > - unsigned int total_copied = 0; > - unsigned long bounce_addr = 0; > - unsigned long dest_addr = 0; > - unsigned long flags; > - struct scatterlist *cur_dest_sgl; > - struct scatterlist *cur_src_sgl; > - > - local_irq_save(flags); > - cur_dest_sgl = orig_sgl; > - cur_src_sgl = bounce_sgl; > - for (i = 0; i < orig_sgl_count; i++) { > - dest_addr = (unsigned long) > - kmap_atomic(sg_page(cur_dest_sgl)) + > - cur_dest_sgl->offset; > - dest = dest_addr; > - destlen = cur_dest_sgl->length; > - > - if (bounce_addr == 0) > - bounce_addr = (unsigned long)kmap_atomic( > - > sg_page(cur_src_sgl)); > - > - while (destlen) { > - src = bounce_addr + cur_src_sgl->offset; > - srclen = cur_src_sgl->length - cur_src_sgl->offset; > - > - copylen = min(srclen, destlen); > - memcpy((void *)dest, (void *)src, copylen); > - > - total_copied += copylen; > - cur_src_sgl->offset += copylen; > - destlen -= copylen; > - dest += copylen; > - > - if (cur_src_sgl->offset == cur_src_sgl->length) { > - /* full */ > - kunmap_atomic((void *)bounce_addr); > - j++; > - > - /* > - * It is possible that the number of elements > - * in the bounce buffer may not be equal to > - * the number of elements in the original > - * scatter list. Handle this correctly. > - */ > - > - if (j == bounce_sgl_count) { > - /* > - * We are done; cleanup and return. > - */ > - kunmap_atomic((void *)(dest_addr - > - cur_dest_sgl->offset)); > - local_irq_restore(flags); > - return total_copied; > - } > - > - /* if we need to use another bounce buffer > */ > - if (destlen || i != orig_sgl_count - 1) { > - cur_src_sgl = sg_next(cur_src_sgl); > - bounce_addr = (unsigned long) > - kmap_atomic( > - > sg_page(cur_src_sgl)); > - } > - } else if (destlen == 0 && i == orig_sgl_count - 1) { > - /* unmap the last bounce that is < PAGE_SIZE > */ > - kunmap_atomic((void *)bounce_addr); > - } > - } > - > - kunmap_atomic((void *)(dest_addr - cur_dest_sgl->offset)); > - cur_dest_sgl = sg_next(cur_dest_sgl); > - } > - > - local_irq_restore(flags); > - > - return total_copied; > -} > - > -/* Assume the bounce_sgl has enough room ie using the > create_bounce_buffer() */ > -static unsigned int copy_to_bounce_buffer(struct scatterlist *orig_sgl, > - struct scatterlist *bounce_sgl, > - unsigned int orig_sgl_count) > -{ > - int i; > - int j = 0; > - unsigned long src, dest; > - unsigned int srclen, destlen, copylen; > - unsigned int total_copied = 0; > - unsigned long bounce_addr = 0; > - unsigned long src_addr = 0; > - unsigned long flags; > - struct scatterlist *cur_src_sgl; > - struct scatterlist *cur_dest_sgl; > - > - local_irq_save(flags); > - > - cur_src_sgl = orig_sgl; > - cur_dest_sgl = bounce_sgl; > - > - for (i = 0; i < orig_sgl_count; i++) { > - src_addr = (unsigned long) > - kmap_atomic(sg_page(cur_src_sgl)) + > - cur_src_sgl->offset; > - src = src_addr; > - srclen = cur_src_sgl->length; > - > - if (bounce_addr == 0) > - bounce_addr = (unsigned long) > - > kmap_atomic(sg_page(cur_dest_sgl)); > - > - while (srclen) { > - /* assume bounce offset always == 0 */ > - dest = bounce_addr + cur_dest_sgl->length; > - destlen = PAGE_SIZE - cur_dest_sgl->length; > - > - copylen = min(srclen, destlen); > - memcpy((void *)dest, (void *)src, copylen); > - > - total_copied += copylen; > - cur_dest_sgl->length += copylen; > - srclen -= copylen; > - src += copylen; > - > - if (cur_dest_sgl->length == PAGE_SIZE) { > - /* full..move to next entry */ > - kunmap_atomic((void *)bounce_addr); > - bounce_addr = 0; > - j++; > - } > - > - /* if we need to use another bounce buffer */ > - if (srclen && bounce_addr == 0) { > - cur_dest_sgl = sg_next(cur_dest_sgl); > - bounce_addr = (unsigned long) > - kmap_atomic( > - sg_page(cur_dest_sgl)); > - } > - > - } > - > - kunmap_atomic((void *)(src_addr - cur_src_sgl->offset)); > - cur_src_sgl = sg_next(cur_src_sgl); > - } > - > - if (bounce_addr) > - kunmap_atomic((void *)bounce_addr); > - > - local_irq_restore(flags); > - > - return total_copied; > -} > - > static void handle_sc_creation(struct vmbus_channel *new_sc) > { > struct hv_device *device = new_sc->primary_channel->device_obj; > @@ -1171,15 +933,6 @@ static void storvsc_command_completion(struct > storvsc_cmd_request *cmd_request) > host = stor_dev->host; > > vm_srb = &cmd_request->vstor_packet.vm_srb; > - if (cmd_request->bounce_sgl_count) { > - if (vm_srb->data_in == READ_TYPE) > - copy_from_bounce_buffer(scsi_sglist(scmnd), > - cmd_request->bounce_sgl, > - scsi_sg_count(scmnd), > - cmd_request->bounce_sgl_count); > - destroy_bounce_buffer(cmd_request->bounce_sgl, > - cmd_request->bounce_sgl_count); > - } > > scmnd->result = vm_srb->scsi_status; > > @@ -1474,6 +1227,9 @@ static int storvsc_device_configure(struct > scsi_device *sdevice) > > blk_queue_rq_timeout(sdevice->request_queue, (storvsc_timeout > * HZ)); > > + /* Ensure there are no gaps in presented sgls */ > + blk_queue_virt_boundary(sdevice->request_queue, PAGE_SIZE - 1); > + > sdevice->no_write_same = 1; > > /* > @@ -1692,40 +1448,13 @@ static int storvsc_queuecommand(struct > Scsi_Host *host, struct scsi_cmnd *scmnd) > payload_sz = sizeof(cmd_request->mpb); > > if (sg_count) { > - /* check if we need to bounce the sgl */ > - if (do_bounce_buffer(sgl, scsi_sg_count(scmnd)) != -1) { > - cmd_request->bounce_sgl = > - create_bounce_buffer(sgl, sg_count, > - length, > - vm_srb->data_in); > - if (!cmd_request->bounce_sgl) > - return SCSI_MLQUEUE_HOST_BUSY; > - > - cmd_request->bounce_sgl_count = > - ALIGN(length, PAGE_SIZE) >> PAGE_SHIFT; > - > - if (vm_srb->data_in == WRITE_TYPE) > - copy_to_bounce_buffer(sgl, > - cmd_request->bounce_sgl, > sg_count); > - > - sgl = cmd_request->bounce_sgl; > - sg_count = cmd_request->bounce_sgl_count; > - } > - > - > if (sg_count > MAX_PAGE_BUFFER_COUNT) { > > payload_sz = (sg_count * sizeof(void *) + > sizeof(struct vmbus_packet_mpb_array)); > payload = kmalloc(payload_sz, GFP_ATOMIC); > - if (!payload) { > - if (cmd_request->bounce_sgl_count) > - destroy_bounce_buffer( > - cmd_request->bounce_sgl, > - cmd_request->bounce_sgl_count); > - > - return > SCSI_MLQUEUE_DEVICE_BUSY; > - } > + if (!payload) > + return SCSI_MLQUEUE_DEVICE_BUSY; > } > > payload->range.len = length; > @@ -1754,11 +1483,6 @@ static int storvsc_queuecommand(struct Scsi_Host > *host, struct scsi_cmnd *scmnd) > > if (ret == -EAGAIN) { > /* no more space */ > - > - if (cmd_request->bounce_sgl_count) > - destroy_bounce_buffer(cmd_request->bounce_sgl, > - cmd_request->bounce_sgl_count); > - > return SCSI_MLQUEUE_DEVICE_BUSY; > } > > -- > 2.4.3 Thanks Vitaly and Christoph. We will run this patch through our testing. Regards, K. Y
diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c index 40c43ae..eeade40 100644 --- a/drivers/scsi/storvsc_drv.c +++ b/drivers/scsi/storvsc_drv.c @@ -393,9 +393,6 @@ static void storvsc_on_channel_callback(void *context); struct storvsc_cmd_request { struct scsi_cmnd *cmd; - unsigned int bounce_sgl_count; - struct scatterlist *bounce_sgl; - struct hv_device *device; /* Synchronize the request/response if needed */ @@ -586,241 +583,6 @@ get_in_err: } -static void destroy_bounce_buffer(struct scatterlist *sgl, - unsigned int sg_count) -{ - int i; - struct page *page_buf; - - for (i = 0; i < sg_count; i++) { - page_buf = sg_page((&sgl[i])); - if (page_buf != NULL) - __free_page(page_buf); - } - - kfree(sgl); -} - -static int do_bounce_buffer(struct scatterlist *sgl, unsigned int sg_count) -{ - int i; - - /* No need to check */ - if (sg_count < 2) - return -1; - - /* We have at least 2 sg entries */ - for (i = 0; i < sg_count; i++) { - if (i == 0) { - /* make sure 1st one does not have hole */ - if (sgl[i].offset + sgl[i].length != PAGE_SIZE) - return i; - } else if (i == sg_count - 1) { - /* make sure last one does not have hole */ - if (sgl[i].offset != 0) - return i; - } else { - /* make sure no hole in the middle */ - if (sgl[i].length != PAGE_SIZE || sgl[i].offset != 0) - return i; - } - } - return -1; -} - -static struct scatterlist *create_bounce_buffer(struct scatterlist *sgl, - unsigned int sg_count, - unsigned int len, - int write) -{ - int i; - int num_pages; - struct scatterlist *bounce_sgl; - struct page *page_buf; - unsigned int buf_len = ((write == WRITE_TYPE) ? 0 : PAGE_SIZE); - - num_pages = ALIGN(len, PAGE_SIZE) >> PAGE_SHIFT; - - bounce_sgl = kcalloc(num_pages, sizeof(struct scatterlist), GFP_ATOMIC); - if (!bounce_sgl) - return NULL; - - sg_init_table(bounce_sgl, num_pages); - for (i = 0; i < num_pages; i++) { - page_buf = alloc_page(GFP_ATOMIC); - if (!page_buf) - goto cleanup; - sg_set_page(&bounce_sgl[i], page_buf, buf_len, 0); - } - - return bounce_sgl; - -cleanup: - destroy_bounce_buffer(bounce_sgl, num_pages); - return NULL; -} - -/* Assume the original sgl has enough room */ -static unsigned int copy_from_bounce_buffer(struct scatterlist *orig_sgl, - struct scatterlist *bounce_sgl, - unsigned int orig_sgl_count, - unsigned int bounce_sgl_count) -{ - int i; - int j = 0; - unsigned long src, dest; - unsigned int srclen, destlen, copylen; - unsigned int total_copied = 0; - unsigned long bounce_addr = 0; - unsigned long dest_addr = 0; - unsigned long flags; - struct scatterlist *cur_dest_sgl; - struct scatterlist *cur_src_sgl; - - local_irq_save(flags); - cur_dest_sgl = orig_sgl; - cur_src_sgl = bounce_sgl; - for (i = 0; i < orig_sgl_count; i++) { - dest_addr = (unsigned long) - kmap_atomic(sg_page(cur_dest_sgl)) + - cur_dest_sgl->offset; - dest = dest_addr; - destlen = cur_dest_sgl->length; - - if (bounce_addr == 0) - bounce_addr = (unsigned long)kmap_atomic( - sg_page(cur_src_sgl)); - - while (destlen) { - src = bounce_addr + cur_src_sgl->offset; - srclen = cur_src_sgl->length - cur_src_sgl->offset; - - copylen = min(srclen, destlen); - memcpy((void *)dest, (void *)src, copylen); - - total_copied += copylen; - cur_src_sgl->offset += copylen; - destlen -= copylen; - dest += copylen; - - if (cur_src_sgl->offset == cur_src_sgl->length) { - /* full */ - kunmap_atomic((void *)bounce_addr); - j++; - - /* - * It is possible that the number of elements - * in the bounce buffer may not be equal to - * the number of elements in the original - * scatter list. Handle this correctly. - */ - - if (j == bounce_sgl_count) { - /* - * We are done; cleanup and return. - */ - kunmap_atomic((void *)(dest_addr - - cur_dest_sgl->offset)); - local_irq_restore(flags); - return total_copied; - } - - /* if we need to use another bounce buffer */ - if (destlen || i != orig_sgl_count - 1) { - cur_src_sgl = sg_next(cur_src_sgl); - bounce_addr = (unsigned long) - kmap_atomic( - sg_page(cur_src_sgl)); - } - } else if (destlen == 0 && i == orig_sgl_count - 1) { - /* unmap the last bounce that is < PAGE_SIZE */ - kunmap_atomic((void *)bounce_addr); - } - } - - kunmap_atomic((void *)(dest_addr - cur_dest_sgl->offset)); - cur_dest_sgl = sg_next(cur_dest_sgl); - } - - local_irq_restore(flags); - - return total_copied; -} - -/* Assume the bounce_sgl has enough room ie using the create_bounce_buffer() */ -static unsigned int copy_to_bounce_buffer(struct scatterlist *orig_sgl, - struct scatterlist *bounce_sgl, - unsigned int orig_sgl_count) -{ - int i; - int j = 0; - unsigned long src, dest; - unsigned int srclen, destlen, copylen; - unsigned int total_copied = 0; - unsigned long bounce_addr = 0; - unsigned long src_addr = 0; - unsigned long flags; - struct scatterlist *cur_src_sgl; - struct scatterlist *cur_dest_sgl; - - local_irq_save(flags); - - cur_src_sgl = orig_sgl; - cur_dest_sgl = bounce_sgl; - - for (i = 0; i < orig_sgl_count; i++) { - src_addr = (unsigned long) - kmap_atomic(sg_page(cur_src_sgl)) + - cur_src_sgl->offset; - src = src_addr; - srclen = cur_src_sgl->length; - - if (bounce_addr == 0) - bounce_addr = (unsigned long) - kmap_atomic(sg_page(cur_dest_sgl)); - - while (srclen) { - /* assume bounce offset always == 0 */ - dest = bounce_addr + cur_dest_sgl->length; - destlen = PAGE_SIZE - cur_dest_sgl->length; - - copylen = min(srclen, destlen); - memcpy((void *)dest, (void *)src, copylen); - - total_copied += copylen; - cur_dest_sgl->length += copylen; - srclen -= copylen; - src += copylen; - - if (cur_dest_sgl->length == PAGE_SIZE) { - /* full..move to next entry */ - kunmap_atomic((void *)bounce_addr); - bounce_addr = 0; - j++; - } - - /* if we need to use another bounce buffer */ - if (srclen && bounce_addr == 0) { - cur_dest_sgl = sg_next(cur_dest_sgl); - bounce_addr = (unsigned long) - kmap_atomic( - sg_page(cur_dest_sgl)); - } - - } - - kunmap_atomic((void *)(src_addr - cur_src_sgl->offset)); - cur_src_sgl = sg_next(cur_src_sgl); - } - - if (bounce_addr) - kunmap_atomic((void *)bounce_addr); - - local_irq_restore(flags); - - return total_copied; -} - static void handle_sc_creation(struct vmbus_channel *new_sc) { struct hv_device *device = new_sc->primary_channel->device_obj; @@ -1171,15 +933,6 @@ static void storvsc_command_completion(struct storvsc_cmd_request *cmd_request) host = stor_dev->host; vm_srb = &cmd_request->vstor_packet.vm_srb; - if (cmd_request->bounce_sgl_count) { - if (vm_srb->data_in == READ_TYPE) - copy_from_bounce_buffer(scsi_sglist(scmnd), - cmd_request->bounce_sgl, - scsi_sg_count(scmnd), - cmd_request->bounce_sgl_count); - destroy_bounce_buffer(cmd_request->bounce_sgl, - cmd_request->bounce_sgl_count); - } scmnd->result = vm_srb->scsi_status; @@ -1474,6 +1227,9 @@ static int storvsc_device_configure(struct scsi_device *sdevice) blk_queue_rq_timeout(sdevice->request_queue, (storvsc_timeout * HZ)); + /* Ensure there are no gaps in presented sgls */ + blk_queue_virt_boundary(sdevice->request_queue, PAGE_SIZE - 1); + sdevice->no_write_same = 1; /* @@ -1692,40 +1448,13 @@ static int storvsc_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *scmnd) payload_sz = sizeof(cmd_request->mpb); if (sg_count) { - /* check if we need to bounce the sgl */ - if (do_bounce_buffer(sgl, scsi_sg_count(scmnd)) != -1) { - cmd_request->bounce_sgl = - create_bounce_buffer(sgl, sg_count, - length, - vm_srb->data_in); - if (!cmd_request->bounce_sgl) - return SCSI_MLQUEUE_HOST_BUSY; - - cmd_request->bounce_sgl_count = - ALIGN(length, PAGE_SIZE) >> PAGE_SHIFT; - - if (vm_srb->data_in == WRITE_TYPE) - copy_to_bounce_buffer(sgl, - cmd_request->bounce_sgl, sg_count); - - sgl = cmd_request->bounce_sgl; - sg_count = cmd_request->bounce_sgl_count; - } - - if (sg_count > MAX_PAGE_BUFFER_COUNT) { payload_sz = (sg_count * sizeof(void *) + sizeof(struct vmbus_packet_mpb_array)); payload = kmalloc(payload_sz, GFP_ATOMIC); - if (!payload) { - if (cmd_request->bounce_sgl_count) - destroy_bounce_buffer( - cmd_request->bounce_sgl, - cmd_request->bounce_sgl_count); - - return SCSI_MLQUEUE_DEVICE_BUSY; - } + if (!payload) + return SCSI_MLQUEUE_DEVICE_BUSY; } payload->range.len = length; @@ -1754,11 +1483,6 @@ static int storvsc_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *scmnd) if (ret == -EAGAIN) { /* no more space */ - - if (cmd_request->bounce_sgl_count) - destroy_bounce_buffer(cmd_request->bounce_sgl, - cmd_request->bounce_sgl_count); - return SCSI_MLQUEUE_DEVICE_BUSY; }
Storvsc driver needs to ensure there are no 'holes' in the presented sg list (all segments in the middle of the list need to be of PAGE_SIZE). When a hole is detected storvsc driver creates a 'bounce sgl' without holes and copies data over with copy_{to,from}_bounce_buffer() functions. Setting virt_boundary_mask to PAGE_SIZE - 1 guarantees we'll never see such holes so we can significantly simplify the driver. This is also supposed to bring us some performance improvement for certain workloads as we eliminate copying. Reported-by: Radim Kr?má? <rkrcmar@redhat.com> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> --- Changes since v1: - This patch is a successor of 'storvsc: get rid of homegrown copy_{to,from}_bounce_buffer()'. Use virt_boundary instead to eliminate the need for bounce buffer completely [Christoph Hellwig]. --- drivers/scsi/storvsc_drv.c | 286 +-------------------------------------------- 1 file changed, 5 insertions(+), 281 deletions(-)