Re: [PATCH v2 08/15] scsi_transport_srp: Add transport layer error handling
On 06/30/13 23:05, David Dillow wrote:
On Fri, 2013-06-28 at 14:53 +0200, Bart Van Assche wrote:
+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 LONG_MAX / HZ
+ dev_loss_tmo LONG_MAX / HZ ? 0 : -EINVAL;
+}
They should also be capped by SCSI_DEVICE_BLOCK_MAX_TIMEOUT instead of
LONG_MAX / HZ, I think.
The fast_io_fail_tmo should indeed be capped by that value. However, I'm
not sure about dev_loss_tmo. I think there are several use cases (e.g.
initiator-side mirroring) where it's useful to set dev_loss_tmo to a
larger value than ten minutes.
+static ssize_t store_srp_rport_fast_io_fail_tmo(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct srp_rport *rport = transport_class_to_srp_rport(dev);
+ char ch[16], *p;
+ int res;
+ int fast_io_fail_tmo;
+
+ sprintf(ch, %.*s, min_t(int, sizeof(ch) - 1, count), buf);
+ p = strchr(ch, '\n');
+ if (p)
+ *p = '\0';
Again, no need for the sprintf if you don't modify the buffer? Instead
of using strchr() to make the strcmp() work with newlines, just do
if (!strcmp(buf, off) || !strcmp(buf, off\n)) {
fast_io_fail_tmo = 1;
} else {
res = kstrtoint(buf, 0, fast_io_fail_tmo);
...
instead?
Same comment applies for store_srp_rport_dev_loss_tmo().
OK, will remove the temporary char arrays.
Bart.
--
To unsubscribe from this list: send the line unsubscribe linux-rdma in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 02/15] IB/srp: Fix race between srp_queuecommand() and srp_claim_req()
On 06/30/13 21:59, David Dillow wrote: Let's push this patch to the back of the pile for any respin; I don't think the others rely on it (haven't gotten there yet) and would rather not hold them up for this. I'll drop this patch for now. It's a race I have not yet been able to trigger anyway. Bart. -- To unsubscribe from this list: send the line unsubscribe linux-rdma in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 14/15] IB/srp: Make transport layer retry count configurable
On 06/30/13 23:48, David Dillow wrote: On Fri, 2013-06-28 at 14:58 +0200, Bart Van Assche wrote: From: Vu Pham vuhu...@mellanox.com Allow the InfiniBand RC retry count to be configured by the user as an option in the target login string. The transport layer timeout in nanoseconds is computed as follows from the retry count: rc_timeout = rc_retry_count * 4 * 4096 * (1 qp-timeout) The default value for tl_retry_count is changed from 7 into 2. Hence with a qp-timedout value of 19 this patch reduces the default transport layer timeout from about 60s to about 17s. The purpose of this patch is to reduce the time needed for SCSI error handling significantly and at the same time to avoid activating the SCSI error handler on an IB path with a regular BER or due to brief IB network congestion. I keep vacillating between preserving the default of 7 and opting for easier/optimized configuration for the common case. It my internal argument over this today, I wondered about changing the QP timeout instead -- doesn't that achieve your goals of allowing for errors and network congestion while optimizing for a reasonable fabric? Going from 19 to 17 drops the timeout by about the same amount, while allowing for more errors. I agree that one or both of the items should be configurable, but I'm still worried about changing the defaults, given the feed back from those that want to use IB over the WAN. The InfiniBand specification mentions the following about differential receiver inputs (C6-11.2.1): A BER of 10^-12 shall be achieved when connected to the worst case transmitter through any compliant channel. The maximum packet size for an InfiniBand packet is about 4 KB (see also section 7.7.8 in the spec). This means that with an 8b/10b encoding the chance to lose a packet over a single link due to bit errors is about 4*10^-8. So the chance to lose a packet over a network consisting of n links with retry count r is about (n*4*10^-8)^r. With r=2 that results already in a really low value, even with multiple links. Since lowering the QP timeout might make congestion worse my preference is to lower the retry count. Bart. -- To unsubscribe from this list: send the line unsubscribe linux-rdma in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 04/15] IB/srp: Fail I/O fast if target offline
On 28.06.2013 14:49, Bart Van Assche wrote: If reconnecting failed we know that no command completion will be received anymore. Hence let the SCSI error handler fail such commands immediately. Signed-off-by: Bart Van Assche bvanass...@acm.org Cc: Roland Dreier rol...@purestorage.com Cc: David Dillow dillo...@ornl.gov Cc: Sebastian Riemer sebastian.rie...@profitbricks.com Cc: Vu Pham v...@mellanox.com --- drivers/infiniband/ulp/srp/ib_srp.c |2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c index 8c95262..5c91521 100644 --- a/drivers/infiniband/ulp/srp/ib_srp.c +++ b/drivers/infiniband/ulp/srp/ib_srp.c @@ -1755,6 +1755,8 @@ static int srp_abort(struct scsi_cmnd *scmnd) if (srp_send_tsk_mgmt(target, req-index, scmnd-device-lun, SRP_TSK_ABORT_TASK) == 0) ret = SUCCESS; + else if (target-transport_offline) + ret = FAST_IO_FAIL; else ret = FAILED; srp_free_req(target, req, scmnd, 0); This doesn't give us much speed advantage IMHO. The check for target-transport_offline should be before calling srp_send_tsk_mgmt(). This way it would also match the patch description better. Cheers, Sebastian -- To unsubscribe from this list: send the line unsubscribe linux-rdma in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 04/15] IB/srp: Fail I/O fast if target offline
On 28.06.2013 14:49, Bart Van Assche wrote: If reconnecting failed we know that no command completion will be received anymore. Hence let the SCSI error handler fail such commands immediately. Signed-off-by: Bart Van Assche bvanass...@acm.org Cc: Roland Dreier rol...@purestorage.com Cc: David Dillow dillo...@ornl.gov Cc: Sebastian Riemer sebastian.rie...@profitbricks.com Cc: Vu Pham v...@mellanox.com --- drivers/infiniband/ulp/srp/ib_srp.c |2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c index 8c95262..5c91521 100644 --- a/drivers/infiniband/ulp/srp/ib_srp.c +++ b/drivers/infiniband/ulp/srp/ib_srp.c @@ -1755,6 +1755,8 @@ static int srp_abort(struct scsi_cmnd *scmnd) if (srp_send_tsk_mgmt(target, req-index, scmnd-device-lun, SRP_TSK_ABORT_TASK) == 0) ret = SUCCESS; + else if (target-transport_offline) + ret = FAST_IO_FAIL; else ret = FAILED; srp_free_req(target, req, scmnd, 0); I'm also missing the concept for srp_reset_device(). There is a very common case that the SCSI error handling and the transport layer error handling run in parallel: Congestion. In congestion some LUNs are blocked while others can still transmit. A little bit later the QP timeout triggers in the middle of the SCSI error handling in srp_abort(), srp_reset_device() or less likely in srp_reset_host(). Cheers, Sebastian -- To unsubscribe from this list: send the line unsubscribe linux-rdma in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 14/15] IB/srp: Make transport layer retry count configurable
On Mon, 2013-07-01 at 10:18 +0200, Bart Van Assche wrote: On 06/30/13 23:48, David Dillow wrote: On Fri, 2013-06-28 at 14:58 +0200, Bart Van Assche wrote: From: Vu Pham vuhu...@mellanox.com Allow the InfiniBand RC retry count to be configured by the user as an option in the target login string. The transport layer timeout in nanoseconds is computed as follows from the retry count: rc_timeout = rc_retry_count * 4 * 4096 * (1 qp-timeout) The default value for tl_retry_count is changed from 7 into 2. Hence with a qp-timedout value of 19 this patch reduces the default transport layer timeout from about 60s to about 17s. The purpose of this patch is to reduce the time needed for SCSI error handling significantly and at the same time to avoid activating the SCSI error handler on an IB path with a regular BER or due to brief IB network congestion. I keep vacillating between preserving the default of 7 and opting for easier/optimized configuration for the common case. It my internal argument over this today, I wondered about changing the QP timeout instead -- doesn't that achieve your goals of allowing for errors and network congestion while optimizing for a reasonable fabric? Going from 19 to 17 drops the timeout by about the same amount, while allowing for more errors. I agree that one or both of the items should be configurable, but I'm still worried about changing the defaults, given the feed back from those that want to use IB over the WAN. The InfiniBand specification mentions the following about differential receiver inputs (C6-11.2.1): A BER of 10^-12 shall be achieved when connected to the worst case transmitter through any compliant channel. There are a _lot_ of networks out there with non-compliant channels, then. The spec says one thing, but the reality matters. Also, perhaps my math is wrong, but that seems to be one error every 18 seconds or so at FDR speeds? The maximum packet size for an InfiniBand packet is about 4 KB (see also section 7.7.8 in the spec). This means that with an 8b/10b encoding the chance to lose a packet over a single link due to bit errors is about 4*10^-8. So the chance to lose a packet over a network consisting of n links with retry count r is about (n*4*10^-8)^r. With r=2 that results already in a really low value, even with multiple links. Since lowering the QP timeout might make congestion worse my preference is to lower the retry count. You assume independent failures, which is suspect -- many times these are data-dependent, or so I tend to think. Jason, do you have any insight on this (overall) topic you could share? -- To unsubscribe from this list: send the line unsubscribe linux-rdma in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 04/15] IB/srp: Fail I/O fast if target offline
On 07/01/13 11:07, Sebastian Riemer wrote: On 28.06.2013 14:49, Bart Van Assche wrote: If reconnecting failed we know that no command completion will be received anymore. Hence let the SCSI error handler fail such commands immediately. Signed-off-by: Bart Van Assche bvanass...@acm.org Cc: Roland Dreier rol...@purestorage.com Cc: David Dillow dillo...@ornl.gov Cc: Sebastian Riemer sebastian.rie...@profitbricks.com Cc: Vu Pham v...@mellanox.com --- drivers/infiniband/ulp/srp/ib_srp.c |2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c index 8c95262..5c91521 100644 --- a/drivers/infiniband/ulp/srp/ib_srp.c +++ b/drivers/infiniband/ulp/srp/ib_srp.c @@ -1755,6 +1755,8 @@ static int srp_abort(struct scsi_cmnd *scmnd) if (srp_send_tsk_mgmt(target, req-index, scmnd-device-lun, SRP_TSK_ABORT_TASK) == 0) ret = SUCCESS; + else if (target-transport_offline) + ret = FAST_IO_FAIL; else ret = FAILED; srp_free_req(target, req, scmnd, 0); This doesn't give us much speed advantage IMHO. The check for target-transport_offline should be before calling srp_send_tsk_mgmt(). This way it would also match the patch description better. Hello Sebastian, Had you perhaps overlooked the following code at the start of srp_send_tsk_mgmt() ? if (!target-connected || target-qp_in_error) return -1; Given this I don't think it matters whether the transport_offline check occurs before or after the srp_send_tsk_mgmt() call. Bart. -- To unsubscribe from this list: send the line unsubscribe linux-rdma in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH for/net-next 3/8] net/mlx5: Mellanox Connect-IB, core driver part 3/3
On Saturday 29 June 2013 07:10, David Miller wrote:
From: Or Gerlitz ogerl...@mellanox.com
Date: Wed, 26 Jun 2013 17:22:12 +0300
+ for (--i; i = 0; --i) {
Please, i-- is more canonical in for() loops.
+ for (--i; i = 0; --i) {
Likewise.
--
To unsubscribe from this list: send the line unsubscribe linux-rdma in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Dave,
For the for loop initial value, i should be decremented before doing any
for-loop calculations (and it is not at all obvious if this is the ordering if
we use
i--, and not --i). Using --i in the initial value makes the ordering obvious.
However, I do agree with respect to the increment that --i and i-- are logically
identical.
Thus, the for loop could read:
for (--i; i = 0; i--) {
However, my own personal opinion is that this is a bit confusing.
I would prefer to leave these lines as they are.
Is that OK with you?
-Jack
--
To unsubscribe from this list: send the line unsubscribe linux-rdma in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 14/15] IB/srp: Make transport layer retry count configurable
On 07/01/13 13:26, David Dillow wrote: On Mon, 2013-07-01 at 10:18 +0200, Bart Van Assche wrote: The InfiniBand specification mentions the following about differential receiver inputs (C6-11.2.1): A BER of 10^-12 shall be achieved when connected to the worst case transmitter through any compliant channel. There are a _lot_ of networks out there with non-compliant channels, then. The spec says one thing, but the reality matters. Also, perhaps my math is wrong, but that seems to be one error every 18 seconds or so at FDR speeds? If data is transferred at line rate on an FDR network, I agree that about one error is expected about every 18 seconds. Sorry but it's not clear to me what makes you think that there are a lot of IB networks with non-compliant channels ? Bart. -- To unsubscribe from this list: send the line unsubscribe linux-rdma in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 04/15] IB/srp: Fail I/O fast if target offline
On 01.07.2013 13:33, Bart Van Assche wrote: --- a/drivers/infiniband/ulp/srp/ib_srp.c +++ b/drivers/infiniband/ulp/srp/ib_srp.c @@ -1755,6 +1755,8 @@ static int srp_abort(struct scsi_cmnd *scmnd) if (srp_send_tsk_mgmt(target, req-index, scmnd-device-lun, SRP_TSK_ABORT_TASK) == 0) ret = SUCCESS; +else if (target-transport_offline) +ret = FAST_IO_FAIL; else ret = FAILED; srp_free_req(target, req, scmnd, 0); This doesn't give us much speed advantage IMHO. The check for target-transport_offline should be before calling srp_send_tsk_mgmt(). This way it would also match the patch description better. Hello Sebastian, Had you perhaps overlooked the following code at the start of srp_send_tsk_mgmt() ? if (!target-connected || target-qp_in_error) return -1; Given this I don't think it matters whether the transport_offline check occurs before or after the srp_send_tsk_mgmt() call. Hi Bart, okay, right. So you get an error due to the connected and qp_in_error state first. Yes, I've overlooked that. Thanks! Cheers, Sebastian -- To unsubscribe from this list: send the line unsubscribe linux-rdma in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 08/15] scsi_transport_srp: Add transport layer error handling
On Mon, 2013-07-01 at 09:01 +0200, Bart Van Assche wrote:
On 06/30/13 23:05, David Dillow wrote:
On Fri, 2013-06-28 at 14:53 +0200, Bart Van Assche wrote:
+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 LONG_MAX / HZ
+ dev_loss_tmo LONG_MAX / HZ ? 0 : -EINVAL;
+}
They should also be capped by SCSI_DEVICE_BLOCK_MAX_TIMEOUT instead of
LONG_MAX / HZ, I think.
The fast_io_fail_tmo should indeed be capped by that value. However, I'm
not sure about dev_loss_tmo. I think there are several use cases (e.g.
initiator-side mirroring) where it's useful to set dev_loss_tmo to a
larger value than ten minutes.
Ah, yes, very good point.
--
To unsubscribe from this list: send the line unsubscribe linux-rdma in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 04/15] IB/srp: Fail I/O fast if target offline
On 01.07.2013 13:38, Bart Van Assche wrote: --- a/drivers/infiniband/ulp/srp/ib_srp.c +++ b/drivers/infiniband/ulp/srp/ib_srp.c @@ -1755,6 +1755,8 @@ static int srp_abort(struct scsi_cmnd *scmnd) if (srp_send_tsk_mgmt(target, req-index, scmnd-device-lun, SRP_TSK_ABORT_TASK) == 0) ret = SUCCESS; +else if (target-transport_offline) +ret = FAST_IO_FAIL; else ret = FAILED; srp_free_req(target, req, scmnd, 0); I'm also missing the concept for srp_reset_device(). There is a very common case that the SCSI error handling and the transport layer error handling run in parallel: Congestion. Can you explain this comment further, and also how this comment relates to patch 04/15 ? Sorry, found it. Even if only one srp_reset_device() fails, then srp_reset_host() is called anyway. So there this check + returning FAST_IO_FAIL doesn't make so much sense. In congestion some LUNs are blocked while others can still transmit. A little bit later the QP timeout triggers in the middle of the SCSI error handling in srp_abort(), srp_reset_device() or less likely in srp_reset_host(). I am aware this can result in concurrent srp_reconnect_rport() calls. However, such concurrent calls are serialized via rport-mutex. I put my comment regarding this to patch 10. Cheers, Sebastian -- To unsubscribe from this list: send the line unsubscribe linux-rdma in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 04/15] IB/srp: Fail I/O fast if target offline
On 07/01/13 14:31, Sebastian Riemer wrote: On 01.07.2013 13:38, Bart Van Assche wrote: --- a/drivers/infiniband/ulp/srp/ib_srp.c +++ b/drivers/infiniband/ulp/srp/ib_srp.c @@ -1755,6 +1755,8 @@ static int srp_abort(struct scsi_cmnd *scmnd) if (srp_send_tsk_mgmt(target, req-index, scmnd-device-lun, SRP_TSK_ABORT_TASK) == 0) ret = SUCCESS; +else if (target-transport_offline) +ret = FAST_IO_FAIL; else ret = FAILED; srp_free_req(target, req, scmnd, 0); I'm also missing the concept for srp_reset_device(). There is a very common case that the SCSI error handling and the transport layer error handling run in parallel: Congestion. Can you explain this comment further, and also how this comment relates to patch 04/15 ? Sorry, found it. Even if only one srp_reset_device() fails, then srp_reset_host() is called anyway. So there this check + returning FAST_IO_FAIL doesn't make so much sense. Hello Sebastian, I agree that if one or more srp_abort() calls return FAILED that srp_reset_host() will be called anyway. However, this patch helps if the first call of srp_abort() occurs after a reconnect failure, which is likely with the default reconnnect_delay and SCSI timeout settings. In that case all srp_abort() calls will return FAST_IO_FAIL, scsi_eh_abort_cmds() will terminate the pending commands and hence the host reset will be skipped. Bart. -- To unsubscribe from this list: send the line unsubscribe linux-rdma in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] libibverbs: Allow arbitrary int values for MTU
Keep IBV_MTU_* enums values as they are, but pass MTU values around as
a struct containing a single int. Per lengthy discusson on the
linux-rdma list, this patch introdces a source code incompatibility.
Although legacy applications can continue to use the enum values, they
will need to be updated to use the struct. Newer applications are
encouraged to use arbitrary int values, not the MTU enums (e.g., 1024,
1500, 9000).
(if people like the idea of this patch, I will send the corresponding
kernel patch)
Signed-off-by: Jeff Squyres jsquy...@cisco.com
---
Makefile.am| 3 ++-
examples/devinfo.c | 20 +++
examples/pingpong.c| 12 -
examples/pingpong.h| 1 -
examples/rc_pingpong.c | 10
examples/srq_pingpong.c| 10
examples/uc_pingpong.c | 10
examples/ud_pingpong.c | 2 +-
include/infiniband/verbs.h | 61 +++---
man/ibv_modify_qp.3| 2 +-
man/ibv_query_port.3 | 4 +--
man/ibv_query_qp.3 | 2 +-
src/cmd.c | 8 +++---
src/marshall.c | 2 +-
14 files changed, 89 insertions(+), 58 deletions(-)
diff --git a/Makefile.am b/Makefile.am
index 40e83be..1159e55 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -54,7 +54,8 @@ man_MANS = man/ibv_asyncwatch.1 man/ibv_devices.1
man/ibv_devinfo.1 \
man/ibv_post_srq_recv.3 man/ibv_query_device.3 man/ibv_query_gid.3 \
man/ibv_query_pkey.3 man/ibv_query_port.3 man/ibv_query_qp.3 \
man/ibv_query_srq.3 man/ibv_rate_to_mult.3 man/ibv_reg_mr.3
\
-man/ibv_req_notify_cq.3 man/ibv_resize_cq.3 man/ibv_rate_to_mbps.3
+man/ibv_req_notify_cq.3 man/ibv_resize_cq.3 man/ibv_rate_to_mbps.3 \
+man/ibv_mtu_to_num.3
DEBIAN = debian/changelog debian/compat debian/control debian/copyright \
debian/ibverbs-utils.install debian/libibverbs1.install \
diff --git a/examples/devinfo.c b/examples/devinfo.c
index ff078e4..e8fb27e 100644
--- a/examples/devinfo.c
+++ b/examples/devinfo.c
@@ -111,18 +111,6 @@ static const char *atomic_cap_str(enum ibv_atomic_cap
atom_cap)
}
}
-static const char *mtu_str(enum ibv_mtu max_mtu)
-{
- switch (max_mtu) {
- case IBV_MTU_256: return 256;
- case IBV_MTU_512: return 512;
- case IBV_MTU_1024: return 1024;
- case IBV_MTU_2048: return 2048;
- case IBV_MTU_4096: return 4096;
- default: return invalid MTU;
- }
-}
-
static const char *width_str(uint8_t width)
{
switch (width) {
@@ -301,10 +289,10 @@ static int print_hca_cap(struct ibv_device *ib_dev,
uint8_t ib_port)
printf(\t\tport:\t%d\n, port);
printf(\t\t\tstate:\t\t\t%s (%d)\n,
port_state_str(port_attr.state), port_attr.state);
- printf(\t\t\tmax_mtu:\t\t%s (%d)\n,
- mtu_str(port_attr.max_mtu), port_attr.max_mtu);
- printf(\t\t\tactive_mtu:\t\t%s (%d)\n,
- mtu_str(port_attr.active_mtu), port_attr.active_mtu);
+ printf(\t\t\tmax_mtu:\t\t%d (%d)\n,
+ ibv_mtu_to_num(port_attr.max_mtu),
port_attr.max_mtu.mtu);
+ printf(\t\t\tactive_mtu:\t\t%d (%d)\n,
+ ibv_mtu_to_num(port_attr.active_mtu),
port_attr.active_mtu.mtu);
printf(\t\t\tsm_lid:\t\t\t%d\n, port_attr.sm_lid);
printf(\t\t\tport_lid:\t\t%d\n, port_attr.lid);
printf(\t\t\tport_lmc:\t\t0x%02x\n, port_attr.lmc);
diff --git a/examples/pingpong.c b/examples/pingpong.c
index 90732ef..d1c22c9 100644
--- a/examples/pingpong.c
+++ b/examples/pingpong.c
@@ -36,18 +36,6 @@
#include stdio.h
#include string.h
-enum ibv_mtu pp_mtu_to_enum(int mtu)
-{
- switch (mtu) {
- case 256: return IBV_MTU_256;
- case 512: return IBV_MTU_512;
- case 1024: return IBV_MTU_1024;
- case 2048: return IBV_MTU_2048;
- case 4096: return IBV_MTU_4096;
- default: return -1;
- }
-}
-
uint16_t pp_get_local_lid(struct ibv_context *context, int port)
{
struct ibv_port_attr attr;
diff --git a/examples/pingpong.h b/examples/pingpong.h
index 9cdc03e..91d217b 100644
--- a/examples/pingpong.h
+++ b/examples/pingpong.h
@@ -35,7 +35,6 @@
#include infiniband/verbs.h
-enum ibv_mtu pp_mtu_to_enum(int mtu);
uint16_t pp_get_local_lid(struct ibv_context *context, int port);
int pp_get_port_info(struct ibv_context *context, int port,
struct ibv_port_attr *attr);
diff --git a/examples/rc_pingpong.c b/examples/rc_pingpong.c
index 15494a1..a7e1836 100644
--- a/examples/rc_pingpong.c
+++ b/examples/rc_pingpong.c
@@ -78,7 +78,7 @@ struct pingpong_dest {
};
static int pp_connect_ctx(struct pingpong_context *ctx, int port, int my_psn,
- enum ibv_mtu mtu, int sl,
+ struct ibv_mtu_t mtu, int sl,
Re: [PATCH for-next 0/8] Add Mellanox mlx5 driver for Connect-IB devices
In general I don't think overriding the CFLAGS (as you do in the mlx5
Makefiles) is a good idea, and in particular here your -Wall -Werror
break the build, at least for my gcc 4.7.3:
CC drivers/infiniband/hw/mlx5/qp.o
/home/roland/Src/linux-merge.git/drivers/infiniband/hw/mlx5/qp.c: In
function ‘sq_overhead’:
/home/roland/Src/linux-merge.git/drivers/infiniband/hw/mlx5/qp.c:234:2:
error: case value ‘4671’ not in enumerated type ‘enum ib_qp_type’
[-Werror=switch]
/home/roland/Src/linux-merge.git/drivers/infiniband/hw/mlx5/qp.c: In
function ‘qp_has_rq’:
/home/roland/Src/linux-merge.git/drivers/infiniband/hw/mlx5/qp.c:334:20:
error: comparison between ‘enum ib_qp_type’ and ‘enum anonymous’
[-Werror=enum-compare]
/home/roland/Src/linux-merge.git/drivers/infiniband/hw/mlx5/qp.c: In
function ‘to_mlx5_st’:
/home/roland/Src/linux-merge.git/drivers/infiniband/hw/mlx5/qp.c:464:2:
error: case value ‘4671’ not in enumerated type ‘enum ib_qp_type’
[-Werror=switch]
/home/roland/Src/linux-merge.git/drivers/infiniband/hw/mlx5/qp.c: In
function ‘create_kernel_qp’:
/home/roland/Src/linux-merge.git/drivers/infiniband/hw/mlx5/qp.c:613:25:
error: comparison between ‘enum ib_qp_type’ and ‘enum anonymous’
[-Werror=enum-compare]
/home/roland/Src/linux-merge.git/drivers/infiniband/hw/mlx5/qp.c: In
function ‘create_qp_common’:
/home/roland/Src/linux-merge.git/drivers/infiniband/hw/mlx5/qp.c:815:25:
error: comparison between ‘enum ib_qp_type’ and ‘enum anonymous’
[-Werror=enum-compare]
/home/roland/Src/linux-merge.git/drivers/infiniband/hw/mlx5/qp.c: In
function ‘get_cqs’:
/home/roland/Src/linux-merge.git/drivers/infiniband/hw/mlx5/qp.c:975:2:
error: case value ‘4671’ not in enumerated type ‘enum ib_qp_type’
[-Werror=switch]
/home/roland/Src/linux-merge.git/drivers/infiniband/hw/mlx5/qp.c: In
function ‘ib_qp_type_str’:
/home/roland/Src/linux-merge.git/drivers/infiniband/hw/mlx5/qp.c:1064:2:
error: case value ‘4671’ not in enumerated type ‘enum ib_qp_type’
[-Werror=switch]
/home/roland/Src/linux-merge.git/drivers/infiniband/hw/mlx5/qp.c: In
function ‘mlx5_ib_create_qp’:
/home/roland/Src/linux-merge.git/drivers/infiniband/hw/mlx5/qp.c:1086:26:
error: comparison between ‘enum ib_qp_type’ and ‘enum anonymous’
[-Werror=enum-compare]
/home/roland/Src/linux-merge.git/drivers/infiniband/hw/mlx5/qp.c:1113:2:
error: case value ‘4671’ not in enumerated type ‘enum ib_qp_type’
[-Werror=switch]
/home/roland/Src/linux-merge.git/drivers/infiniband/hw/mlx5/qp.c: In
function ‘__mlx5_ib_modify_qp’:
/home/roland/Src/linux-merge.git/drivers/infiniband/hw/mlx5/qp.c:1452:20:
error: comparison between ‘enum ib_qp_type’ and ‘enum anonymous’
[-Werror=enum-compare]
/home/roland/Src/linux-merge.git/drivers/infiniband/hw/mlx5/qp.c: In
function ‘mlx5_ib_modify_qp’:
/home/roland/Src/linux-merge.git/drivers/infiniband/hw/mlx5/qp.c:1615:20:
error: comparison between ‘enum ib_qp_type’ and ‘enum anonymous’
[-Werror=enum-compare]
/home/roland/Src/linux-merge.git/drivers/infiniband/hw/mlx5/qp.c: In
function ‘mlx5_ib_post_send’:
/home/roland/Src/linux-merge.git/drivers/infiniband/hw/mlx5/qp.c:2166:19:
error: comparison between ‘enum ib_wr_opcode’ and ‘enum anonymous’
[-Werror=enum-compare]
/home/roland/Src/linux-merge.git/drivers/infiniband/hw/mlx5/qp.c:2165:3:
error: case value ‘4671’ not in enumerated type ‘enum ib_qp_type’
[-Werror=switch]
What is this IB_QPT_REG_UMR stuff anyway? Shouldn't we strip out all
that from the mlx5 driver until it's available in the core code?
Especially stuff like
/*
* we do not expose this yet so we use a value out of range */
enum {
IB_QPT_REG_UMR = IB_QPT_MAX + 0x1234,
};
/* === this should be passed to the vergbs layer */
enum {
IB_WR_SET_PSV = IB_WR_BIND_MW + 10,
IB_WR_GET_PSV,
IB_WR_CHECK_PSV,
IB_WR_RGET_PSV,
IB_WR_RCHECK_PSV,
IB_WR_UMR,
};
enum {
IB_SEND_UMR_UNREG = IB_SEND_IP_CSUM 1,
};
enum ib_latency_class {
IB_LATENCY_CLASS_LOW,
IB_LATENCY_CLASS_MEDIUM,
IB_LATENCY_CLASS_HIGH,
IB_LATENCY_CLASS_FAST_PATH
};
/* === this should be passed to the vergbs layer */
looks like it shouldn't be in your submission. (What are vergbs anyway? :)
--
To unsubscribe from this list: send the line unsubscribe linux-rdma in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH for-next 0/8] Add Mellanox mlx5 driver for Connect-IB devices
On Mon, Jul 1, 2013 at 11:03 AM, Joe Perches j...@perches.com wrote: There's some value in block enabling/disabling messages that dynamic_debug doesn't currently offer. As far as I can see, the mlx5 stuff ends up being per-file. Which dynamic debug already does offer. - R. -- To unsubscribe from this list: send the line unsubscribe linux-rdma in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH for/net-next 3/8] net/mlx5: Mellanox Connect-IB, core driver part 3/3
From: Jack Morgenstein ja...@dev.mellanox.co.il
Date: Mon, 1 Jul 2013 14:44:54 +0300
Thus, the for loop could read:
for (--i; i = 0; i--) {
However, my own personal opinion is that this is a bit confusing.
I would prefer to leave these lines as they are.
Is that OK with you?
Please adjust the right-most increment.
--
To unsubscribe from this list: send the line unsubscribe linux-rdma in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH for/net-next 3/8] net/mlx5: Mellanox Connect-IB, core driver part 3/3
From: Jack Morgenstein ja...@dev.mellanox.co.il
Date: Mon, 1 Jul 2013 14:44:54 +0300
On Saturday 29 June 2013 07:10, David Miller wrote:
From: Or Gerlitz ogerl...@mellanox.com
Date: Wed, 26 Jun 2013 17:22:12 +0300
+ for (--i; i = 0; --i) {
Please, i-- is more canonical in for() loops.
+ for (--i; i = 0; --i) {
Likewise.
--
To unsubscribe from this list: send the line unsubscribe linux-rdma in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Dave,
For the for loop initial value, i should be decremented before doing any
for-loop calculations (and it is not at all obvious if this is the ordering
if we use
i--, and not --i). Using --i in the initial value makes the ordering obvious.
However, I do agree with respect to the increment that --i and i-- are
logically
identical.
Thus, the for loop could read:
for (--i; i = 0; i--) {
However, my own personal opinion is that this is a bit confusing.
I would prefer to leave these lines as they are.
Is that OK with you?
Actually, you should adjust both decrements to read i--.
Look, if someone doesn't grok that the leftmost decrement happens
before any of the loop body or tests, they don't understand how
for() loops work.
And you're syntax is just confusing people who actually _do_
understand how this part of the C language operates.
--
To unsubscribe from this list: send the line unsubscribe linux-rdma in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH for-next 0/8] Add Mellanox mlx5 driver for Connect-IB devices
Also, sparse warns about the following. It seems there's an endianness bug in int mlx5_ib_umem_populate_pas(struct mlx5_ib_dev *dev, struct ib_umem *umem, int npages, u64 *pas) in mem.c, since it doesn't match what void mlx5_ib_populate_pas(struct mlx5_ib_dev *dev, struct ib_umem *umem, int page_shift, __be64 *pas, int umr) does, nor does it match the declaration int mlx5_ib_umem_populate_pas(struct mlx5_ib_dev *dev, struct ib_umem *umem, int npages, __be64 *pas); in mlx5_ib.h. Nor does it have any callers, so it's a bit hard to tell if it's really and truly a bug. - R. -- To unsubscribe from this list: send the line unsubscribe linux-rdma in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH for-next 0/8] Add Mellanox mlx5 driver for Connect-IB devices
On Mon, 2013-07-01 at 14:20 -0700, Roland Dreier wrote:
On Mon, Jul 1, 2013 at 1:19 PM, Joe Perches j...@perches.com wrote:
I think these are the groupings.
+enum {
+ MLX5_MOD_MAIN,
+ MLX5_MOD_CMDIF,
+ MLX5_MOD_EQ,
+ MLX5_MOD_QP,
+ MLX5_MOD_PGALLOC,
+ MLX5_MOD_FW,
+ MLX5_MOD_UAR,
+ MLX5_MOD_ALLOC,
+ MLX5_MOD_DEBUG,
+ MLX5_MOD_HEALTH,
+ MLX5_MOD_MAD,
+ MLX5_MOD_MCG,
+ MLX5_MOD_MR,
+ MLX5_MOD_PD,
+ MLX5_MOD_PORT,
+ MLX5_MOD_SRQ,
+ MLX5_MOD_CQ,
+ MLX5_MOD_CMD_DATA, /* print command payload only */
+ MLX5_CMD_DATA_TIME,
+};
Right, but then look how they're used. For example,
drivers/net/ethernet/mellanox/mlx5/core/main.c has:
MLX5_MOD_DBG_MASK(MLX5_MOD_MAIN);
so MLX5_MOD_MAIN just means messages in main.c, etc.
Thanks. OK, yeah, that's completely unnecessary.
Just using dynamic_debug is far better.
--
To unsubscribe from this list: send the line unsubscribe linux-rdma in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
