Re: [PATCH] sctp: Fix mis-ordering of user space data when multihoming

[Vlad Yasevich <vladislav.yasevich@hp.com>]

Neil Horman wrote:
>> We could do that yes, but it concerns me, as assigning the tsn in
>>> sctp_outq_flush leaves us in a position where we assign tsn to chunks that might
>>> get dropped prior to submission to the ip layer.  Consider if we have a routing
>>> table disruption, and we  follow the no_route path in sctp_packet_transmit.  In
>>> that situation, we will discard chunks with tsns assigned, leaving a gap in our
>>> stream.  Unless we have a recovery path for that, I think the better option is
>>> to wait to assign tsns until we are sure we can submit them to the ip layer
>>> safely (where the transmitted queue can re-tranmit them if need be).  If you can
>>> explain the SACK case in a little more detail above, perhaps we can come up with
>>> some logic to govern when it is and is not safe to call sctp_packet_transmit
>>> from sctp_packet_transmit_chunk for data chunks.
>> Assume that we have a number of queued chunks that add up to multiple MTUs
>> all going to the same transport (typical case).  They are currently gated by
>> congestion window.
>>
>> A SACK arrives triggering a flush.  With the proposed patch, once we fill a
>> single MTU, the main loop in sctp_outq_flush will exist and we will transmit
>> only a single packet and under-utilize our congesting window thus preventing
>> future growth.  With the old code, we had multiple packets sent out thus
>> filling the congestion window.
>>
>> Another thing your patch didn't take into account is that every time we change
>> the transport in sctp_outq_flush, we reset the packet, effectively marking it
>> empty.  You would end up leaking chunks if there was any queuing effects.
>>
>> If a transient routing problem happens and the packet fails to get sent, that's
>> no different then a loss event in the network.  It will get reported back as
>> gaps or, if the failure is long term, it will be detected with HBs and
>> retransmissions.  So I don't see a problem of assigning TSNs when the DATA is
>> added to the packet.  We don't really want to do it any earlier though.
>>
> 
> Yeah, ok, heres a new version, instead of just skipping the packet transmit in
> transmit_chunk, we instead simply assign a tsn in sctp_outq_flush, after we
> dequeue a data chunk from the outq and do the normal expiration and invalid
> stream checking.
> 
> Regards
> Neil
> 

Hi Neil

I don't think we can do that in sctp_outq_flush().

You can do it in either sctp_packet_append_data() or sctp_packet_append_chunk().
I think it will be cleaner in append_data(), but that's your choice.

-vlad