GetTcpInfo syscall for obtaining RTT not working |
|||||
Issue descriptionGetTcpInfo syscall is made in TCPSocketPosix to obtain TCP RTT. This value is used in multiple places (fed into the network quality estimator as well as used to collect various histograms such as Net.TcpRtt.AtDisconnect). In Canary, the count of the entries in Net.TcpRtt.AtDisconnect is showing ~50% decrease in counts (http://shortn/_qALHQf3JTX). Based on histograms, possible bad commit range: https://chromium.googlesource.com/chromium/src/+log/65.0.3311.0..65.0.3312.0?pretty=fuller&n=10000
,
Jan 8 2018
Net.TcpFastOpenSocketConnection is also showing a spike for buckets TCP_FASTOPEN_SYN_DATA_GETSOCKOPT_FAILED and TCP_FASTOPEN_NO_SYN_DATA_GETSOCKOPT_FAILED. Histogram here: http://shortn/_XqEOFY8hA7
,
Jan 8 2018
No changes in the network error codes histograms (Either total count or success rate). Also looks like the numbers are returning to their levels in early October. Given that, I'm not sure this is worth investigating, unless we understand their rise in the first place.
,
Jan 8 2018
FWIW, I can repro this on ToT. On ToT, the getsockopt() call fails.
,
Jan 8 2018
I bisected in the commit range. GetTcpInfo() stopped working after https://chromium-review.googlesource.com/c/chromium/src/+/834609 landed. CC'ing bsheedy@ if they have any insights on this.
,
Jan 9 2018
Debugged a bit more. It seems that the check to compare the size of TcpInfo struct and the value of |info_len| is no longer matching. The former is more than the latter: https://cs.chromium.org/chromium/src/net/socket/tcp_socket_posix.cc?rcl=d0827a723f95029b44b8f3b721f956243487ec31&l=156
,
Jan 9 2018
,
Jan 9 2018
Sorry, I don't think I'll be able to provide much help on this - I know some of the major changes between NDK r12 (what Chromium used before) and r16 (what we use now), such as unified headers and the removal of some MIPS headers, but no idea about what could be causing this change in behavior.
,
Jan 9 2018
Looking at the old and new tcp_info struct: old: http://shortn/_dwIc1wyT7V new: http://shortn/_uaFJLNZDT2 In the new struct, there are more fields such as |tcpi_pacing_rate|.
,
Jan 10 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e36036f34eb60ba6905e2628924094d216e36937 commit e36036f34eb60ba6905e2628924094d216e36937 Author: Tarun Bansal <tbansal@chromium.org> Date: Wed Jan 10 00:25:44 2018 Update GetTcpInfo to work on the updated Android NDK Remove the check that the length of the TcpInfo struct matches the count of bytes written by the SysCall. Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester Bug: 799989 Change-Id: I6b3273fb391f56e38691fe975f6640883afe314d Reviewed-on: https://chromium-review.googlesource.com/855502 Reviewed-by: Matt Menke <mmenke@chromium.org> Commit-Queue: Tarun Bansal <tbansal@chromium.org> Cr-Commit-Position: refs/heads/master@{#528185} [modify] https://crrev.com/e36036f34eb60ba6905e2628924094d216e36937/net/nqe/network_quality_estimator_unittest.cc [modify] https://crrev.com/e36036f34eb60ba6905e2628924094d216e36937/net/socket/tcp_socket_posix.cc
,
Jan 13 2018
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by tbansal@chromium.org
, Jan 8 2018