[PATCH V3] SA Busy Handling

[PATCH V3] SA Busy Handling

[Mike Heinz]

[-- Attachment #1: Type: text/plain, Size: 5058 bytes --]

Fixed the loss of the trap repress handling, which must have gotten lost somewhere between May and now...

Hal - your formatting nit, when I looked at the patch file, I see the correct spacing around the '&&' in the if statement.

---
This patch provides a new module parameter for ib_mad that tells ib_mad to treat BUSY responses as timeouts.

parm:           treat_busy_as_timeout:When true, treat BUSY responses as if they were timeouts. (int)

When this parameter is true, ib_mad will unilaterally treat BUSY responses as timeouts regardless of the desired behavior of the caller.

In addition, this patch also provides changes to ib_mad_send_buf to allow senders of MAD requests to request this behavior even if the parameter is not set. The patch does NOT provide changes to ib_umad to allow user apps to select this functionality.

Signed-off-by: Michael Heinz <michael.heinz-h88ZbnxC6KDQT0dZR+AlfA@public.gmane.org>
---

 drivers/infiniband/core/mad.c      |   22 ++++++++++++++++++++++
 drivers/infiniband/core/mad_priv.h |    1 +
 include/rdma/ib_mad.h              |   10 ++++++++++
 3 files changed, 33 insertions(+), 0 deletions(-)

diff --git a/drivers/infiniband/core/mad.c b/drivers/infiniband/core/mad.c
index 64e660c..2e103ed 100644
--- a/drivers/infiniband/core/mad.c
+++ b/drivers/infiniband/core/mad.c
@@ -54,6 +54,10 @@ MODULE_PARM_DESC(send_queue_size, "Size of send queue in number of work requests
 module_param_named(recv_queue_size, mad_recvq_size, int, 0444);
 MODULE_PARM_DESC(recv_queue_size, "Size of receive queue in number of work requests");
 
+int mad_wait_on_busy = 0;
+module_param_named(treat_busy_as_timeout, mad_wait_on_busy, int, 0444);
+MODULE_PARM_DESC(treat_busy_as_timeout, "When true, treat BUSY responses as if they were timeouts.");
+
 static struct kmem_cache *ib_mad_cache;
 
 static struct list_head ib_mad_port_list;
@@ -1120,6 +1124,7 @@ int ib_post_send_mad(struct ib_mad_send_buf *send_buf,
 		mad_send_wr->timeout = msecs_to_jiffies(send_buf->timeout_ms);
 		mad_send_wr->max_retries = send_buf->retries;
 		mad_send_wr->retries_left = send_buf->retries;
+		mad_send_wr->wait_on_busy = send_buf->wait_on_busy || mad_wait_on_busy;
 		send_buf->retries = 0;
 		/* Reference for work request to QP + response */
 		mad_send_wr->refcount = 1 + (mad_send_wr->timeout > 0);
@@ -1828,6 +1833,9 @@ static void ib_mad_complete_recv(struct ib_mad_agent_private *mad_agent_priv,
 
 	/* Complete corresponding request */
 	if (ib_response_mad(mad_recv_wc->recv_buf.mad)) {
+		u16 busy = be16_to_cpu(mad_recv_wc->recv_buf.mad->mad_hdr.status) &
+							   IB_MGMT_MAD_STATUS_BUSY;
+
 		spin_lock_irqsave(&mad_agent_priv->lock, flags);
 		mad_send_wr = ib_find_send_mad(mad_agent_priv, mad_recv_wc);
 		if (!mad_send_wr) {
@@ -1836,6 +1844,20 @@ static void ib_mad_complete_recv(struct ib_mad_agent_private *mad_agent_priv,
 			deref_mad_agent(mad_agent_priv);
 			return;
 		}
+
+		printk(KERN_DEBUG PFX "Completing recv %p: busy = %d, retries_left = %d, wait_on_busy = %d\n",
+			   mad_send_wr, busy, mad_send_wr->retries_left, mad_send_wr->wait_on_busy);
+
+		if (busy && mad_send_wr->retries_left && mad_send_wr->wait_on_busy &&
+		   (mad_recv_wc->recv_buf.mad->mad_hdr.method != IB_MGMT_METHOD_TRAP_REPRESS)) {
+			/* Just let the query timeout and have it requeued later */
+			spin_unlock_irqrestore(&mad_agent_priv->lock, flags);
+			ib_free_recv_mad(mad_recv_wc);
+			deref_mad_agent(mad_agent_priv);
+			printk(KERN_INFO PFX "SA/SM responded MAD_STATUS_BUSY. Allowing request to time out.\n");
+			return;
+		}
+
 		ib_mark_mad_done(mad_send_wr);
 		spin_unlock_irqrestore(&mad_agent_priv->lock, flags);
 
diff --git a/drivers/infiniband/core/mad_priv.h b/drivers/infiniband/core/mad_priv.h
index 8b4df0a..b6477cf 100644
--- a/drivers/infiniband/core/mad_priv.h
+++ b/drivers/infiniband/core/mad_priv.h
@@ -135,6 +135,7 @@ struct ib_mad_send_wr_private {
 	unsigned long timeout;
 	int max_retries;
 	int retries_left;
+	int wait_on_busy;
 	int retry;
 	int refcount;
 	enum ib_wc_status status;
diff --git a/include/rdma/ib_mad.h b/include/rdma/ib_mad.h
index d3b9401..fadc2c3 100644
--- a/include/rdma/ib_mad.h
+++ b/include/rdma/ib_mad.h
@@ -77,6 +77,15 @@
 
 #define IB_MGMT_MAX_METHODS			128
 
+/* MAD Status field bit masks */
+#define IB_MGMT_MAD_STATUS_SUCCESS                      0x0000
+#define IB_MGMT_MAD_STATUS_BUSY                         0x0001
+#define IB_MGMT_MAD_STATUS_REDIRECT_REQD                0x0002
+#define IB_MGMT_MAD_STATUS_BAD_VERERSION                0x0004
+#define IB_MGMT_MAD_STATUS_UNSUPPORTED_METHOD           0x0008
+#define IB_MGMT_MAD_STATUS_UNSUPPORTED_METHOD_ATTRIB    0x000c
+#define IB_MGMT_MAD_STATUS_INVALID_ATTRIB_VALUE         0x001c
+
 /* RMPP information */
 #define IB_MGMT_RMPP_VERSION			1
 
@@ -246,6 +255,7 @@ struct ib_mad_send_buf {
 	int			seg_count;
 	int			seg_size;
 	int			timeout_ms;
+	int			wait_on_busy;
 	int			retries;
 };


[-- Attachment #2: sa_busy_20101202.patch --]
[-- Type: application/octet-stream, Size: 4797 bytes --]

This patch provides a new module parameter for ib_mad that tells ib_mad to treat BUSY responses as 
timeouts.

parm:           treat_busy_as_timeout:When true, treat BUSY responses as if they were timeouts. (int)

When this parameter is true, ib_mad will unilaterally treat BUSY responses as timeouts regardless 
of the desired behavior of the caller.

In addition, this patch also provides changes to ib_mad_send_buf to allow senders of MAD requests
to request this behavior even if the parameter is not set. The patch does NOT provide changes to 
ib_umad to allow user apps to select this functionality.

Signed-off-by: Michael Heinz <michael.heinz@qlogic.com>
---

 drivers/infiniband/core/mad.c      |   22 ++++++++++++++++++++++
 drivers/infiniband/core/mad_priv.h |    1 +
 include/rdma/ib_mad.h              |   10 ++++++++++
 3 files changed, 33 insertions(+), 0 deletions(-)

diff --git a/drivers/infiniband/core/mad.c b/drivers/infiniband/core/mad.c
index 64e660c..2e103ed 100644
--- a/drivers/infiniband/core/mad.c
+++ b/drivers/infiniband/core/mad.c
@@ -54,6 +54,10 @@ MODULE_PARM_DESC(send_queue_size, "Size of send queue in number of work requests
 module_param_named(recv_queue_size, mad_recvq_size, int, 0444);
 MODULE_PARM_DESC(recv_queue_size, "Size of receive queue in number of work requests");
 
+int mad_wait_on_busy = 0;
+module_param_named(treat_busy_as_timeout, mad_wait_on_busy, int, 0444);
+MODULE_PARM_DESC(treat_busy_as_timeout, "When true, treat BUSY responses as if they were timeouts.");
+
 static struct kmem_cache *ib_mad_cache;
 
 static struct list_head ib_mad_port_list;
@@ -1120,6 +1124,7 @@ int ib_post_send_mad(struct ib_mad_send_buf *send_buf,
 		mad_send_wr->timeout = msecs_to_jiffies(send_buf->timeout_ms);
 		mad_send_wr->max_retries = send_buf->retries;
 		mad_send_wr->retries_left = send_buf->retries;
+		mad_send_wr->wait_on_busy = send_buf->wait_on_busy || mad_wait_on_busy;
 		send_buf->retries = 0;
 		/* Reference for work request to QP + response */
 		mad_send_wr->refcount = 1 + (mad_send_wr->timeout > 0);
@@ -1828,6 +1833,9 @@ static void ib_mad_complete_recv(struct ib_mad_agent_private *mad_agent_priv,
 
 	/* Complete corresponding request */
 	if (ib_response_mad(mad_recv_wc->recv_buf.mad)) {
+		u16 busy = be16_to_cpu(mad_recv_wc->recv_buf.mad->mad_hdr.status) &
+							   IB_MGMT_MAD_STATUS_BUSY;
+
 		spin_lock_irqsave(&mad_agent_priv->lock, flags);
 		mad_send_wr = ib_find_send_mad(mad_agent_priv, mad_recv_wc);
 		if (!mad_send_wr) {
@@ -1836,6 +1844,20 @@ static void ib_mad_complete_recv(struct ib_mad_agent_private *mad_agent_priv,
 			deref_mad_agent(mad_agent_priv);
 			return;
 		}
+
+		printk(KERN_DEBUG PFX "Completing recv %p: busy = %d, retries_left = %d, wait_on_busy = %d\n",
+			   mad_send_wr, busy, mad_send_wr->retries_left, mad_send_wr->wait_on_busy);
+
+		if (busy && mad_send_wr->retries_left && mad_send_wr->wait_on_busy &&
+		   (mad_recv_wc->recv_buf.mad->mad_hdr.method != IB_MGMT_METHOD_TRAP_REPRESS)) {
+			/* Just let the query timeout and have it requeued later */
+			spin_unlock_irqrestore(&mad_agent_priv->lock, flags);
+			ib_free_recv_mad(mad_recv_wc);
+			deref_mad_agent(mad_agent_priv);
+			printk(KERN_INFO PFX "SA/SM responded MAD_STATUS_BUSY. Allowing request to time out.\n");
+			return;
+		}
+
 		ib_mark_mad_done(mad_send_wr);
 		spin_unlock_irqrestore(&mad_agent_priv->lock, flags);
 
diff --git a/drivers/infiniband/core/mad_priv.h b/drivers/infiniband/core/mad_priv.h
index 8b4df0a..b6477cf 100644
--- a/drivers/infiniband/core/mad_priv.h
+++ b/drivers/infiniband/core/mad_priv.h
@@ -135,6 +135,7 @@ struct ib_mad_send_wr_private {
 	unsigned long timeout;
 	int max_retries;
 	int retries_left;
+	int wait_on_busy;
 	int retry;
 	int refcount;
 	enum ib_wc_status status;
diff --git a/include/rdma/ib_mad.h b/include/rdma/ib_mad.h
index d3b9401..fadc2c3 100644
--- a/include/rdma/ib_mad.h
+++ b/include/rdma/ib_mad.h
@@ -77,6 +77,15 @@
 
 #define IB_MGMT_MAX_METHODS			128
 
+/* MAD Status field bit masks */
+#define IB_MGMT_MAD_STATUS_SUCCESS                      0x0000
+#define IB_MGMT_MAD_STATUS_BUSY                         0x0001
+#define IB_MGMT_MAD_STATUS_REDIRECT_REQD                0x0002
+#define IB_MGMT_MAD_STATUS_BAD_VERERSION                0x0004
+#define IB_MGMT_MAD_STATUS_UNSUPPORTED_METHOD           0x0008
+#define IB_MGMT_MAD_STATUS_UNSUPPORTED_METHOD_ATTRIB    0x000c
+#define IB_MGMT_MAD_STATUS_INVALID_ATTRIB_VALUE         0x001c
+
 /* RMPP information */
 #define IB_MGMT_RMPP_VERSION			1
 
@@ -246,6 +255,7 @@ struct ib_mad_send_buf {
 	int			seg_count;
 	int			seg_size;
 	int			timeout_ms;
+	int			wait_on_busy;
 	int			retries;
 };
 

Re: [PATCH V3] SA Busy Handling

[Hal Rosenstock]

Mike,

On 12/2/2010 3:28 PM, Mike Heinz wrote:
> Fixed the loss of the trap repress handling, which must have gotten lost somewhere between May and now...
>
> Hal - your formatting nit, when I looked at the patch file, I see the correct spacing around the '&&' in the if statement.

OK; I see that now; I was commenting on the text in the body of the 
message and not the attachment. I'm not sure why the body text is 
whitespace mangled.

> ---
> This patch provides a new module parameter for ib_mad that tells ib_mad to treat BUSY responses as timeouts.
>
> parm:           treat_busy_as_timeout:When true, treat BUSY responses as if they were timeouts. (int)
>
> When this parameter is true, ib_mad will unilaterally treat BUSY responses as timeouts regardless of the desired behavior of the caller.
>
> In addition, this patch also provides changes to ib_mad_send_buf to allow senders of MAD requests to request this behavior even if the parameter is not set. The patch does NOT provide changes to ib_umad to allow user apps to select this functionality.
>
> Signed-off-by: Michael Heinz<michael.heinz-h88ZbnxC6KDQT0dZR+AlfA@public.gmane.org>

> ---
>
>   drivers/infiniband/core/mad.c      |   22 ++++++++++++++++++++++
>   drivers/infiniband/core/mad_priv.h |    1 +
>   include/rdma/ib_mad.h              |   10 ++++++++++
>   3 files changed, 33 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/infiniband/core/mad.c b/drivers/infiniband/core/mad.c
> index 64e660c..2e103ed 100644
> --- a/drivers/infiniband/core/mad.c
> +++ b/drivers/infiniband/core/mad.c
> @@ -54,6 +54,10 @@ MODULE_PARM_DESC(send_queue_size, "Size of send queue in number of work requests
>   module_param_named(recv_queue_size, mad_recvq_size, int, 0444);
>   MODULE_PARM_DESC(recv_queue_size, "Size of receive queue in number of work requests");
>
> +int mad_wait_on_busy = 0;
> +module_param_named(treat_busy_as_timeout, mad_wait_on_busy, int, 0444);
> +MODULE_PARM_DESC(treat_busy_as_timeout, "When true, treat BUSY responses as if they were timeouts.");
> +
>   static struct kmem_cache *ib_mad_cache;
>
>   static struct list_head ib_mad_port_list;
> @@ -1120,6 +1124,7 @@ int ib_post_send_mad(struct ib_mad_send_buf *send_buf,
>   		mad_send_wr->timeout = msecs_to_jiffies(send_buf->timeout_ms);
>   		mad_send_wr->max_retries = send_buf->retries;
>   		mad_send_wr->retries_left = send_buf->retries;
> +		mad_send_wr->wait_on_busy = send_buf->wait_on_busy || mad_wait_on_busy;
>   		send_buf->retries = 0;
>   		/* Reference for work request to QP + response */
>   		mad_send_wr->refcount = 1 + (mad_send_wr->timeout>  0);
> @@ -1828,6 +1833,9 @@ static void ib_mad_complete_recv(struct ib_mad_agent_private *mad_agent_priv,
>
>   	/* Complete corresponding request */
>   	if (ib_response_mad(mad_recv_wc->recv_buf.mad)) {
> +		u16 busy = be16_to_cpu(mad_recv_wc->recv_buf.mad->mad_hdr.status)&
> +							   IB_MGMT_MAD_STATUS_BUSY;
> +
>   		spin_lock_irqsave(&mad_agent_priv->lock, flags);
>   		mad_send_wr = ib_find_send_mad(mad_agent_priv, mad_recv_wc);
>   		if (!mad_send_wr) {
> @@ -1836,6 +1844,20 @@ static void ib_mad_complete_recv(struct ib_mad_agent_private *mad_agent_priv,
>   			deref_mad_agent(mad_agent_priv);
>   			return;
>   		}
> +
> +		printk(KERN_DEBUG PFX "Completing recv %p: busy = %d, retries_left = %d, wait_on_busy = %d\n",
> +			   mad_send_wr, busy, mad_send_wr->retries_left, mad_send_wr->wait_on_busy);
> +
> +		if (busy&&  mad_send_wr->retries_left&&  mad_send_wr->wait_on_busy&&
> +		   (mad_recv_wc->recv_buf.mad->mad_hdr.method != IB_MGMT_METHOD_TRAP_REPRESS)) {
> +			/* Just let the query timeout and have it requeued later */
> +			spin_unlock_irqrestore(&mad_agent_priv->lock, flags);
> +			ib_free_recv_mad(mad_recv_wc);
> +			deref_mad_agent(mad_agent_priv);
> +			printk(KERN_INFO PFX "SA/SM responded MAD_STATUS_BUSY. Allowing request to time out.\n");

Is this really needed ? Won't this spam the log ?

> +			return;
> +		}
> +
>   		ib_mark_mad_done(mad_send_wr);
>   		spin_unlock_irqrestore(&mad_agent_priv->lock, flags);

> diff --git a/drivers/infiniband/core/mad_priv.h b/drivers/infiniband/core/mad_priv.h
> index 8b4df0a..b6477cf 100644
> --- a/drivers/infiniband/core/mad_priv.h
> +++ b/drivers/infiniband/core/mad_priv.h
> @@ -135,6 +135,7 @@ struct ib_mad_send_wr_private {
>   	unsigned long timeout;
>   	int max_retries;
>   	int retries_left;
> +	int wait_on_busy;
>   	int retry;
>   	int refcount;
>   	enum ib_wc_status status;
> diff --git a/include/rdma/ib_mad.h b/include/rdma/ib_mad.h
> index d3b9401..fadc2c3 100644
> --- a/include/rdma/ib_mad.h
> +++ b/include/rdma/ib_mad.h
> @@ -77,6 +77,15 @@
>
>   #define IB_MGMT_MAX_METHODS			128
>
> +/* MAD Status field bit masks */
> +#define IB_MGMT_MAD_STATUS_SUCCESS                      0x0000
> +#define IB_MGMT_MAD_STATUS_BUSY                         0x0001
> +#define IB_MGMT_MAD_STATUS_REDIRECT_REQD                0x0002
> +#define IB_MGMT_MAD_STATUS_BAD_VERERSION                0x0004

typo... BAD_VERSION

-- Hal

> +#define IB_MGMT_MAD_STATUS_UNSUPPORTED_METHOD           0x0008
> +#define IB_MGMT_MAD_STATUS_UNSUPPORTED_METHOD_ATTRIB    0x000c
> +#define IB_MGMT_MAD_STATUS_INVALID_ATTRIB_VALUE         0x001c
> +
>   /* RMPP information */
>   #define IB_MGMT_RMPP_VERSION			1
>
> @@ -246,6 +255,7 @@ struct ib_mad_send_buf {
>   	int			seg_count;
>   	int			seg_size;
>   	int			timeout_ms;
> +	int			wait_on_busy;
>   	int			retries;
>   };
>

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH V3] SA Busy Handling

[Hefty, Sean]

> In addition, this patch also provides changes to ib_mad_send_buf to allow
> senders of MAD requests to request this behavior even if the parameter is
> not set. The patch does NOT provide changes to ib_umad to allow user apps
> to select this functionality.

This should be removed unless something uses it.  Do you have a follow on patch that sets this for some caller?

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH V3] SA Busy Handling

[Mike Heinz]

Not at this time, because the last time we discussed this, several individuals insisted that we provide a user space mechanism for selecting it.

If we don't want to use this, then we should just go all the way back to the original patch I submitted in the spring. That version does not have the user space hook or the module parameter.

-----Original Message-----
From: linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Hefty, Sean
Sent: Friday, December 03, 2010 11:50 AM
To: Mike Heinz; Hal Rosenstock; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Subject: RE: [PATCH V3] SA Busy Handling

> In addition, this patch also provides changes to ib_mad_send_buf to allow
> senders of MAD requests to request this behavior even if the parameter is
> not set. The patch does NOT provide changes to ib_umad to allow user apps
> to select this functionality.

This should be removed unless something uses it.  Do you have a follow on patch that sets this for some caller?

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH V3] SA Busy Handling

[Hefty, Sean]

> Not at this time, because the last time we discussed this, several
> individuals insisted that we provide a user space mechanism for selecting
> it.
> 
> If we don't want to use this, then we should just go all the way back to
> the original patch I submitted in the spring. That version does not have
> the user space hook or the module parameter.

This is more of what I was suggesting.  Long term, I believe that we want more policy in the mad layer.  It should limit the number of outstanding queries against the SA, adjust timeouts based on calculated values, determine how many times to retry based on a total timeout provided by the user, etc.  If we can get agreement on this direction, then handling busy responses would be part of this.  Until the infrastructure is in place to do this, simply dropping busy responses for now is acceptable to me.

- Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH V3] SA Busy Handling

[Mike Heinz]

Agreed.

-----Original Message-----
From: linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Hefty, Sean
Sent: Friday, December 03, 2010 1:57 PM
To: Mike Heinz; Hal Rosenstock; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Subject: RE: [PATCH V3] SA Busy Handling

> Not at this time, because the last time we discussed this, several
> individuals insisted that we provide a user space mechanism for selecting
> it.
> 
> If we don't want to use this, then we should just go all the way back to
> the original patch I submitted in the spring. That version does not have
> the user space hook or the module parameter.

This is more of what I was suggesting.  Long term, I believe that we want more policy in the mad layer.  It should limit the number of outstanding queries against the SA, adjust timeouts based on calculated values, determine how many times to retry based on a total timeout provided by the user, etc.  If we can get agreement on this direction, then handling busy responses would be part of this.  Until the infrastructure is in place to do this, simply dropping busy responses for now is acceptable to me.

- Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V3] SA Busy Handling

[Roland Dreier]

 > If we don't want to use this, then we should just go all the way back
 > to the original patch I submitted in the spring. That version does
 > not have the user space hook or the module parameter.

I definitely feel we should not have the module parameter.  The new
behavior is supposed to always be better, right?  So why would anyone
ever want to mess with the module parameter?

However I think I would like to have at least some idea of what the
smarter way to handle "busy" responses will be.  If we're always just
going to treat them like timeouts, then it seems the SA would be smarter
to avoid sending them in the first place.

 - R.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH V3] SA Busy Handling

[Mike Heinz]

The reason for preferring a BUSY response to simply having the SM throw the packets on the floor is that it makes it possible to differentiate between an overloaded SM and communications problems when debugging a fabric problem.

-----Original Message-----
From: linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Roland Dreier
Sent: Friday, December 03, 2010 4:44 PM
To: Mike Heinz
Cc: Hefty, Sean; Hal Rosenstock; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Subject: Re: [PATCH V3] SA Busy Handling

 > If we don't want to use this, then we should just go all the way back
 > to the original patch I submitted in the spring. That version does
 > not have the user space hook or the module parameter.

I definitely feel we should not have the module parameter.  The new
behavior is supposed to always be better, right?  So why would anyone
ever want to mess with the module parameter?

However I think I would like to have at least some idea of what the
smarter way to handle "busy" responses will be.  If we're always just
going to treat them like timeouts, then it seems the SA would be smarter
to avoid sending them in the first place.

 - R.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V3] SA Busy Handling

[Roland Dreier]

 > The reason for preferring a BUSY response to simply having the SM
 > throw the packets on the floor is that it makes it possible to
 > differentiate between an overloaded SM and communications problems
 > when debugging a fabric problem.

Is that really the main reason for all this?  Wouldn't it be simpler to
have the SM log something locally when it's overloaded, rather than
generating even more traffic on the fabric?

 - R.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH V3] SA Busy Handling

[Hefty, Sean]

> However I think I would like to have at least some idea of what the
> smarter way to handle "busy" responses will be.  If we're always just
> going to treat them like timeouts, then it seems the SA would be smarter
> to avoid sending them in the first place.

I'm making stuff up here, but one possible difference between handling busy response versus a timeout could be: a busy response increases the timeout before a mad is retried and wouldn't necessarily decrease the total number of retries that would be attempted.  A timed out mad would be retried immediately, with a longer timeout on the back-end.

However, I can't think of a reason why it would be better for the SA to respond with busy under heavy load rather than just dropping the mad.  If busy conveyed some sort of retry time, that would be nice.

- Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH V3] SA Busy Handling

[Mike Heinz]

Sean's got the idea exactly right, but it's important to also remember that the BUSY response is part of the original spec and was part of the old IB Gold and Infinicon stacks as well. I'm not sure how IB Gold handled BUSY responses, but the ICS stack definitely used a separate, exponential retry/timing model for BUSY responses. Sean's idea about including a time value in the BUSY response is fine, but would involve changing the Infiniband specification.

So, what we have right now is that OFED treats BUSY as a hard error, which can cause some modules to  spam the SM. (In addition, none of the user-land tools use any kind of retries at all, but that's a different issue.)

Finally, another reason for using the BUSY is to *prevent* overloading and congestion by causing a percentage of requests to delay rather than simply waiting till the SM is overloaded and rejecting everything. Yes, you could simulate by throwing a percentage of the requests on the floor, but it is still more flexible for both the SM and the client to use the BUSY response.

-----Original Message-----
From: Hefty, Sean [mailto:sean.hefty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org] 
Sent: Friday, December 03, 2010 5:25 PM
To: Roland Dreier; Mike Heinz
Cc: Hal Rosenstock; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Subject: RE: [PATCH V3] SA Busy Handling

> However I think I would like to have at least some idea of what the
> smarter way to handle "busy" responses will be.  If we're always just
> going to treat them like timeouts, then it seems the SA would be smarter
> to avoid sending them in the first place.

I'm making stuff up here, but one possible difference between handling busy response versus a timeout could be: a busy response increases the timeout before a mad is retried and wouldn't necessarily decrease the total number of retries that would be attempted.  A timed out mad would be retried immediately, with a longer timeout on the back-end.

However, I can't think of a reason why it would be better for the SA to respond with busy under heavy load rather than just dropping the mad.  If busy conveyed some sort of retry time, that would be nice.

- Sean

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V3] SA Busy Handling

[Hal Rosenstock]

On 12/6/2010 10:27 AM, Mike Heinz wrote:
>
> (In addition, none of the user-land tools use any kind of retries at all, but that's a different issue.)

Which user land tools are you referring to not using retries ?

Certain infiniband-diags do use the retry mechanism while others do not. 
It depends on the (nature of the) diag command. The simple manual 
query/response ones (using umad_send) do not use retry whereas others 
(more sophisticated ones like ibnetdiscover) do. In any case, if retries 
are appropriate and not currently implemented, it is simple to add them in.

-- Hal
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH V3] SA Busy Handling

[Mike Heinz]

Looking at the management source code, I don't see a single instance of umad_send() where retries is set to a non-zero value except saquery, which I think used to be zero as well.

-----Original Message-----
From: Hal Rosenstock [mailto:hal-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org] 
Sent: Monday, December 06, 2010 11:47 AM
To: Mike Heinz
Cc: Hefty, Sean; Roland Dreier; Todd Rimmer; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Subject: Re: [PATCH V3] SA Busy Handling

On 12/6/2010 10:27 AM, Mike Heinz wrote:
>
> (In addition, none of the user-land tools use any kind of retries at all, but that's a different issue.)

Which user land tools are you referring to not using retries ?

Certain infiniband-diags do use the retry mechanism while others do not. 
It depends on the (nature of the) diag command. The simple manual 
query/response ones (using umad_send) do not use retry whereas others 
(more sophisticated ones like ibnetdiscover) do. In any case, if retries 
are appropriate and not currently implemented, it is simple to add them in.

-- Hal

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V3] SA Busy Handling

[Hal Rosenstock]

On 12/6/2010 11:50 AM, Mike Heinz wrote:
> Looking at the management source code, I don't see a single instance of umad_send() where retries is set to a non-zero value except saquery, which I think used to be zero as well.

In general, those using umad_send are the simple request/response ones 
other than ibnetdiscover (in libibnetdisc).

Other diags that use libibmad (for the mad rpc mechanism) have 3 retries.

-- Hal
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH V3] SA Busy Handling

[Hefty, Sean]

> Looking at the management source code, I don't see a single instance of
> umad_send() where retries is set to a non-zero value except saquery, which
> I think used to be zero as well.

Since umad_send is a public interface, applications may exist outside of the ib-mgmt tree that make use of it with non-zero retries.  ib_acm is at least one.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html