possible RFC6675 compute_pipe counting issue.

[Scheffenegger, Richard]

Hi Hiren, Jonathan,

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

Sure; I still have to work a bit on the PRR patch - I don't like replicating the mechanism in two places, and outside of the normal SACK procedures, when PRR really is an improvement upon 3517. As 6675 pipe (and entering loss recovery) would also be bundled up there, may take me some time too, to get all that in a nice, concise patch.  

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

Surely, once I believe the code is ready. In the meantime, still looking for reviewers for this simple one 
https://reviews.freebsd.org/D17614

I recently added two packetdrill scripts to demonstrate the (timing) different w/o (std) and with that patch. Potentially, these scripts can be added to Michaels collection of FreeBSd network unit test scripts: 
https://github.com/freebsd-net/tcp-testsuite/


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