buglwIP - A Lightweight TCP/IP stack - Bugs: bug #51154, ABC (Appropriate Byte Counting)...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #51154: ABC (Appropriate Byte Counting) fails to work with TCP_WND set to maximum.

Submitter:  Tim Cussins <tcussins>
Submitted:  Thu 01 Jun 2017 10:09:00 AM UTC
   
 
Category:  TCP Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  Fixed
Privacy:  Public Assigned to:  jcunningham
Open/Closed:  Closed Planned Release:  None
lwIP version:  git head

Jump to the original submission

Wed 14 Jun 2017 07:42:31 PM UTC, comment #13: 

Joel, thinking it through, I think it's better to leave it the way it is. The result should be correct and I feel better using TCP_SND_BUF as a limitation on write only instead of having a side effect on the actual internal TCP implementation.

-> Closed

Simon Goldschmidt <goldsimon>
Group administrator
Fri 02 Jun 2017 08:55:22 PM UTC, comment #12: 

Patch committed in 3eaf976152d781a195caa80cf1e1ed34de631c95. This fixes the introduced regression in ABC changes

Simon,

I'm open to continuing the discussion on a better way to limit cwnd.  Another thing I thought of was that our send window is also limited by TCP_SND_BUF, which is what we are using to initialize ssthresh to get our "arbitrarily high limit" recommended by the RFC.

The send window is going to be limited to the smallest of the following:
1) TCP_SND_BUF
2) snd_wnd_max (assuming empty window where snd_wnd == snd_wnd_max)
3) cwnd

So we could stop incrementing cwnd when it reaches TCP_SND_BUF, which should ensure there is no rollover.  TCP_SND_BUF is a nice compile time constant, though I know there was talk about implementing SO_SNDBUF, which would mean the number could change during the pcb's lifetime

Joel Cunningham <jcunningham>
Group Member
Fri 02 Jun 2017 07:25:03 PM UTC, comment #11: 


> TCP_WND controls our receiving window, not the remote's receive window


I know. I think until some years ago we took TCP_WND as upper limit on the send window, too. That's what I was thinking of. Anyway, it don't seems like that (any more?), so nevermind.

Simon Goldschmidt <goldsimon>
Group administrator
Fri 02 Jun 2017 06:11:53 PM UTC, comment #10: 

Looks good I guess?

Simon Goldschmidt <goldsimon>
Group administrator
Fri 02 Jun 2017 08:53:59 AM UTC, comment #9: 

Hi Joel,

Your patch resolves my issue :) Thanks!

Tim

Tim Cussins <tcussins>
Thu 01 Jun 2017 08:51:34 PM UTC, comment #8: 

Simon,

TCP_WND controls our receiving window, not the remote's receive window (which is what the sending is limited by).  Maybe we could stop at pcb->snd_wnd_max, though I'm not sure 100% sure about the relationship between snd_wnd_max and ssthresh and if its ok to stop growing cwnd when snd_wnd_max < ssthresh.

Having cwnd grow larger than snd_wnd_max is what LwIP has historically done and is safe because we do:


wnd = LWIP_MIN(pcb->snd_wnd, pcb->cwnd);


in tcp_output() to gate sending.  Setting cwnd to the maximum value in the rollover case really just lets the sender use that last bit of window for cases when snd_wnd_max == maximum value of tcpwnd_size_t, which is likely in the non-scaling case where the remote's window is 65535 or close to!


Joel Cunningham <jcunningham>
Group Member
Thu 01 Jun 2017 07:42:08 PM UTC, comment #7: 

Good catch. However, is this really correct? Shouldn't we stop at "TCP_WND" instead of "(tcpwnd_size_t)-1"?

Simon Goldschmidt <goldsimon>
Group administrator
Thu 01 Jun 2017 05:44:12 PM UTC, comment #6: 

I was able to reproduce the issue, there is integer promotion happening in the inequality that checks for rollover, meaning the check doesn't work.

Tim ran into the rollover because cwnd is 1080, which is smaller than the next segment (guessing based on 1460 SMSS), so sending stalls


if (pcb->cwnd + increase > pcb->cwnd) {
  pcb->cwnd += increase;
}


This was evaluating to TRUE even though all variables are of type tcpwnd_size_t, then rollover was occurring for pcb->cwnd += increase;

The previous code did the following:


if ((tcpwnd_size_t)(pcb->cwnd + pcb->mss) > pcb->cwnd) {
  pcb->cwnd += pcb->mss;


And the cast causes the rollover before the inequality comparison (done as an int I believe due to integer promotion rules)

I have attached a patch that introduces a new macro, TCP_WND_INC that uses the correct cast, but also sets the window to maximum value if it were to rollover.  This should ensure we can fully utilize the tcpwnd_size_t number space for cwnd.  Before, once the increment would cause a rollover, we wouldn't increment cwnd to the full value

Tim,

Can you try my patch?

(file #40837)

Joel Cunningham <jcunningham>
Group Member
Thu 01 Jun 2017 05:27:50 PM UTC, comment #5: 

Could "snd_buf = 0" be the problem?
Don't know why ABC would make a difference here though.

Simon Goldschmidt <goldsimon>
Group administrator
Thu 01 Jun 2017 03:54:28 PM UTC, comment #4: 

Hi Joel,

LWIP_WND_SCALE = 0 (default)
TCP_RCV_SCALE = 0 (default)
TCP_SND_BUF = TCP_WND = 64k-1

I'm not sure why I set TCP_SND_BUF to TCP_WND precisely.

And the pcb in stuck state: I'll just give you the whole struct :)

  local_ip = {
    addr = xxx :O
  },
  remote_ip = {
    addr = xxx :)
  },
  netif_idx = 0 '\000',
  so_options = 4 '\004',
  tos = 0 '\000',
  ttl = 255 '\377',
  next = 0x0 <>,
  callback_arg = 0x362500,
  state = ESTABLISHED,
  prio = 64 '@',
  local_port = 5002,
  remote_port = 57240,
  flags = 0,
  polltmr = 1 '\001',
  pollinterval = 2 '\002',
  last_timer = 53 '5',
  tmr = 7,
  rcv_nxt = 3825489976,
  rcv_wnd = 65535,
  rcv_ann_wnd = 65535,
  rcv_ann_right_edge = 3825555511,
  rtime = -1,
  mss = 1460,
  rttest = 0,
  rtseq = 2161524,
  sa = 1,
  sv = 3,
  rto = 3,
  nrtx = 0 '\000',
  dupacks = 0 '\000',
  lastack = 2165904,
  cwnd = 1080,
  ssthresh = 65535,
  rto_end = 0,
  snd_nxt = 2165904,
  snd_wl1 = 3825489976,
  snd_wl2 = 2165904,
  snd_lbb = 2231439,
  snd_wnd = 65535,
  snd_wnd_max = 65535,
  snd_buf = 0,
  snd_queuelen = 45,
  unsent_oversize = 165,
  bytes_acked = 0,
  unsent = 0x3620a0,
  unacked = 0x0 <>,
  ooseq = 0x0 <>,
  refused_data = 0x0 <>,
  listener = 0x34fea8,
  sent = 0x1356ec <sent_tcp>,
  recv = 0x1351f0 <recv_tcp>,
  connected = 0x0 <>,
  poll = 0x1350f0 <poll_tcp>,
  errf = 0x135418 <err_tcp>,
  keep_idle = 7200000,
  persist_cnt = 0 '\000',
  persist_backoff = 0 '\000',
  persist_probe = 0 '\000',
  keep_cnt_sent = 0 '\000'

Tim Cussins <tcussins>
Thu 01 Jun 2017 02:52:30 PM UTC, comment #3: 

If you could also report these pcb values in the stuck state:

snd_wnd
snd_wnd_max
snd_buf
snd_queuelen

So we can determine if the sending is being limited by your send buffer or the real window

Joel Cunningham <jcunningham>
Group Member
Thu 01 Jun 2017 02:25:00 PM UTC, comment #2: 

Tim,

TCP_WND is not used in the ABC or congestion control algorithms because that window is for the receiver side and ABC deals with the sender.  It's not clear to me how a window of 60K would change any behavior in the congestion control over a 64K-1 window.  Something strange is going on.

Can you provide the following values to help me understanding your configuration:

 LWIP_WND_SCALE
 TCP_RCV_SCALE
 TCP_SND_BUF 

Growth of the congestion window is checked to prevent rollover so pcb->cwnd should never get smaller (unless there is a retransmission)

Would it be possible for you to take a look at the pcb while its in the stuck state and report the following values from the pcb:

 cwnd
 ssthresh
 mss
 bytes_acked
 flags
 
Thanks!

Joel Cunningham <jcunningham>
Group Member
Thu 01 Jun 2017 10:11:52 AM UTC, comment #1: 

Typo alert! That sentence should read:

"A thread dedicated to tcp tx quickly stalls."

Tim Cussins <tcussins>
Thu 01 Jun 2017 10:09:00 AM UTC, original submission:  

Hi,

Our deployed configuration of lwIP sets TCP_WND = 64k-1, which is the maximum allowable value (value is stored as 16bit unsigned int). This delivers a large performance increase over the default value (4 * MSS).

The Appropriate Byte Counting patch (de90d03e) breaks our configuration: A thread dedicated to tcp rx quickly stalls.

Reducing our TCP_WND to 60k appears to resolve the issue, which would suggest an arithmetic overflow in the ABC code due to our large TCP_WND.

If we can establish what the maximum allowable TCP_WND is, lwIP could enforce it at compile time.

Thanks,
Tim

Tim Cussins <tcussins>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by jcunningham (Posted a comment)
  • -email is unavailable- added by tcussins (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-06-14 goldsimon Open/ClosedOpen Closed
    2017-06-02 jcunningham StatusNone Fixed
    2017-06-01 jcunningham Attached File- Added 0001-tcp-fix-cwnd-rollover-introduced-by-ABC.patch, #40837
    2017-06-01 jcunningham Assigned toNone jcunningham

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code