From: Lawrence Brakmo <brakmo@fb•com>
To: Kenneth Klette Jonassen <kennetkl@ifi•uio.no>
Cc: netdev <netdev@vger•kernel.org>, Kernel Team <Kernel-team@fb•com>,
"Neal Cardwell" <ncardwell@google•com>,
Eric Dumazet <eric.dumazet@gmail•com>,
Yuchung Cheng <ycheng@google•com>,
Stephen Hemminger <stephen@networkplumber•org>
Subject: Re: [RFC PATCH v5 net-next 4/4] tcp: add NV congestion control
Date: Fri, 21 Aug 2015 21:29:55 +0000 [thread overview]
Message-ID: <D1FCE296.6F6A%brakmo@fb.com> (raw)
In-Reply-To: <CA++eYdvYopM+TSiWa+gt4Z0RkjAM2Kyxvga8XodSm4w=kSmxVw@mail.gmail.com>
Kenneth, thank you for your comments, I¹ve implemented most of the
improvements you've mentioned.
I¹m finishing the new patch and the updated results, they should
be done by Monday (including cdg).
On 8/5/15, 5:51 PM, "knneth@gmail•com on behalf of Kenneth Klette
Jonassen" <knneth@gmail•com on behalf of kennetkl@ifi•uio.no> wrote:
>On Wed, Aug 5, 2015 at 3:39 AM, Lawrence Brakmo <brakmo@fb•com> wrote:
>> This is a request for comments.
>
>Nice to see more development on delay-based congestion control.
Thank you.
>
>It would be good to see how NV stacks up against CDG. Any chance of
>adding cdg as a congestion control parameter to your experiments?
>Experiments on NV without its temporary cwnd reductions would also be
>of interest -- to get a reference of how effective this mechanism is.
I¹m finishing with cdg experiments, they will be up on Monday together
with an update to the NV patch.
I will also have some experiments with variations in the temporary cwnd
reduction. This mechanism is meant to reduce min_rtt creep, but it is
now always successful. Its drawback is that it can increase high
percentile latency.
>
>
>> +#define NV_INIT_RTT 0xffffffff
>
>Maybe use U32_MAX?
Done
>
>
>> +static void tcpnv_init(struct sock *sk)
>> +{
>> + struct tcpnv *ca = inet_csk_ca(sk);
>> +
>> + tcpnv_reset(ca, sk);
>> +
>> + ca->nv_min_rtt_reset_jiffies = jiffies + 2*HZ;
>> + ca->nv_min_rtt = NV_INIT_RTT;
>> + ca->nv_min_rtt_new = NV_INIT_RTT;
>> + ca->nv_enable = nv_enable;
>
>Can this assignment be ca->nv_enable = 1? That would match the
>TCP_CA_Open case in tcpnv_state().
Done
>
>
>> + if (nv_dec_eval_min_calls > 255)
>> + nv_dec_eval_min_calls = 255;
>> + if (nv_rtt_min_cnt > 63)
>> + nv_rtt_min_cnt = 63;
>
>nv_dec_eval_min_calls can be clamped to 0-255 by changing its type to u8.
>
>nv_rtt_min_cnt can also be u8? In struct tcpnv, perhaps move
>nv_rtt_cnt to the available byte.
Done
>
>
>> +static void tcpnv_cong_avoid(struct sock *sk, u32 ack, u32 acked)
>> +{
>> + struct tcp_sock *tp = tcp_sk(sk);
>> + struct tcpnv *ca = inet_csk_ca(sk);
>> +
>> + if (!tcp_is_cwnd_limited(sk))
>> + return;
>> +
>> + /* Only grow cwnd if NV has not detected congestion */
>> + if (nv_enable && ca->nv_enable && !ca->nv_allow_cwnd_growth)
>> + return;
>
>The check for ca->nv_enable might be overly harsh on some unfortunate
>sockets in TCP_CA_Disorder. Is it needed here?
TCP_CA_Disorder should not affect ca->nv_enable in the new patch
>
>
>> +static void tcpnv_acked(struct sock *sk, struct ack_sample *sample)
>
>Maybe move some of this function to tcpnv_cong_avoid()?
It needs to be here since We need the information provided in argument
sample
>
>
>> +{
>> + const struct inet_connection_sock *icsk = inet_csk(sk);
>> + struct tcp_sock *tp = tcp_sk(sk);
>> + struct tcpnv *ca = inet_csk_ca(sk);
>> + unsigned long now = jiffies;
>> + s64 rate64 = 0;
>> + u32 rate, max_win, cwnd_by_slope;
>> + u32 avg_rtt;
>> + u32 bytes_acked = 0;
>> +
>> + /* Some calls are for duplicates without timetamps */
>> + if (sample->rtt_us < 0)
>> + return;
>> +
>> + /* If not in TCP_CA_Open state, skip. */
>> + if (icsk->icsk_ca_state != TCP_CA_Open)
>> + return;
>
>Consider using samples in other states too, especially
>TCP_CA_Disorder. Linux 4.2 enhances RTT sampling from SACKs, so any
>non-negative RTT sample should be fully usable.
Done
>
next prev parent reply other threads:[~2015-08-21 21:31 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-08-05 1:39 [RFC PATCH v5 net-next 0/4] tcp: add NV congestion control Lawrence Brakmo
2015-08-05 1:39 ` [RFC PATCH v5 net-next 1/4] tcp: replace cnt & rtt with struct in pkts_acked() Lawrence Brakmo
2015-08-05 1:39 ` [RFC PATCH v5 net-next 2/4] tcp: refactor struct tcp_skb_cb Lawrence Brakmo
2015-08-05 1:39 ` [RFC PATCH v5 net-next 3/4] tcp: add in_flight to tcp_skb_cb Lawrence Brakmo
2015-08-05 1:39 ` [RFC PATCH v5 net-next 4/4] tcp: add NV congestion control Lawrence Brakmo
2015-08-06 0:51 ` Kenneth Klette Jonassen
2015-08-21 21:29 ` Lawrence Brakmo [this message]
2015-08-26 0:05 ` Lawrence Brakmo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=D1FCE296.6F6A%brakmo@fb.com \
--to=brakmo@fb$(echo .)com \
--cc=Kernel-team@fb$(echo .)com \
--cc=eric.dumazet@gmail$(echo .)com \
--cc=kennetkl@ifi$(echo .)uio.no \
--cc=ncardwell@google$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=stephen@networkplumber$(echo .)org \
--cc=ycheng@google$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox