On 05/16/2012 07:03 PM, David Dillow wrote: > On Wed, 2012-05-16 at 11:54 -0400, Bernd Schubert wrote: >> 2) Low SRP command queues. Is there a reason why >> SRP_RQ_SHIFT/SRP_RQ_SIZE and their depend values such as SRP_RQ_SIZE are >> so small? > > That's a decision that has been around since the beginning of the driver > as far as I can tell. It looks to be a balance between device needs and > memory usage, and it can certainly be raised -- I've run locally with > SRP_RQ_SHIFT set to 7 (shost.can_queue 126) and I'm sure 8 would be no > problem, either. I didn't see a performance improvement on my workload, > but may you will. > > Because we take the minimum of our initiator queue depth and the initial > credits from the target (each req consumes a credit), going higher than > 8 doesn't buy us much, as I don't know off-hand of any target that gives > out more than 256 credits. > > The memory used for the command ring will vary depending on the value of > SRP_RQ_SHIFT and the number of s/g entries allows to be put in the > command. 255 s/g entries requires an 8 KB allocation for each request > (~4200 bytes), so we currently require 512 KB of buffers for the send > queue for each target. Going to 8 will require 2 MB max per target, > which probably isn't a real issue. > > There's also a response ring with an allocation size that depends on the > target, but those should be pretty small buffers, say 1 KB * (1<< > SRP_RQ_SHIFT). >
David, below is a first version to convert SRP_RQ_SHIFT into a new module option "srp_rq_size". I already tested it, but I also need to re-read it myself. Author: Bernd Schubert <[email protected]> Date: Mon May 21 10:25:01 2012 +0200 infiniband/srp: convert SRP_RQ_SHIFT into a module parameter When SRP_RQ_SHIFT is a fix parameter the optimal value is unclear. The current value of 6 might be too small for several storage systems and larger values might take too much memory for other systems. So make it a parameter and let the admin decide. Signed-off-by: Bernd Schubert <[email protected]> diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c index 0bfa545..67fa47f 100644 --- a/drivers/infiniband/ulp/srp/ib_srp.c +++ b/drivers/infiniband/ulp/srp/ib_srp.c @@ -62,6 +62,7 @@ MODULE_LICENSE("Dual BSD/GPL"); static unsigned int srp_sg_tablesize; static unsigned int cmd_sg_entries; static unsigned int indirect_sg_entries; +static unsigned int srp_rq_size, srp_sq_size, srp_cmd_sq_size; static bool allow_ext_sg; static int topspin_workarounds = 1; @@ -76,6 +77,9 @@ module_param(indirect_sg_entries, uint, 0444); MODULE_PARM_DESC(indirect_sg_entries, "Default max number of gather/scatter entries (default is 12, max is " __stringify(SCSI_MAX_SG_CHAIN_SEGMENTS) ")"); +module_param(srp_rq_size, uint, 0444); +MODULE_PARM_DESC(srp_sg_tablesize, "Request queue size (default is 64, max is " __stringify(SRP_RQ_MAX) ")"); + module_param(allow_ext_sg, bool, 0444); MODULE_PARM_DESC(allow_ext_sg, "Default behavior when there are more than cmd_sg_entries S/G entries after mapping; fails the request when false (default false)"); @@ -227,14 +231,14 @@ static int srp_create_target_ib(struct srp_target_port *target) return -ENOMEM; target->recv_cq = ib_create_cq(target->srp_host->srp_dev->dev, - srp_recv_completion, NULL, target, SRP_RQ_SIZE, 0); + srp_recv_completion, NULL, target, srp_rq_size, 0); if (IS_ERR(target->recv_cq)) { ret = PTR_ERR(target->recv_cq); goto err; } target->send_cq = ib_create_cq(target->srp_host->srp_dev->dev, - srp_send_completion, NULL, target, SRP_SQ_SIZE, 0); + srp_send_completion, NULL, target, srp_sq_size, 0); if (IS_ERR(target->send_cq)) { ret = PTR_ERR(target->send_cq); goto err_recv_cq; @@ -243,8 +247,8 @@ static int srp_create_target_ib(struct srp_target_port *target) ib_req_notify_cq(target->recv_cq, IB_CQ_NEXT_COMP); init_attr->event_handler = srp_qp_event; - init_attr->cap.max_send_wr = SRP_SQ_SIZE; - init_attr->cap.max_recv_wr = SRP_RQ_SIZE; + init_attr->cap.max_send_wr = srp_sq_size; + init_attr->cap.max_recv_wr = srp_rq_size; init_attr->cap.max_recv_sge = 1; init_attr->cap.max_send_sge = 1; init_attr->sq_sig_type = IB_SIGNAL_ALL_WR; @@ -287,9 +291,9 @@ static void srp_free_target_ib(struct srp_target_port *target) ib_destroy_cq(target->send_cq); ib_destroy_cq(target->recv_cq); - for (i = 0; i < SRP_RQ_SIZE; ++i) + for (i = 0; i < srp_rq_size; ++i) srp_free_iu(target->srp_host, target->rx_ring[i]); - for (i = 0; i < SRP_SQ_SIZE; ++i) + for (i = 0; i < srp_sq_size; ++i) srp_free_iu(target->srp_host, target->tx_ring[i]); } @@ -460,7 +464,7 @@ static void srp_free_req_data(struct srp_target_port *target) struct srp_request *req; int i; - for (i = 0, req = target->req_ring; i < SRP_CMD_SQ_SIZE; ++i, ++req) { + for (i = 0, req = target->req_ring; i < srp_cmd_sq_size; ++i, ++req) { kfree(req->fmr_list); kfree(req->map_page); if (req->indirect_dma_addr) { @@ -472,6 +476,41 @@ static void srp_free_req_data(struct srp_target_port *target) } } +/** + * Free target rings + */ +static void srp_free_target_rings(struct srp_target_port *target) +{ + kfree(target->tx_ring); + kfree(target->rx_ring); + kfree(target->req_ring); +} + +/** + * Target ring allocations with a size depending on module options + */ +static int srp_alloc_target_rings(struct srp_target_port *target) +{ + target->tx_ring = kzalloc(srp_sq_size * sizeof(**(target->tx_ring)), GFP_KERNEL); + if (!target->tx_ring) + return -ENOMEM; + + target->rx_ring = kzalloc(srp_rq_size * sizeof(**(target->rx_ring)), GFP_KERNEL); + if (!target->rx_ring) { + kfree(target->tx_ring); + return -ENOMEM; + } + + target->req_ring = kzalloc(srp_cmd_sq_size * sizeof(*(target->req_ring)), GFP_KERNEL); + if (!target->req_ring) { + kfree(target->tx_ring); + kfree(target->rx_ring); + return -ENOMEM; + } + + return 0; +} + static void srp_remove_work(struct work_struct *work) { struct srp_target_port *target = @@ -489,6 +528,7 @@ static void srp_remove_work(struct work_struct *work) ib_destroy_cm_id(target->cm_id); srp_free_target_ib(target); srp_free_req_data(target); + srp_free_target_rings(target); scsi_host_put(target->scsi_host); } @@ -620,14 +660,14 @@ static int srp_reconnect_target(struct srp_target_port *target) while (ib_poll_cq(target->send_cq, 1, &wc) > 0) ; /* nothing */ - for (i = 0; i < SRP_CMD_SQ_SIZE; ++i) { + for (i = 0; i < srp_cmd_sq_size; ++i) { struct srp_request *req = &target->req_ring[i]; if (req->scmnd) srp_reset_req(target, req); } INIT_LIST_HEAD(&target->free_tx); - for (i = 0; i < SRP_SQ_SIZE; ++i) + for (i = 0; i < srp_sq_size; ++i) list_add(&target->tx_ring[i]->list, &target->free_tx); target->qp_in_error = 0; @@ -969,7 +1009,7 @@ static void srp_put_tx_iu(struct srp_target_port *target, struct srp_iu *iu, * Note: * An upper limit for the number of allocated information units for each * request type is: - * - SRP_IU_CMD: SRP_CMD_SQ_SIZE, since the SCSI mid-layer never queues + * - SRP_IU_CMD: srp_cmd_sq_size, since the SCSI mid-layer never queues * more than Scsi_Host.can_queue requests. * - SRP_IU_TSK_MGMT: SRP_TSK_MGMT_SQ_SIZE. * - SRP_IU_RSP: 1, since a conforming SRP target never sends more than @@ -1320,7 +1360,7 @@ static int srp_alloc_iu_bufs(struct srp_target_port *target) { int i; - for (i = 0; i < SRP_RQ_SIZE; ++i) { + for (i = 0; i < srp_rq_size; ++i) { target->rx_ring[i] = srp_alloc_iu(target->srp_host, target->max_ti_iu_len, GFP_KERNEL, DMA_FROM_DEVICE); @@ -1328,7 +1368,7 @@ static int srp_alloc_iu_bufs(struct srp_target_port *target) goto err; } - for (i = 0; i < SRP_SQ_SIZE; ++i) { + for (i = 0; i < srp_sq_size; ++i) { target->tx_ring[i] = srp_alloc_iu(target->srp_host, target->max_iu_len, GFP_KERNEL, DMA_TO_DEVICE); @@ -1341,12 +1381,12 @@ static int srp_alloc_iu_bufs(struct srp_target_port *target) return 0; err: - for (i = 0; i < SRP_RQ_SIZE; ++i) { + for (i = 0; i < srp_rq_size; ++i) { srp_free_iu(target->srp_host, target->rx_ring[i]); target->rx_ring[i] = NULL; } - for (i = 0; i < SRP_SQ_SIZE; ++i) { + for (i = 0; i < srp_sq_size; ++i) { srp_free_iu(target->srp_host, target->tx_ring[i]); target->tx_ring[i] = NULL; } @@ -1401,7 +1441,7 @@ static void srp_cm_rep_handler(struct ib_cm_id *cm_id, if (ret) goto error_free; - for (i = 0; i < SRP_RQ_SIZE; i++) { + for (i = 0; i < srp_rq_size; i++) { struct srp_iu *iu = target->rx_ring[i]; ret = srp_post_recv(target, iu); if (ret) @@ -1649,7 +1689,7 @@ static int srp_reset_device(struct scsi_cmnd *scmnd) if (target->tsk_mgmt_status) return FAILED; - for (i = 0; i < SRP_CMD_SQ_SIZE; ++i) { + for (i = 0; i < srp_cmd_sq_size; ++i) { struct srp_request *req = &target->req_ring[i]; if (req->scmnd && req->scmnd->device == scmnd->device) srp_reset_req(target, req); @@ -1841,9 +1881,9 @@ static struct scsi_host_template srp_template = { .eh_device_reset_handler = srp_reset_device, .eh_host_reset_handler = srp_reset_host, .sg_tablesize = SRP_DEF_SG_TABLESIZE, - .can_queue = SRP_CMD_SQ_SIZE, + .can_queue = SRP_CMD_SQ_DEF_SIZE, .this_id = -1, - .cmd_per_lun = SRP_CMD_SQ_SIZE, + .cmd_per_lun = SRP_CMD_SQ_DEF_SIZE, .use_clustering = ENABLE_CLUSTERING, .shost_attrs = srp_host_attrs }; @@ -2034,7 +2074,7 @@ static int srp_parse_options(const char *buf, struct srp_target_port *target) printk(KERN_WARNING PFX "bad max cmd_per_lun parameter '%s'\n", p); goto out; } - target->scsi_host->cmd_per_lun = min(token, SRP_CMD_SQ_SIZE); + target->scsi_host->cmd_per_lun = min_t(unsigned, token, srp_cmd_sq_size); break; case SRP_OPT_IO_CLASS: @@ -2131,9 +2171,15 @@ static ssize_t srp_create_target(struct device *dev, target_host->max_id = 1; target_host->max_lun = SRP_MAX_LUN; target_host->max_cmd_len = sizeof ((struct srp_cmd *) (void *) 0L)->cdb; + target_host->can_queue = srp_cmd_sq_size; + target_host->cmd_per_lun = srp_cmd_sq_size; target = host_to_target(target_host); + ret = srp_alloc_target_rings(target); + if (ret) + goto err; + target->io_class = SRP_REV16A_IB_IO_CLASS; target->scsi_host = target_host; target->srp_host = host; @@ -2163,7 +2209,7 @@ static ssize_t srp_create_target(struct device *dev, spin_lock_init(&target->lock); INIT_LIST_HEAD(&target->free_tx); INIT_LIST_HEAD(&target->free_reqs); - for (i = 0; i < SRP_CMD_SQ_SIZE; ++i) { + for (i = 0; i < srp_cmd_sq_size; ++i) { struct srp_request *req = &target->req_ring[i]; req->fmr_list = kmalloc(target->cmd_sg_cnt * sizeof (void *), @@ -2435,6 +2481,7 @@ static void srp_remove_one(struct ib_device *device) ib_destroy_cm_id(target->cm_id); srp_free_target_ib(target); srp_free_req_data(target); + srp_free_target_rings(target); scsi_host_put(target->scsi_host); } @@ -2479,6 +2526,17 @@ static int __init srp_init_module(void) indirect_sg_entries = cmd_sg_entries; } + if (!srp_rq_size) + srp_rq_size = SRP_RQ_DEF_SIZE; + + if (srp_rq_size > SRP_RQ_MAX) { + printk(KERN_WARNING PFX "Clamping srp_rq_size to %d\n", SRP_RQ_MAX); + srp_rq_size = SRP_RQ_MAX; + } + + srp_sq_size = srp_rq_size; + srp_cmd_sq_size = srp_sq_size - SRP_RSP_SQ_SIZE - SRP_TSK_MGMT_SQ_SIZE; + ib_srp_transport_template = srp_attach_transport(&ib_srp_transport_functions); if (!ib_srp_transport_template) diff --git a/drivers/infiniband/ulp/srp/ib_srp.h b/drivers/infiniband/ulp/srp/ib_srp.h index 020caf0..f2195fd 100644 --- a/drivers/infiniband/ulp/srp/ib_srp.h +++ b/drivers/infiniband/ulp/srp/ib_srp.h @@ -57,14 +57,13 @@ enum { SRP_MAX_LUN = 512, SRP_DEF_SG_TABLESIZE = 12, - SRP_RQ_SHIFT = 6, - SRP_RQ_SIZE = 1 << SRP_RQ_SHIFT, + SRP_RQ_DEF_SIZE = 64, + SRP_SQ_DEF_SIZE = SRP_RQ_DEF_SIZE, + SRP_RQ_MAX = 1024, - SRP_SQ_SIZE = SRP_RQ_SIZE, SRP_RSP_SQ_SIZE = 1, - SRP_REQ_SQ_SIZE = SRP_SQ_SIZE - SRP_RSP_SQ_SIZE, SRP_TSK_MGMT_SQ_SIZE = 1, - SRP_CMD_SQ_SIZE = SRP_REQ_SQ_SIZE - SRP_TSK_MGMT_SQ_SIZE, + SRP_CMD_SQ_DEF_SIZE = SRP_SQ_DEF_SIZE - SRP_RSP_SQ_SIZE - SRP_TSK_MGMT_SQ_SIZE, SRP_TAG_NO_REQ = ~0U, SRP_TAG_TSK_MGMT = 1U << 31, @@ -169,9 +168,9 @@ struct srp_target_port { int zero_req_lim; - struct srp_iu *tx_ring[SRP_SQ_SIZE]; - struct srp_iu *rx_ring[SRP_RQ_SIZE]; - struct srp_request req_ring[SRP_CMD_SQ_SIZE]; + struct srp_iu **tx_ring; /* table of srp_sq_size */ + struct srp_iu **rx_ring; /* table of srp_rq_size */ + struct srp_request *req_ring; /* table of srp_cmd_sq_size */ struct work_struct work; -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
