possible RFC6675 compute_pipe counting issue.

Thu Dec 20 19:18:14 UTC 2018

After looking at this again, it seems to me, what sackhint.last_sack_ack is tracking really is identical to tp->snd_fack, and the only point is to check if th_ack or "snd_fack" should be checked against for the RTT sampling

Why not use snd_fack instead and save the memory access?

(in the regular ACK case, snd_fack would have to be set to snd_una / th_ack, prior to the call to ertt_packet_measurement, in tcp_input)

-----Original Message-----
From: Scheffenegger, Richard 
Sent: Donnerstag, 20. Dezember 2018 17:42
To: Lawrence Stewart <lstewart at netflix.com>
Cc: freebsd-transport at freebsd.org; hiren at strugglingcoder.info; jtl at FreeBSD.org
Subject: RE: possible RFC6675 compute_pipe counting issue.

Sorry to bother all of you again,

Unfortunately, I think I found another logical error in the general SACK and related code, but again, would like to check with you first (as it's more likely that I am just mistaken):

I looked at sackhint.last_sack_ack, what this is used for.

And I found, that Lawrence uses this in the khelp/h_ertt.c code, to arrive at a better RTT probe during loss recovery.

But the question I have is the following:

A receiver is expected, to return the most recent change in the 1st SACK option block. 

But in tcp_sack.c, we first  mangle the ordering of the received SACK option blocks, to become consecutive increasing in sequence space, to facilitate single-pass update of the scoreboard, and only then the topmost (highest) SACK block is picked for the ertt calculation.

This is fine, as long as packets are sent in sequential order (common 😊), and not reordered during delivery (hmm), and the ACK return path doesn't do any reordering, and we don't want to use any SACK retransmitted packets for further RTT updates...

Long story short: Shouldn't "sackhint.last_sack_ack" be set to the end of the 1st SACK block received, rather then the last, ordered SACK block, to yield the proper RTT timing information?

(I admit to have not studied the h_ertt code in detail).

Best regards,
   Richard

-----Original Message-----
From: hiren at strugglingcoder.info <hiren at strugglingcoder.info>
Sent: Donnerstag, 20. Dezember 2018 00:17
To: Scheffenegger, Richard <Richard.Scheffenegger at netapp.com>; jtl at FreeBSD.org
Cc: freebsd-transport at freebsd.org
Subject: Re: possible RFC6675 compute_pipe counting issue.

On 12/19/18 at 09:21P, Scheffenegger, Richard wrote:
> Hi Hiren,

Hi Richard,
(I may not get time to look at this soon but responding right now before I forget :-))
> 
> I'm trying to reactivate my old Lost Retransmission Detection patch.
> 
> And I found your 2015 patch around sacked_bytes (RFC6675) here
> 
> https://github.com/rscheff/freebsd/commit/d3c29dd43ff23e293d5988258493
> 015fb4a072ba
> 
> (The line ~389 in tcp_sack.c)
> 
> But I think the code is problematic in two reasons:
> 
> a) very thin clients, which may only send SACK deltas instead of full 
> excerpts of the scoreboard (e.g. only a sack block covering the most 
> recent received data)
> b) whenever the receiver scoreboard has more discontinuous entries than what a single ACK can carry as SACK fields (typically 3, possibly 4 or as little as one).
> 
> However, these will overestimate the outstanding data (pipe).
> 
> (a) A cheating receiver sending multiple, identical sack blocks could game the sender, though, as the check is done on a ack sack block by block check, rather than when the scoreboard is updated...
> 
> 
> Reason: Your patch only looks at the SACK data contained in the most recent ACK, rather than proper accounting of the non-sack holes in the scoreboard.

Yes, you are right. And I tried to note this in code/commitlog/code-review somewhere.

> 
> When I improved Aris PRR code, I did the accounting when the scoreboard is being updated; sacked_bytes need to exclude any snd.una move to the right, but should be similar:
> 
> https://github.com/rscheff/freebsd/commit/4a4877478fe55e8ebdfa0daf9a2b
> e4b212d07cde
> 
> 
> As PRR needs a proper value for the delivered delta bytes per ACK, it should be simple to keep track of the correct value for the SACKed bytes (not holes) in the scoreboard too...
> 
> Obviously, this can not be that much of an issue, as RFC6675 pipe is disabled by default, and would underestimate the number of sacked bytes at the receiver, unless the receiver has malicious intent...
> 
> Any thoughts?

I'll try to look at the patch as time permits and it may take a long time. I am ccing Jonathan who was involved iirc in the discussion of better 6675 support.

As your patch also touches tcpcb, it needs more eyes. NF may have a bunch of changes here so best to coordinate with them.

I guess you can open a phabricator review request and go from there?

Cheers,
Hiren