|
Bugzilla – Full Text Bug Listing |
| Summary: | TCP RTT TimeStamp option and WindowScale is not RFC-compliant | ||
|---|---|---|---|
| Product: | ns-3 | Reporter: | Tommaso Pecorella <tommaso.pecorella> |
| Component: | tcp | Assignee: | natale.patriciello |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | CC: | natale.patriciello, ns-bugs, tomh |
| Priority: | P5 | ||
| Version: | ns-3-dev | ||
| Hardware: | All | ||
| OS: | All | ||
| Attachments: |
Window scale compliant to RFC
Timestamp compliants to the RFC Window scale compliant to RFC |
||
|
Description
Tommaso Pecorella
2015-02-17 17:09:26 UTC
Created attachment 2234 [details] Window scale compliant to RFC First part of the patch. It is based on the edits in bug 2262 and bug 2258. A third option on window scale is set: if the TCP receive a SYN with WS enabled, but the users has set the attribute to false, the SYN-ACK has the WS option but the shift = 1, regardless of buffer size. Pratically, WS is disabled, but we are RFC-compliant. Modified the test to respect this case. Created attachment 2236 [details]
Timestamp compliants to the RFC
(In reply to natale.patriciello from comment #2) > Created attachment 2236 [details] > Timestamp compliants to the RFC + * RFC is incostistent in case a <SYN> contains WS option, but the + * user disabled it through Attributes. + * As choice, we use the latter approach, enabling WS but advertising + * 1 as shift, regardless of the buffer size (and doing so we pratically + * disable the window scale, keeping the send of the option) The RFC is clear in Section 2.2 of 7323 that if one side disabled WS, then WS is disabled in both directions. So I would disable WS altogether if either (1) user disabled by attributes, or (2) SYN received from peer did not have WS option, rather than use scale factor of 1. so I would delete case two below: + uint8_t m_winScalingEnabled; /**!< Window Scale option enabled (RFC 7323). + * 0 = disabled + * 1 = enabled + * 2 = partially enabled (enabled but forcing shift = 1) consider to split this into two patches (timestamp and window scale); I changed the name of this bug to reflect this. Created attachment 2239 [details]
Window scale compliant to RFC
Ok, removed the case 2 (window scale is a boolean as before). Managed inside DoForwardUp (removed ReadOptions)
I have the last comment: is really necessary to use m_highTxAck or we can check against the SND.UNA ? They basically are the same. (In reply to natale.patriciello from comment #6) > I have the last comment: is really necessary to use m_highTxAck or we can > check against the SND.UNA ? They basically are the same. do you mean RCV.NXT (m_nextRxSeq of the Rx buffer)? Under delayed acks, highTxAck may lag m_nextRxSeq. (In reply to Tom Henderson from comment #7) > (In reply to natale.patriciello from comment #6) > > I have the last comment: is really necessary to use m_highTxAck or we can > > check against the SND.UNA ? They basically are the same. > > do you mean RCV.NXT (m_nextRxSeq of the Rx buffer)? Under delayed acks, > highTxAck may lag m_nextRxSeq. Oh, yes, you are right, RCV.NXT. I think it holds the same value..so there is not the need to have another variable. Do you agree? (In reply to natale.patriciello from comment #8) > Oh, yes, you are right, RCV.NXT. I think it holds the same value..so there > is not the need to have another variable. Do you agree? RCV.NXT may not be the same as lastAckSent if the TCP is delaying its ACK. If this distinction matters in the timestamp algorithm, then keep the variables separate. If not, then you may be able to get by with one. Since the RFC refers to 'last.ACK.sent' explicitly, I'd probably lean towards keeping the variable that is named in this way, for clarity. 11830:9973a3314d80 and 11831:7971d493f4c3 |