On 06/28/2012 02:07 PM, Neil Horman wrote:
On Thu, Jun 28, 2012 at 11:58:56AM -0400, Vlad Yasevich wrote:
On 06/28/2012 11:33 AM, Neil Horman wrote:
On Wed, Jun 27, 2012 at 03:44:22PM -0400, Vlad Yasevich wrote:
On 06/27/2012 01:28 PM, Neil Horman wrote:
On Wed, Jun 27, 2012 at 11:10:26AM -0400, Vlad Yasevich wrote:
I didn't think of this yesterday, but I think it would be much
better to use pkt->transport here since you are adding the chunk to
the packet and it will go out on the transport of the packet. You
are also guaranteed that pkt->transport is set.
I don't think it really matters, as the chunk transport is used to lookup the
packet that we append to, and if the chunk transport is unset, its somewhat
questionable as to weather we should bundle, but if packet->transport is set,
its probably worth it to avoid the extra conditional.
Just looked at the code flow. chunk->transport may not be set until
the end of sctp_packet_append_chunk. For new data, transport may
not be set. For retransmitted data, transport is set to last
transport data was sent on. So, we could be looking at the wrong
transport. What you are trying to decided is if the current
transport we will be used can take the SACK, but you may not be
looking at the current transport. Looking at packet->transport is
the correct thing to do.
-vlad
So, I agree after what you said above, that this is the right thing to do. That
said, I just tested the change with the SCTP_RR test in netperf, and it wound up
giving me horrid performance (Its reporting about 5 transactions per second).
It appears that whats happening is that, because the test alternates which
transports it sends out, and because it waits for a sack of teh prior packet
before it sends out the next transaction, we're always missing the bundle
opportunity, and always waiting for the 200ms timeout for the sack to occur.
While I know this is a pessimal case, it really seems bad to me. It seems that
because I was using chunk->transport previously, I luckily got the transport
wrong sometimes, and it managed to bundle more often.
So I'm not sure what to do here. I had really wanted to avoid adding a sysctl
here, but given that this is likely a corner cases, it seems that might be the
best approach. Do you have any thoughts?
Neil
that's strange. did you modify the SCTP_RR to alternate transports?
Seems like responses in the RR test need to go the address of the
sender so that we don't see things like:
Request (t) --->
<--- Response (t2)
Should be:
Request (t1) --->
<--- Response (t1)
-vlad
That would seem to me to be the case too....
However, having looked at this some more, it seems I just jumped the gun on
this. Its happening because sctp_eat_data variants are issuing a SCTP_GEN_SACK
command at the end of every received packet, which causes the moved_ctsn value
to get cleared.
Ok, that should only happen the very first time as we are supposed to
ack the first data immediately. On subsequent packets it should just
start a timer because we are following the 2pkt/200ms rule.
Then, when the response happens, we should bundle the SACK as long as
the data is leaving on the transport that moved the CTSN.
So we might be using the wrong transport and as result you send data and
then end up waiting for a SACK.
-vlad
We follow the sack every other packet rule instead of taking
the opportunity to bundle on send, so we're sending a packet with a sack, and a
second packet with a 1 byte data chunk (thats part of the SCTP_RR test).
I'm not sure why I didn't see this when I was using the chunk->transport
pointer. Maybe I was just getting lucky with timing...
I'll see how I can go about fixing this.
Neil
--
To unsubscribe from this list: send the line "unsubscribe netdev" 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
