| Message ID | 5368DB90.9050209@acm.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 5/6/2014 3:54 PM, Bart Van Assche wrote: > This patch is needed by the patch that adds fast registration support. > > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > Cc: Roland Dreier <roland@purestorage.com> > Cc: David Dillow <dave@thedillows.org> > Cc: Sagi Grimberg <sagig@mellanox.com> > Cc: Vu Pham <vu@mellanox.com> > Cc: Sebastian Parschauer <sebastian.riemer@profitbricks.com> > --- > drivers/infiniband/ulp/srp/ib_srp.c | 41 ++++++++++++++++++++++++------------- > 1 file changed, 27 insertions(+), 14 deletions(-) > > diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c > index ba434d6..1c4b0d3 100644 > --- a/drivers/infiniband/ulp/srp/ib_srp.c > +++ b/drivers/infiniband/ulp/srp/ib_srp.c > @@ -574,17 +574,18 @@ static void srp_disconnect_target(struct srp_target_port *target) > } > } > > -static void srp_free_req_data(struct srp_target_port *target) > +static void srp_free_req_data(struct srp_target_port *target, > + struct srp_request *req_ring) > { Something here feels wrong (or partially right). > struct ib_device *ibdev = target->srp_host->srp_dev->dev; > struct srp_request *req; > int i; > > - if (!target->req_ring) > + if (!req_ring) > return; > > for (i = 0; i < target->req_ring_size; ++i) { > - req = &target->req_ring[i]; > + req = &req_ring[i]; You loop for {ring A size} and operates on ring B elements. They will probably be the same but the notion seems buggy. Will it be better to untie this routine from srp_target_port at all? > kfree(req->fmr_list); > kfree(req->map_page); > if (req->indirect_dma_addr) { > @@ -595,27 +596,34 @@ static void srp_free_req_data(struct srp_target_port *target) > kfree(req->indirect_desc); > } > > - kfree(target->req_ring); > - target->req_ring = NULL; > + kfree(req_ring); > } > > +/** > + * srp_alloc_req_data() - allocate or reallocate request data > + * @target: SRP target port. > + * > + * If target->req_ring was non-NULL before this function got invoked it will > + * also be non-NULL after this function has finished. > + */ > static int srp_alloc_req_data(struct srp_target_port *target) > { > struct srp_device *srp_dev = target->srp_host->srp_dev; > struct ib_device *ibdev = srp_dev->dev; > - struct srp_request *req; > + struct list_head free_reqs; > + struct srp_request *req_ring, *req; > dma_addr_t dma_addr; > int i, ret = -ENOMEM; > > - INIT_LIST_HEAD(&target->free_reqs); > + INIT_LIST_HEAD(&free_reqs); > > - target->req_ring = kzalloc(target->req_ring_size * > - sizeof(*target->req_ring), GFP_KERNEL); > - if (!target->req_ring) > + req_ring = kzalloc(target->req_ring_size * sizeof(*req_ring), > + GFP_KERNEL); > + if (!req_ring) > goto out; > > for (i = 0; i < target->req_ring_size; ++i) { > - req = &target->req_ring[i]; > + req = &req_ring[i]; > req->fmr_list = kmalloc(target->cmd_sg_cnt * sizeof(void *), > GFP_KERNEL); > req->map_page = kmalloc(SRP_FMR_SIZE * sizeof(void *), > @@ -632,11 +640,16 @@ static int srp_alloc_req_data(struct srp_target_port *target) > > req->indirect_dma_addr = dma_addr; > req->index = i; > - list_add_tail(&req->list, &target->free_reqs); > + list_add_tail(&req->list, &free_reqs); > } > + swap(target->req_ring, req_ring); > + INIT_LIST_HEAD(&target->free_reqs); > + list_splice(&free_reqs, &target->free_reqs); > ret = 0; > > out: > + srp_free_req_data(target, req_ring); > + > return ret; > } > > @@ -669,7 +682,7 @@ static void srp_remove_target(struct srp_target_port *target) > srp_free_target_ib(target); > cancel_work_sync(&target->tl_err_work); > srp_rport_put(target->rport); > - srp_free_req_data(target); > + srp_free_req_data(target, target->req_ring); > > spin_lock(&target->srp_host->target_lock); > list_del(&target->list); > @@ -2750,7 +2763,7 @@ err_free_ib: > srp_free_target_ib(target); > > err_free_mem: > - srp_free_req_data(target); > + srp_free_req_data(target, target->req_ring); > > err: > scsi_host_put(target_host); -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/07/14 12:50, Sagi Grimberg wrote: > On 5/6/2014 3:54 PM, Bart Van Assche wrote: >> -static void srp_free_req_data(struct srp_target_port *target) >> +static void srp_free_req_data(struct srp_target_port *target, >> + struct srp_request *req_ring) >> { >> struct ib_device *ibdev = target->srp_host->srp_dev->dev; >> struct srp_request *req; >> int i; >> - if (!target->req_ring) >> + if (!req_ring) >> return; >> for (i = 0; i < target->req_ring_size; ++i) { >> - req = &target->req_ring[i]; >> + req = &req_ring[i]; > > You loop for {ring A size} and operates on ring B elements. They will > probably be the same but the notion seems buggy. > Will it be better to untie this routine from srp_target_port at all? Hello Sagi, Had you noticed that target->req_ring_size is not modified during resource allocation or reallocation ? That is why target->req_ring_size has not been converted into a function argument in this patch. Bart. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 5/7/2014 5:11 PM, Bart Van Assche wrote: > On 05/07/14 12:50, Sagi Grimberg wrote: >> On 5/6/2014 3:54 PM, Bart Van Assche wrote: >>> -static void srp_free_req_data(struct srp_target_port *target) >>> +static void srp_free_req_data(struct srp_target_port *target, >>> + struct srp_request *req_ring) >>> { >>> struct ib_device *ibdev = target->srp_host->srp_dev->dev; >>> struct srp_request *req; >>> int i; >>> - if (!target->req_ring) >>> + if (!req_ring) >>> return; >>> for (i = 0; i < target->req_ring_size; ++i) { >>> - req = &target->req_ring[i]; >>> + req = &req_ring[i]; >> You loop for {ring A size} and operates on ring B elements. They will >> probably be the same but the notion seems buggy. >> Will it be better to untie this routine from srp_target_port at all? > Hello Sagi, > > Had you noticed that target->req_ring_size is not modified during > resource allocation or reallocation ? That is why target->req_ring_size > has not been converted into a function argument in this patch. Yes, I guess I'm fine with that. Reviewed-by: Sagi Grimberg <sagig@mellanox.com> -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" 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/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c index ba434d6..1c4b0d3 100644 --- a/drivers/infiniband/ulp/srp/ib_srp.c +++ b/drivers/infiniband/ulp/srp/ib_srp.c @@ -574,17 +574,18 @@ static void srp_disconnect_target(struct srp_target_port *target) } } -static void srp_free_req_data(struct srp_target_port *target) +static void srp_free_req_data(struct srp_target_port *target, + struct srp_request *req_ring) { struct ib_device *ibdev = target->srp_host->srp_dev->dev; struct srp_request *req; int i; - if (!target->req_ring) + if (!req_ring) return; for (i = 0; i < target->req_ring_size; ++i) { - req = &target->req_ring[i]; + req = &req_ring[i]; kfree(req->fmr_list); kfree(req->map_page); if (req->indirect_dma_addr) { @@ -595,27 +596,34 @@ static void srp_free_req_data(struct srp_target_port *target) kfree(req->indirect_desc); } - kfree(target->req_ring); - target->req_ring = NULL; + kfree(req_ring); } +/** + * srp_alloc_req_data() - allocate or reallocate request data + * @target: SRP target port. + * + * If target->req_ring was non-NULL before this function got invoked it will + * also be non-NULL after this function has finished. + */ static int srp_alloc_req_data(struct srp_target_port *target) { struct srp_device *srp_dev = target->srp_host->srp_dev; struct ib_device *ibdev = srp_dev->dev; - struct srp_request *req; + struct list_head free_reqs; + struct srp_request *req_ring, *req; dma_addr_t dma_addr; int i, ret = -ENOMEM; - INIT_LIST_HEAD(&target->free_reqs); + INIT_LIST_HEAD(&free_reqs); - target->req_ring = kzalloc(target->req_ring_size * - sizeof(*target->req_ring), GFP_KERNEL); - if (!target->req_ring) + req_ring = kzalloc(target->req_ring_size * sizeof(*req_ring), + GFP_KERNEL); + if (!req_ring) goto out; for (i = 0; i < target->req_ring_size; ++i) { - req = &target->req_ring[i]; + req = &req_ring[i]; req->fmr_list = kmalloc(target->cmd_sg_cnt * sizeof(void *), GFP_KERNEL); req->map_page = kmalloc(SRP_FMR_SIZE * sizeof(void *), @@ -632,11 +640,16 @@ static int srp_alloc_req_data(struct srp_target_port *target) req->indirect_dma_addr = dma_addr; req->index = i; - list_add_tail(&req->list, &target->free_reqs); + list_add_tail(&req->list, &free_reqs); } + swap(target->req_ring, req_ring); + INIT_LIST_HEAD(&target->free_reqs); + list_splice(&free_reqs, &target->free_reqs); ret = 0; out: + srp_free_req_data(target, req_ring); + return ret; } @@ -669,7 +682,7 @@ static void srp_remove_target(struct srp_target_port *target) srp_free_target_ib(target); cancel_work_sync(&target->tl_err_work); srp_rport_put(target->rport); - srp_free_req_data(target); + srp_free_req_data(target, target->req_ring); spin_lock(&target->srp_host->target_lock); list_del(&target->list); @@ -2750,7 +2763,7 @@ err_free_ib: srp_free_target_ib(target); err_free_mem: - srp_free_req_data(target); + srp_free_req_data(target, target->req_ring); err: scsi_host_put(target_host);
This patch is needed by the patch that adds fast registration support. Signed-off-by: Bart Van Assche <bvanassche@acm.org> Cc: Roland Dreier <roland@purestorage.com> Cc: David Dillow <dave@thedillows.org> Cc: Sagi Grimberg <sagig@mellanox.com> Cc: Vu Pham <vu@mellanox.com> Cc: Sebastian Parschauer <sebastian.riemer@profitbricks.com> --- drivers/infiniband/ulp/srp/ib_srp.c | 41 ++++++++++++++++++++++++------------- 1 file changed, 27 insertions(+), 14 deletions(-)