On Mon, Nov 02, 2009 at 08:41:06PM -0500, Neil Horman wrote: > On Mon, Nov 02, 2009 at 01:57:42PM -0500, Vlad Yasevich wrote: > > > > > > Neil Horman wrote: > > > On Mon, Nov 02, 2009 at 12:58:27PM -0500, Vlad Yasevich wrote: > > >> > > >> 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(). > > >> > > > Why can't we do it in sctp_outq_flush? > > > > Ok, looking at the 'resent' code you left in packet_transmit, this will work, > > but we now end up assigning sequence numbers to DATA that may not be transmitted > > this time around. > > > > It will also make FWD-TSNs a bit weird. Worth a test. My personal preference > > would be to do it when the chunk is added to the packet. > > > > Ok, very well. I've moved the assignment to the point right after we actually > enqueue the chunk to the offered packet. > Ping, sorry vlad, not sure where we've left off with this. I've given this some testing here, and it works for me. Were there more concerns you had with this variant of the patch? Thanks & Regards Neil -- 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
