Neil Horman wrote: > 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? > Just running some tests here. It also looks like this was based on the pre .31 code. In the .31 code, you can put this in sctp_packet_append_data() and save us yet another branch based on chunk_is_data(). -vlad > 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
