[PATCH] iw_cm: Call provider listen_destroy function before changing state.

[PATCH] iw_cm: Call provider listen_destroy function before changing state.

[Steve Wise]

When destroying a listening cmid, the iwcm first sets the cmid state to
DESTROYING, then releases the lock and calls into the iwarp provider
to destroy the listening endpoint.  Since the cmid is not locked, its
possible for the iwarp provider to pass a connection request event to
the iwcm before the provider has destroyed the listening endpoint. This
connect request will be silently dropped by the iwcm because the state
of the cmid is DESTROYING.  The iwarp provider, however, doesn't know the
connect request was dropped.  This causes the iwarp provider to never free
up the resources from this connection because the assumption is the iwcm
will _always_ either accept or reject this connection.  The solution is
to only change the cmid state  after the iwarp provider has destroyed its
listening endpoint, and thus no more connection requests will be posted.

Signed-off-by: Steve Wise <swise-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org>
---

 drivers/infiniband/core/iwcm.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/infiniband/core/iwcm.c b/drivers/infiniband/core/iwcm.c
index 1a696f7..3b43bba 100644
--- a/drivers/infiniband/core/iwcm.c
+++ b/drivers/infiniband/core/iwcm.c
@@ -340,11 +340,11 @@ static void destroy_cm_id(struct iw_cm_id *cm_id)
 	spin_lock_irqsave(&cm_id_priv->lock, flags);
 	switch (cm_id_priv->state) {
 	case IW_CM_STATE_LISTEN:
-		cm_id_priv->state = IW_CM_STATE_DESTROYING;
 		spin_unlock_irqrestore(&cm_id_priv->lock, flags);
 		/* destroy the listening endpoint */
 		ret = cm_id->device->iwcm->destroy_listen(cm_id);
 		spin_lock_irqsave(&cm_id_priv->lock, flags);
+		cm_id_priv->state = IW_CM_STATE_DESTROYING;
 		break;
 	case IW_CM_STATE_ESTABLISHED:
 		cm_id_priv->state = IW_CM_STATE_DESTROYING;

--
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] iw_cm: Call provider listen_destroy function before changing state.

[Hefty, Sean]

> diff --git a/drivers/infiniband/core/iwcm.c b/drivers/infiniband/core/iwcm.c
> index 1a696f7..3b43bba 100644
> --- a/drivers/infiniband/core/iwcm.c
> +++ b/drivers/infiniband/core/iwcm.c
> @@ -340,11 +340,11 @@ static void destroy_cm_id(struct iw_cm_id *cm_id)
>  	spin_lock_irqsave(&cm_id_priv->lock, flags);
>  	switch (cm_id_priv->state) {
>  	case IW_CM_STATE_LISTEN:
> -		cm_id_priv->state = IW_CM_STATE_DESTROYING;
>  		spin_unlock_irqrestore(&cm_id_priv->lock, flags);

What does this spinlock protect against if all we do is read the state under it?

Re: [PATCH] iw_cm: Call provider listen_destroy function before changing state.

[Steve Wise]

On 02/22/2012 01:50 PM, Hefty, Sean wrote:
>> diff --git a/drivers/infiniband/core/iwcm.c b/drivers/infiniband/core/iwcm.c
>> index 1a696f7..3b43bba 100644
>> --- a/drivers/infiniband/core/iwcm.c
>> +++ b/drivers/infiniband/core/iwcm.c
>> @@ -340,11 +340,11 @@ static void destroy_cm_id(struct iw_cm_id *cm_id)
>>   	spin_lock_irqsave(&cm_id_priv->lock, flags);
>>   	switch (cm_id_priv->state) {
>>   	case IW_CM_STATE_LISTEN:
>> -		cm_id_priv->state = IW_CM_STATE_DESTROYING;
>>   		spin_unlock_irqrestore(&cm_id_priv->lock, flags);
> What does this spinlock protect against if all we do is read the state under it?
>

Probably nothing in this particular path.  It seems to serialize adding/removing references to the qp and manipulating 
cm_id_priv->qp.  But I don't want to revisit the entire serialization of this module. :)

But the point of the patch is to destroy the listening endpoint in the provider _before_ we stop accepting connect requests.

--
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] iw_cm: Call provider listen_destroy function before changing state.

[Steve Wise]

On 02/22/2012 02:02 PM, Steve Wise wrote:
> On 02/22/2012 01:50 PM, Hefty, Sean wrote:
>>> diff --git a/drivers/infiniband/core/iwcm.c b/drivers/infiniband/core/iwcm.c
>>> index 1a696f7..3b43bba 100644
>>> --- a/drivers/infiniband/core/iwcm.c
>>> +++ b/drivers/infiniband/core/iwcm.c
>>> @@ -340,11 +340,11 @@ static void destroy_cm_id(struct iw_cm_id *cm_id)
>>>       spin_lock_irqsave(&cm_id_priv->lock, flags);
>>>       switch (cm_id_priv->state) {
>>>       case IW_CM_STATE_LISTEN:
>>> -        cm_id_priv->state = IW_CM_STATE_DESTROYING;
>>>           spin_unlock_irqrestore(&cm_id_priv->lock, flags);
>> What does this spinlock protect against if all we do is read the state under it?
>>
>
> Probably nothing in this particular path.  It seems to serialize adding/removing references to the qp and manipulating 
> cm_id_priv->qp.  But I don't want to revisit the entire serialization of this module. :)
>
> But the point of the patch is to destroy the listening endpoint in the provider _before_ we stop accepting connect 
> requests.
>

Oh you were pointing out that I could rearrange the spin lock usage here since we no longer set the state in this one case?
--
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] iw_cm: Call provider listen_destroy function before changing state.

[Roland Dreier]

On Wed, Feb 22, 2012 at 11:43 AM, Steve Wise
<swise-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org> wrote:
>      spin_lock_irqsave(&cm_id_priv->lock, flags);
>        switch (cm_id_priv->state) {
>        case IW_CM_STATE_LISTEN:
> -               cm_id_priv->state = IW_CM_STATE_DESTROYING;
>                spin_unlock_irqrestore(&cm_id_priv->lock, flags);
>                /* destroy the listening endpoint */
>                ret = cm_id->device->iwcm->destroy_listen(cm_id);
>                spin_lock_irqsave(&cm_id_priv->lock, flags);
> +               cm_id_priv->state = IW_CM_STATE_DESTROYING;
>                break;
>        case IW_CM_STATE_ESTABLISHED:
>                cm_id_priv->state = IW_CM_STATE_DESTROYING;

This seems a bit wonky to me... with the old code, we set the state to
DESTROYING inside the lock, so we know all previous connections
were either done with their locked region or saw the new state.

Now we have a window where are done with destroy_listen() but
the state is still not DESTROYING.

Is it possible to have the provider reject requests instead of silently
dropping them?  Would that be a better fix?

 - 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] iw_cm: Call provider listen_destroy function before changing state.

[Steve Wise]

On 02/22/2012 02:32 PM, Roland Dreier wrote:
> On Wed, Feb 22, 2012 at 11:43 AM, Steve Wise
> <swise-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org>  wrote:
>>       spin_lock_irqsave(&cm_id_priv->lock, flags);
>>         switch (cm_id_priv->state) {
>>         case IW_CM_STATE_LISTEN:
>> -               cm_id_priv->state = IW_CM_STATE_DESTROYING;
>>                 spin_unlock_irqrestore(&cm_id_priv->lock, flags);
>>                 /* destroy the listening endpoint */
>>                 ret = cm_id->device->iwcm->destroy_listen(cm_id);
>>                 spin_lock_irqsave(&cm_id_priv->lock, flags);
>> +               cm_id_priv->state = IW_CM_STATE_DESTROYING;
>>                 break;
>>         case IW_CM_STATE_ESTABLISHED:
>>                 cm_id_priv->state = IW_CM_STATE_DESTROYING;
> This seems a bit wonky to me... with the old code, we set the state to
> DESTROYING inside the lock, so we know all previous connections
> were either done with their locked region or saw the new state.
>
> Now we have a window where are done with destroy_listen() but
> the state is still not DESTROYING.
>
> Is it possible to have the provider reject requests instead of silently
> dropping them?  Would that be a better fix?
>

Perhaps.   Or the connect request event handler could return a status indicating whether or not it queued the request 
and will subsequently accept or reject it.



>   - 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] iw_cm: Call provider listen_destroy function before changing state.

[Steve Wise]

On 02/22/2012 02:43 PM, Steve Wise wrote:
> On 02/22/2012 02:32 PM, Roland Dreier wrote:
>> On Wed, Feb 22, 2012 at 11:43 AM, Steve Wise
>> <swise-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org>  wrote:
>>>       spin_lock_irqsave(&cm_id_priv->lock, flags);
>>>         switch (cm_id_priv->state) {
>>>         case IW_CM_STATE_LISTEN:
>>> -               cm_id_priv->state = IW_CM_STATE_DESTROYING;
>>>                 spin_unlock_irqrestore(&cm_id_priv->lock, flags);
>>>                 /* destroy the listening endpoint */
>>>                 ret = cm_id->device->iwcm->destroy_listen(cm_id);
>>>                 spin_lock_irqsave(&cm_id_priv->lock, flags);
>>> +               cm_id_priv->state = IW_CM_STATE_DESTROYING;
>>>                 break;
>>>         case IW_CM_STATE_ESTABLISHED:
>>>                 cm_id_priv->state = IW_CM_STATE_DESTROYING;
>> This seems a bit wonky to me... with the old code, we set the state to
>> DESTROYING inside the lock, so we know all previous connections
>> were either done with their locked region or saw the new state.
>>
>> Now we have a window where are done with destroy_listen() but
>> the state is still not DESTROYING.
>>
>> Is it possible to have the provider reject requests instead of silently
>> dropping them?  Would that be a better fix?
>>
>
> Perhaps.   Or the connect request event handler could return a status indicating whether or not it queued the request 
> and will subsequently accept or reject it.

Actually, the events are deferred and scheduled so my idea doesn't hold water.

I'll code up and test your reject idea.

Thanks,

Steve.


>
>
--
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