assimilation / assimilation-official

This is the official main repository for the Assimilation project
51 stars 9 forks source link

Medium: clientlib: don't set nextrexmit time on ACK #61

Closed dmuhamedagic closed 6 years ago

dmuhamedagic commented 6 years ago

Moving nextrexmit time into future on receiving an ACK seems unwarranted. On ACK receive, the frameset is anyway going to be removed from the queue. If there are more framesets in the queue, they should be then sent at the time which has been set when a previous (now probably acknowledged) frameset was sent. That way we get packets going out at a rate of 1 per rexmit_interval (currently set to 2 seconds). Otherwise, the rate appears to be roughly halfed (as actually observed). If we want a slower rate, then perhaps the FSPROTO_REXMITINTERVAL should be updated accordingly.

Alan-R commented 6 years ago

I think this change was a good one - but it shouldn't affect anything unless packets are getting lost. So it bothers me that you have observed that it does seem to slow things down.

We should be transmitting a packet each time we get an ACK. It should normally be ACKs that limit the pace of transmission, not retransmit requests. The only exception is if we ack slower than the retransmit interval.

dmuhamedagic commented 6 years ago

On Sat, May 05, 2018 at 01:39:41AM +0000, Alan Robertson wrote:

I think this change was a good one - but it shouldn't affect anything unless packets are getting lost. So it bothers me that you have observed that it does seem to slow things down.

I'm not sure whether packets were indeed lost. But anyway this thing was superfluous.

We should be transmitting a packet each time we get an ACK. It should normally be ACKs that limit the pace of transmission, not retransmit requests. The only exception is if we ack slower than the retransmit interval.

Yes. However, at least on startup, the queue always quickly gets bigger than the window and the nextrexmit parameter is then what controls the transmission. Also for the packets that weren't sent, but couldn't go out because of the window full condition. It should be perhaps named nextxmit?

Alan-R commented 6 years ago

This should only be the case if it always takes us > 2 secs to ack each packet. The nanoprobe should be acking packets as they're received. (auto-ack). The CMA acks them after they are completely processed.

Alan-R commented 6 years ago

That really only controls re-transmissions. If the retransmit timers are controlling initial transmissions (and not the window size) - then that's a bug.

It won't violate the window size. Things should be getting transmitted the first time for one of two reasons:    -- When they were queued, the window wasn't full yet    -- An incoming ACK arrived and they are now inside the window.

If nextrexmit does violate the window size, then that's a bug. I bet it doesn't (but only a small bet) ;-).

On the other hand, a bug that waits until it's first in the queue to be sent wouldn't be unimaginable.

But if you really want to know, then you need to watch the SEQNO frames in the framesets - then you'll really know when they're sent the first time.