Re: [PATCH v2] net: sctp: Fix a_rwnd/rwnd management to reflect real state of the receiver's buffer

[Vlad Yasevich <vyasevich@gmail.com>]

On 02/11/2014 02:56 PM, Matija Glavinic Pecotic wrote:
> Hello Vlad,
>
> On 02/11/2014 03:52 PM, ext Vlad Yasevich wrote:
>> Hi Matija
>>
>> On 02/09/2014 02:15 AM, Matija Glavinic Pecotic wrote:
>>>
>>> --- net-next.orig/net/sctp/ulpevent.c
>>> +++ net-next/net/sctp/ulpevent.c
>>> @@ -989,7 +989,7 @@ static void sctp_ulpevent_receive_data(s
>>>  	skb = sctp_event2skb(event);
>>>  	/* Set the owner and charge rwnd for bytes received.  */
>>>  	sctp_ulpevent_set_owner(event, asoc);
>>> -	sctp_assoc_rwnd_decrease(asoc, skb_headlen(skb));
>>> +	sctp_assoc_rwnd_update(asoc, false);
>>>
>>>  	if (!skb->data_len)
>>>  		return;
>>> @@ -1035,8 +1035,9 @@ static void sctp_ulpevent_release_data(s
>>>  	}
>>>
>>>  done:
>>> -	sctp_assoc_rwnd_increase(event->asoc, len);
>>> -	sctp_ulpevent_release_owner(event);
>>> +	atomic_sub(event->rmem_len, &event->asoc->rmem_alloc);
>>> +	sctp_assoc_rwnd_update(event->asoc, true);
>>> +	sctp_association_put(event->asoc)
>>
>> Can't we simply change the order of window update and release instead
>> of open coding it like this?
>
> that was the initial idea, but sctp_ulpevent_release_owner puts
> the association and calls sctp_association_destroy if its time to
> do so. IMHO, in the case if we would switch it, we would open a
> potential race condition.

On the tx side, I agree that there would be race.  One the recieve side,
I don't think you could ever be in the codition where the last reference
on the asoc is held by a data chunk you are feeing.

However, to be completely safe we could do:
    asoc = event->asoc;
    sctp_association_hold(asoc);
    sctp_ulpevent_release_owner(event);
    sctp_assoc_rwnd_update(asoc, true);
    sctp_assocition_put(asoc);


The reason I don't like to open-code release owner is that if it
ever changes, we'll have to rememeber to change this open-coded
implementation as well.

Thanks
-vlad

>
> I agree this doesn't look the best. But since we should call
> sctp_assoc_rwnd_update after accounting and before put, we have only
> option to move sctp_assoc_rwnd_update to _ulpevent_release_owner. As on
> this path we wish to update peer and generate sack, but we for sure do not
> want it on all paths where ulpevent_release_owner is used, I see no
> alternative but to add additional parameter to ulpevent_release_owner
> which would be just passed to rwnd_update - bool update_peer. On the
> other hand, I wonder whether ulpevent_release_owner would do more then
> it should in that case?
>
>>
>> -vlad
>>
>>>  }
>>>
>>>  static void sctp_ulpevent_release_frag_data(struct sctp_ulpevent
*event)
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>