IB SRP initiator patches for kernel 3.11

drivers/scsi/scsi_transport_srp.c struct srp_internal { · line 79

Quoted 8 lines · Bart Van Assche

drivers/scsi/scsi_transport_srp.c +8 -0

+int srp_tmo_valid(int fast_io_fail_tmo, int dev_loss_tmo)
+{
+	return (fast_io_fail_tmo < 0 || dev_loss_tmo < 0 ||
+		fast_io_fail_tmo < dev_loss_tmo) &&
+		fast_io_fail_tmo <= SCSI_DEVICE_BLOCK_MAX_TIMEOUT &&
+		dev_loss_tmo < LONG_MAX / HZ ? 0 : -EINVAL;
+}
+EXPORT_SYMBOL_GPL(srp_tmo_valid);

This would have been more readable:

int srp_tmo_valid(int fast_io_fail_tmp, int dev_loss_tmo)

{
	/* Fast IO fail must be off, or no greater than the max timeout */
	if (fast_io_fail_tmo > SCSI_DEVICE_BLOCK_MAX_TIMEOUT)
		return -EINVAL;

	/* Device timeout must be off, or fit into jiffies */
	if (dev_loss_tmo >= LONG_MAX / HZ)
		return -EINVAL;

	/* Fast IO must trigger before device loss, or one of the
	 * timeouts must be disabled.

	 */
	if (fast_io_fail_tmo < 0 || dev_loss_tmo < 0)
		return 0;
	if (fast_io_fail < dev_loss_tmo)
		return 0;

	return -EINVAL;	
}

Though, now that I've unpacked it -- I don't think it is OK for dev_loss_tmo to be off, but fast IO to be on? That drops another conditional.

Also, FC caps dev_loss_tmo at SCSI_DEVICE_BLOCK_MAX_TIMEOUT if fail_io_fast_tmo is off; I agree with your reasoning about leaving it unlimited if fast fail is on, but does that still hold if it is off?

Quoted 30 lines · David Dillow

drivers/scsi/scsi_transport_srp.c struct srp_internal { · line 79

Quoted 8 lines · Bart Van Assche

drivers/scsi/scsi_transport_srp.c +8 -0

+int srp_tmo_valid(int fast_io_fail_tmo, int dev_loss_tmo)
+{
+	return (fast_io_fail_tmo < 0 || dev_loss_tmo < 0 ||
+		fast_io_fail_tmo < dev_loss_tmo) &&
+		fast_io_fail_tmo <= SCSI_DEVICE_BLOCK_MAX_TIMEOUT &&
+		dev_loss_tmo < LONG_MAX / HZ ? 0 : -EINVAL;
+}
+EXPORT_SYMBOL_GPL(srp_tmo_valid);

This would have been more readable:

int srp_tmo_valid(int fast_io_fail_tmp, int dev_loss_tmo)

{
	/* Fast IO fail must be off, or no greater than the max timeout */
	if (fast_io_fail_tmo > SCSI_DEVICE_BLOCK_MAX_TIMEOUT)
		return -EINVAL;

	/* Device timeout must be off, or fit into jiffies */
	if (dev_loss_tmo >= LONG_MAX / HZ)
		return -EINVAL;

	/* Fast IO must trigger before device loss, or one of the
	 * timeouts must be disabled.
	 */
	if (fast_io_fail_tmo < 0 || dev_loss_tmo < 0)
		return 0;
	if (fast_io_fail < dev_loss_tmo)
		return 0;

Quoted 2 lines

	return -EINVAL;	
}

Isn't that a matter of personal taste which of the above two is more clear ?

It might also depend on the number of mathematics courses in someones educational background :-)

Quoted 3 lines

Though, now that I've unpacked it -- I don't think it is OK for dev_loss_tmo to be off, but fast IO to be on? That drops another conditional.

The combination of dev_loss_tmo off and reconnect_delay > 0 worked fine in my tests. An I/O failure was detected shortly after the cable to the target was pulled. I/O resumed shortly after the cable to the target was reinserted.

Quoted 3 lines

Also, FC caps dev_loss_tmo at SCSI_DEVICE_BLOCK_MAX_TIMEOUT if fail_io_fast_tmo is off; I agree with your reasoning about leaving it unlimited if fast fail is on, but does that still hold if it is off?

I think setting dev_loss_tmo to a large value only makes sense if the value of reconnect_delay is not too large. Setting both to a large value would result in slow recovery after a transport layer failure has been corrected.

Quoted 4 lines · Bart Van Assche

The combination of dev_loss_tmo off and reconnect_delay > 0 worked fine in my tests. An I/O failure was detected shortly after the cable to the target was pulled. I/O resumed shortly after the cable to the target was reinserted.

Perhaps I don't understand your answer -- I'm asking about dev_loss_tmo < 0, and fast_io_fail_tmo >= 0. The other transports do not allow this scenario, and I'm asking if it makes sense for SRP to allow it.

But now that you mention reconnect_delay, what is the meaning of that when it is negative? That's not in the documentation. And should it be considered in srp_tmo_valid() -- are there values of reconnect_delay that cause problems?

I'm starting to get a bit concerned about this patch -- can you, Vu, and Sebastian comment on the testing you have done?

Quoted 8 lines

Quoted 3 lines

Also, FC caps dev_loss_tmo at SCSI_DEVICE_BLOCK_MAX_TIMEOUT if fail_io_fast_tmo is off; I agree with your reasoning about leaving it unlimited if fast fail is on, but does that still hold if it is off?

I think setting dev_loss_tmo to a large value only makes sense if the value of reconnect_delay is not too large. Setting both to a large value would result in slow recovery after a transport layer failure has been corrected.

So you agree it should be capped? I can't tell from your response.

Quoted 10 lines · David Dillow

Quoted 4 lines · Bart Van Assche

The combination of dev_loss_tmo off and reconnect_delay > 0 worked fine in my tests. An I/O failure was detected shortly after the cable to the target was pulled. I/O resumed shortly after the cable to the target was reinserted.

Perhaps I don't understand your answer -- I'm asking about dev_loss_tmo < 0, and fast_io_fail_tmo >= 0. The other transports do not allow this scenario, and I'm asking if it makes sense for SRP to allow it.

Quoted 4 lines

But now that you mention reconnect_delay, what is the meaning of that when it is negative? That's not in the documentation. And should it be considered in srp_tmo_valid() -- are there values of reconnect_delay that cause problems?

None of the combinations that can be configured from user space can bring the kernel in trouble. If reconnect_delay <= 0 that means that the time-based reconnect mechanism is disabled.

Quoted 13 lines · Bart Van Assche

Quoted 11 lines · David Dillow

Quoted 5 lines · Bart Van Assche

The combination of dev_loss_tmo off and reconnect_delay > 0 worked fine in my tests. An I/O failure was detected shortly after the cable to the target was pulled. I/O resumed shortly after the cable to the target was reinserted.

Perhaps I don't understand your answer -- I'm asking about dev_loss_tmo < 0, and fast_io_fail_tmo >= 0. The other transports do not allow this scenario, and I'm asking if it makes sense for SRP to allow it.

Quoted 7 lines

Quoted 5 lines

But now that you mention reconnect_delay, what is the meaning of that when it is negative? That's not in the documentation. And should it be considered in srp_tmo_valid() -- are there values of reconnect_delay that cause problems?

None of the combinations that can be configured from user space can bring the kernel in trouble. If reconnect_delay <= 0 that means that the time-based reconnect mechanism is disabled.

Then it should use the same semantics as the other attributes, and have the user store "off" to turn it off.

And I'm getting the strong sense that the answer to my question about fast_io_fail_tmo >= 0 when dev_loss_tmo is that we should not allow that combination, even if it doesn't break the kernel. If it doesn't make sense, there is no reason to create an opportunity for user confusion.

And I'm getting the strong sense that the answer to my question about fast_io_fail_tmo >= 0 when dev_loss_tmo is that we should not allow that combination, even if it doesn't break the kernel. If it doesn't make sense, there is no reason to create an opportunity for user confusion.

Quoted 4 lines · David Dillow

And I'm getting the strong sense that the answer to my question about fast_io_fail_tmo >= 0 when dev_loss_tmo is that we should not allow that combination, even if it doesn't break the kernel. If it doesn't make sense, there is no reason to create an opportunity for user confusion.

Let's take a step back. I think we agree that the only combinations of timeout parameters that are useful are those combinations that guarantee that SCSI commands will be finished in a reasonable time and also that allow multipath to detect failed paths. The first requirement comes down to limiting the value fast_io_fail_tmo can be set to. The second requirement means that either reconnect_delay or fast_io_fail_tmo must be set (please note that a reconnect failure changes the state of all involved SCSI devices into SDEV_TRANSPORT_OFFLINE). So how about modifying srp_tmo_valid() as follows:
* Add an argument called "reconnect_delay".
* Add the following code in that function:

     if (reconnect_delay < 0 && fast_io_fail_tmo < 0 && dev_loss_tmo < 0)
         return -EINVAL;

Quoted 4 lines · David Dillow

And I'm getting the strong sense that the answer to my question about fast_io_fail_tmo >= 0 when dev_loss_tmo is that we should not allow that combination, even if it doesn't break the kernel. If it doesn't make sense, there is no reason to create an opportunity for user confusion.

Let's take a step back. I think we agree that the only combinations of timeout parameters that are useful are those combinations that guarantee that SCSI commands will be finished in a reasonable time and also that allow multipath to detect failed paths. The first requirement comes down to limiting the value fast_io_fail_tmo can be set to. The second requirement means that either reconnect_delay or fast_io_fail_tmo must be set (please note that a reconnect failure changes the state of all involved SCSI devices into SDEV_TRANSPORT_OFFLINE). So how about modifying srp_tmo_valid() as follows:
* Add an argument called "reconnect_delay".
* Add the following code in that function:

     if (reconnect_delay < 0 && fast_io_fail_tmo < 0 && dev_loss_tmo < 0)
         return -EINVAL;

Quoted 8 lines

Quoted 6 lines

Though, now that I've unpacked it -- I don't think it is OK for dev_loss_tmo to be off, but fast IO to be on? That drops another conditional.

The combination of dev_loss_tmo off and reconnect_delay > 0 worked fine in my tests. An I/O failure was detected shortly after the cable to the target was pulled. I/O resumed shortly after the cable to the target was reinserted.

Perhaps I don't understand your answer -- I'm asking about dev_loss_tmo < 0, and fast_io_fail_tmo >= 0. The other transports do not allow this scenario, and I'm asking if it makes sense for SRP to allow it.

But now that you mention reconnect_delay, what is the meaning of that when it is negative? That's not in the documentation. And should it be considered in srp_tmo_valid() -- are there values of reconnect_delay that cause problems?

I'm starting to get a bit concerned about this patch -- can you, Vu, and Sebastian comment on the testing you have done?

After running cable pull test on two local IB links for several hrs, I/Os got stuck.
Further commands "multipath -ll" or "fdisk -l" got stuck and never return Here are the stack dump for srp-x kernel threads.
I'll run with #DEBUG to get more debug info on scsi host & rport

Finish all outstanding I/O requests after fast_io_fail_tmo expired, which speeds up failover in a multipath setup. This patch is a reworked version of a patch from Sebastian Riemer.

Reported-by Sebastian Riemer <[email protected]>

Signed-off-by Bart Van Assche <[email protected]>

Acked-by David Dillow <[email protected]>

Quoted 7 lines · Bart Van Assche

Finish all outstanding I/O requests after fast_io_fail_tmo expired, which speeds up failover in a multipath setup. This patch is a reworked version of a patch from Sebastian Riemer.

Reported-by Sebastian Riemer <[email protected]>

Signed-off-by Bart Van Assche <[email protected]>

Acked-by David Dillow <[email protected]>

I don't believe I ack'd this; I don't want the callers doing the result shift, do it in srp_finish_req().

Having it in the caller has the
advantage that the compiler can optimize the shift operation out because the number that is being shifted left is a constant.

And if later on it
would be necessary to set more fields of the SCSI result in a caller of srp_finish_req() then that will be possible without having to modify the srp_finish_req() function itself.

Quoted 3 lines · Bart Van Assche

Having it in the caller has the
advantage that the compiler can optimize the shift operation out because the number that is being shifted left is a constant.

srp_finish_req() is likely to be inlined, so the compiler will be able to make this optimization. Regardless, this is so far in the noise that it looses to readability.

Several InfiniBand HCA's allow to configure the completion vector per queue pair. This allows to spread the workload created by IB completion interrupts over multiple MSI-X vectors and hence over multiple CPU cores. In other words, configuring the completion vector properly not only allows to reduce latency on an initiator connected to multiple SRP targets but also allows to improve throughput.

Signed-off-by Bart Van Assche <[email protected]>

Acked-by David Dillow <[email protected]>

Cc Roland Dreier <[email protected]>

Cc Vu Pham <vu-VPRAkNaXOzVWk0Htik3J/[email protected]>

Cc Sebastian Riemer <[email protected]>

---
  Documentation/ABI/stable/sysfs-driver-ib_srp |    7 +++++++
  drivers/infiniband/ulp/srp/ib_srp.c          |   26 ++++++++++++++++++++++++--
  drivers/infiniband/ulp/srp/ib_srp.h          |    1 +
  3 files changed, 32 insertions(+), 2 deletions(-)

Documentation/ABI/stable/sysfs-driver-ib_srp +7 -0

index 481aae9..5c53d28 100644
@@ -54,6 +54,13 @@ Description:	Interface for making ib_srp connect to a new target.
  		  ib_srp. Specifying a value that exceeds cmd_sg_entries is
  		  only safe with partial memory descriptor list support enabled
  		  (allow_ext_sg=1).
+		* comp_vector, a number in the range 0..n-1 specifying the
+		  MSI-X completion vector. Some HCA's allocate multiple (n)
+		  MSI-X vectors per HCA port. If the IRQ affinity masks of
+		  these interrupts have been configured such that each MSI-X
+		  interrupt is handled by a different CPU then the comp_vector
+		  parameter can be used to spread the SRP completion workload
+		  over multiple CPU's.
 
  What:		/sys/class/infiniband_srp/srp-<hca>-<port_number>/ibdev
  Date:		January 2, 2006

drivers/infiniband/ulp/srp/ib_srp.c +24 -2

index 2557b7a..6c164f6 100644
@@ -294,14 +294,16 @@ static int srp_create_target_ib(struct srp_target_port *target)
  		return -ENOMEM;
 
  	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,
+			       target->comp_vector);
  	if (IS_ERR(recv_cq)) {
  		ret = PTR_ERR(recv_cq);
  		goto err;
  	}
 
  	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,
+			       target->comp_vector);
  	if (IS_ERR(send_cq)) {
  		ret = PTR_ERR(send_cq);
  		goto err_recv_cq;

@@ -1976,6 +1978,14 @@ static ssize_t show_local_ib_device(struct device *dev,
  	return sprintf(buf, "%s\n", target->srp_host->srp_dev->dev->name);
  }
 
+static ssize_t show_comp_vector(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct srp_target_port *target = host_to_target(class_to_shost(dev));
+
+	return sprintf(buf, "%d\n", target->comp_vector);
+}
+
  static ssize_t show_cmd_sg_entries(struct device *dev,
  				   struct device_attribute *attr, char *buf)
  {

@@ -2002,6 +2012,7 @@ static DEVICE_ATTR(req_lim,         S_IRUGO, show_req_lim,         NULL);
  static DEVICE_ATTR(zero_req_lim,    S_IRUGO, show_zero_req_lim,	   NULL);
  static DEVICE_ATTR(local_ib_port,   S_IRUGO, show_local_ib_port,   NULL);
  static DEVICE_ATTR(local_ib_device, S_IRUGO, show_local_ib_device, NULL);
+static DEVICE_ATTR(comp_vector,     S_IRUGO, show_comp_vector,     NULL);
  static DEVICE_ATTR(cmd_sg_entries,  S_IRUGO, show_cmd_sg_entries,  NULL);
  static DEVICE_ATTR(allow_ext_sg,    S_IRUGO, show_allow_ext_sg,    NULL);
 

@@ -2016,6 +2027,7 @@ static struct device_attribute *srp_host_attrs[] = {
  	&dev_attr_zero_req_lim,
  	&dev_attr_local_ib_port,
  	&dev_attr_local_ib_device,
+	&dev_attr_comp_vector,
  	&dev_attr_cmd_sg_entries,
  	&dev_attr_allow_ext_sg,
  	NULL

@@ -2140,6 +2152,7 @@ enum {
  	SRP_OPT_CMD_SG_ENTRIES	= 1 << 9,
  	SRP_OPT_ALLOW_EXT_SG	= 1 << 10,
  	SRP_OPT_SG_TABLESIZE	= 1 << 11,
+	SRP_OPT_COMP_VECTOR	= 1 << 12,
  	SRP_OPT_ALL		= (SRP_OPT_ID_EXT	|
  				   SRP_OPT_IOC_GUID	|
  				   SRP_OPT_DGID		|

@@ -2160,6 +2173,7 @@ static const match_table_t srp_opt_tokens = {
  	{ SRP_OPT_CMD_SG_ENTRIES,	"cmd_sg_entries=%u"	},
  	{ SRP_OPT_ALLOW_EXT_SG,		"allow_ext_sg=%u"	},
  	{ SRP_OPT_SG_TABLESIZE,		"sg_tablesize=%u"	},
+	{ SRP_OPT_COMP_VECTOR,		"comp_vector=%u"	},
  	{ SRP_OPT_ERR,			NULL 			}
  };
 

@@ -2315,6 +2329,14 @@ static int srp_parse_options(const char *buf, struct srp_target_port *target)
  			target->sg_tablesize = token;
  			break;
 
+		case SRP_OPT_COMP_VECTOR:
+			if (match_int(args, &token) || token < 0) {
+				pr_warn("bad comp_vector parameter '%s'\n", p);
+				goto out;
+			}
+			target->comp_vector = token;
+			break;
+
  		default:
  			pr_warn("unknown parameter or missing value '%s' in target creation request\n",
  				p);

drivers/infiniband/ulp/srp/ib_srp.h +0 -0

index e45d9d0..cbc0b14 100644

Quoted 7 lines · Bart Van Assche

Several InfiniBand HCA's allow to configure the completion vector per queue pair. This allows to spread the workload created by IB completion interrupts over multiple MSI-X vectors and hence over multiple CPU cores. In other words, configuring the completion vector properly not only allows to reduce latency on an initiator connected to multiple SRP targets but also allows to improve throughput.

Hey Bart,
Just wrote a small patch to allow srp_daemon spread connection across HCA's completion vectors.
But re-thinking on this, is it really a good idea to give the user control over completion
vectors for CQs he doesn't really owns. This way the user must retrieve the maximum completion
vectors from the ib_device and consider this when adding a connection and In addition will need to set proper IRQ affinity.

Perhaps the driver can manage this on it's own without involving the user, take the mlx4_en driver for
example, it spreads it's CQs across HCAs completion vectors without involving the user. the user that
opens a socket has no influence of the underlying cq<->comp-vector assignment.

The only use-case I can think of is where the user will want to use only a subset of the completion-vectors
if the user will want to reserve some completion-vectors for native IB applications but I don't know
how common it is.

Other from that, I think it is always better to spread the CQs across HCA completion-vectors, so perhaps the driver
just assign connection CQs across comp-vecs without getting args from the user, but simply iterate over comp_vectors.

What do you think?

Quoted 32 lines · Sagi Grimberg

Quoted 7 lines · Bart Van Assche

Several InfiniBand HCA's allow to configure the completion vector per queue pair. This allows to spread the workload created by IB completion interrupts over multiple MSI-X vectors and hence over multiple CPU cores. In other words, configuring the completion vector properly not only allows to reduce latency on an initiator connected to multiple SRP targets but also allows to improve throughput.

Hey Bart,
Just wrote a small patch to allow srp_daemon spread connection across HCA's completion vectors.
But re-thinking on this, is it really a good idea to give the user control over completion
vectors for CQs he doesn't really owns. This way the user must retrieve the maximum completion
vectors from the ib_device and consider this when adding a connection and In addition will need to set proper IRQ affinity.

Perhaps the driver can manage this on it's own without involving the user, take the mlx4_en driver for
example, it spreads it's CQs across HCAs completion vectors without involving the user. the user that
opens a socket has no influence of the underlying cq<->comp-vector assignment.

The only use-case I can think of is where the user will want to use only a subset of the completion-vectors
if the user will want to reserve some completion-vectors for native IB applications but I don't know
how common it is.

Quoted 6 lines

Other from that, I think it is always better to spread the CQs across HCA completion-vectors, so perhaps the driver
just assign connection CQs across comp-vecs without getting args from the user, but simply iterate over comp_vectors.

What do you think?

Hello Sagi,

Sorry but I do not think it is a good idea to let srp_daemon assign the completion vector. While this might work well on single-socket systems this will result in suboptimal results on NUMA systems. For certain workloads on NUMA systems, and when a NUMA initiator system is connected to multiple target systems, the optimal configuration is to make sure that all processing that is associated with a single SCSI host occurs on the same NUMA node. This means configuring the completion vector value such that IB interrupts are generated on the same NUMA node where the associated SCSI host and applications are running.

More in general, performance tuning on NUMA systems requires system-wide knowledge of all applications that are running and also of which interrupt is processed by which NUMA node. So choosing a proper value for the completion vector is only possible once the system topology and the IRQ affinity masks are known. I don't think we should build knowledge of all this in srp_daemon.

Bart.

srp_daemon is a package designated for the customer to automatically detect targets in the IB fabric. From our experience here in Mellanox, customers/users like automatic "plug&play" tools.
They are reluctant to build their own scriptology to enhance performance and settle with srp_daemon which is preferred over use of ibsrpdm and manual adding new targets.
Regardless, the completion vectors assignment is meaningless without setting proper IRQ affinity, so in the worst case where the user didn't set his IRQ affinity,
this assignment will perform like the default completion vector assignment as all IRQs are directed without any masking i.e. core 0.

From my experiments in NUMA systems, optimal performance is gained where all IRQs are directed to half of the cores on the NUMA node close to the HCA, and all traffic generators share the other half of the cores on the same NUMA node. So based on that knowledge, I thought that srp_daemon/srp driver will assign it's CQs across the HCAs completion vectors, and the user is encouraged to set the IRQ affinity as described above to gain optimal performance.
Adding connections over the far NUMA node don't seem to benefit performance too much...

As I mentioned, a use-case I see that may raise a problem here, is if the user would like to maintain multiple SRP connections and reserve some completion vectors for other IB applications on the system. in this case the user will be able to disable srp_daemon/srp driver completion vectors assignment.

So, this was just an idea, and easy implementation that would potentially give the user semi-automatic performance optimized configuration...

Quoted 28 lines · Sagi Grimberg

srp_daemon is a package designated for the customer to automatically detect targets in the IB fabric. From our experience here in Mellanox, customers/users like automatic "plug&play" tools.
They are reluctant to build their own scriptology to enhance performance and settle with srp_daemon which is preferred over use of ibsrpdm and manual adding new targets.
Regardless, the completion vectors assignment is meaningless without setting proper IRQ affinity, so in the worst case where the user didn't set his IRQ affinity,
this assignment will perform like the default completion vector assignment as all IRQs are directed without any masking i.e. core 0.

From my experiments in NUMA systems, optimal performance is gained where all IRQs are directed to half of the cores on the NUMA node close to the HCA, and all traffic generators share the other half of the cores on the same NUMA node. So based on that knowledge, I thought that srp_daemon/srp driver will assign it's CQs across the HCAs completion vectors, and the user is encouraged to set the IRQ affinity as described above to gain optimal performance.
Adding connections over the far NUMA node don't seem to benefit performance too much...

As I mentioned, a use-case I see that may raise a problem here, is if the user would like to maintain multiple SRP connections and reserve some completion vectors for other IB applications on the system. in this case the user will be able to disable srp_daemon/srp driver completion vectors assignment.

Quoted 3 lines

So, this was just an idea, and easy implementation that would potentially give the user semi-automatic performance optimized configuration...

Hello Sagi,

I agree with you that it would help a lot if completion vector assignment could be automated such that end users do not have to care about assigning completion vector numbers. The challenge is to find an approach that is general enough such that it works for all possible use cases. One possible approach is to let a tool that has knowledge about the application fill in completion vector numbers in srp_daemon.conf and let srp_daemon use the values generated by this tool. That approach would avoid that srp_daemon has to have any knowledge about the application but would still allow srp_daemon to assign the completion vector numbers.

Bart.

This sounds like a nice Idea, but there an inherent problem about applications coming and going while the connections are static (somewhat), how can you control pinning an arbitrary application running (over SRP devices of-course) at certain point of time.

So will you agree at least to give target->comp_vector a default of IB_CQ_VECTOR_LEAST_ATTACHED?
From my point of view, a user that don't have a slightest clue about
completion vectors and performance optimization, this is somewhat better than doing nothing...

Quoted 10 lines · Sagi Grimberg

This sounds like a nice Idea, but there an inherent problem about applications coming and going while the connections are static (somewhat), how can you control pinning an arbitrary application running (over SRP devices of-course) at certain point of time.

So will you agree at least to give target->comp_vector a default of IB_CQ_VECTOR_LEAST_ATTACHED?
From my point of view, a user that don't have a slightest clue about
completion vectors and performance optimization, this is somewhat better than doing nothing...

Hello Sagi,

That sounds like an interesting proposal to me. But did the patch that adds the IB_CQ_VECTOR_LEAST_ATTACHED feature ever get accepted in the upstream Linux kernel ? I have tried to find that symbol in Linux kernel v3.11-rc1 but couldn't find it. Maybe I have overlooked something ?

Bart.

Just wrote a small patch to allow srp_daemon spread connection across HCA's completion vectors.

Quoted 2 lines · Sagi Grimberg

Just wrote a small patch to allow srp_daemon spread connection across HCA's completion vectors.

Hello Sagi,

How about the following approach:
- Add support for reading the completion vector from srp_daemon.conf, similar to how several other parameters are already read from that file.

- If the completion vector parameter has not been set in srp_daemon.conf, let srp_daemon assign a completion vector such that IB interrupts for different SRP hosts use different completion vectors.

Bart.

Allow the InfiniBand RC retry count to be configured by the user as an option in the target login string. Reducing this retry count helps with reducing path failover time.

[bvanassche: Rewrote patch description / changed default retry count from 2 back to 7]

The individual patches in this series are as follows:
0001-IB-srp-Fix-remove_one-crash-due-to-resource-exhausti.patch 0002-IB-srp-Fix-race-between-srp_queuecommand-and-srp_cla.patch 0003-IB-srp-Avoid-that-srp_reset_host-is-skipped-after-a-.patch 0004-IB-srp-Fail-I-O-fast-if-target-offline.patch 0005-IB-srp-Skip-host-settle-delay.patch 0006-IB-srp-Maintain-a-single-connection-per-I_T-nexus.patch 0007-IB-srp-Keep-rport-as-long-as-the-IB-transport-layer.patch

0008-scsi_transport_srp-Add-transport-layer-error-handlin.patch 0009-IB-srp-Add-srp_terminate_io.patch 0010-IB-srp-Use-SRP-transport-layer-error-recovery.patch 0011-IB-srp-Start-timers-if-a-transport-layer-error-occur.patch 0012-IB-srp-Fail-SCSI-commands-silently.patch 0013-IB-srp-Make-HCA-completion-vector-configurable.patch 0014-IB-srp-Make-transport-layer-retry-count-configurable.patch 0015-IB-srp-Bump-driver-version-and-release-date.patch

Some of these patches were already picked by Roland (SB), I would suggest that you post V4 and drop the ones which were accepted.