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@xxxxxxxxxxxxxxx
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
