Re: git: d090464ecd4a - main - Change the unit of srtt and rto to usec, inspired by these in struct "tcp_info". Therefore, no need hz and tcp_rtt_scale in the headline of the log. Update the man page as well.
- In reply to: Mitchell Horne : "Re: git: d090464ecd4a - main - Change the unit of srtt and rto to usec, inspired by these in struct "tcp_info". Therefore, no need hz and tcp_rtt_scale in the headline of the log. Update the man page as well."
- Go to: [ bottom of page ] [ top of archives ] [ this month ]
Date: Tue, 25 Apr 2023 22:02:43 UTC
Apologize. I am thinking there must be some mess up between 'arc' and 'git
commit --amend',
as my original git commit message was well organized into 72
chars-per-line. The test plan
added by the 'arc' during "arc diff --create" (I thought this command is
used to create the
Phabricator review page) is automatically mixed into the updated git
message.
My mentor already notified me about this mess. Now I know how to reproduce
this problem,
and I will take care of it in the future. :)
Best Regards,
Cheng Cui
On Tue, Apr 25, 2023 at 5:52 PM Mitchell Horne <mhorne@freebsd.org> wrote:
>
>
> On 4/25/23 18:26, Cheng Cui wrote:
> > The branch main has been updated by cc:
> >
> > URL:
> https://cgit.FreeBSD.org/src/commit/?id=d090464ecd4af5cd400ef5cbbfe8409d019eac34
> >
> > commit d090464ecd4af5cd400ef5cbbfe8409d019eac34
> > Author: Cheng Cui <cc@FreeBSD.org>
> > AuthorDate: 2023-04-25 11:52:28 +0000
> > Commit: Cheng Cui <cc@FreeBSD.org>
> > CommitDate: 2023-04-25 17:26:39 +0000
> >
> > Change the unit of srtt and rto to usec, inspired by these in
> struct "tcp_info". Therefore, no need hz and tcp_rtt_scale in the headline
> of the log. Update the man page as well.
> >
> > Summary: Simplify srtt and rto values in siftr log.
> >
> > Test Plan:
> > Tested in Emulab testbed:
> > cc@s1:~ % sudo sysctl net.inet.siftr
> > net.inet.siftr.port_filter: 0
> > net.inet.siftr.genhashes: 0
> > net.inet.siftr.ppl: 1
> > net.inet.siftr.logfile: /var/log/siftr.log
> > net.inet.siftr.enabled: 0
> > cc@s1:~ % sudo sysctl net.inet.siftr.port_filter=5001
> > net.inet.siftr.port_filter: 0 -> 5001
> > cc@s1:~ % sudo sysctl net.inet.siftr.enabled=1
> > net.inet.siftr.enabled: 0 -> 1
> > cc@s1:~ %
> > cc@s1:~ % iperf -c r1 -n 1M
> > ------------------------------------------------------------
> > Client connecting to r1, TCP port 5001
> > TCP window size: 32.0 KByte (default)
> > ------------------------------------------------------------
> > [ 1] local 10.1.1.2 port 33817 connected with 10.1.1.3 port 5001
> > [ ID] Interval Transfer Bandwidth
> > [ 1] 0.00-0.91 sec 1.00 MBytes 9.22 Mbits/sec
> > cc@s1:~ % sudo sysctl net.inet.siftr.enabled=0
> > net.inet.siftr.enabled: 1 -> 0
> >
> > cc@s1:~ % ll /var/log/siftr.log
> > -rw-r--r-- 1 root wheel 91K Apr 25 09:38 /var/log/siftr.log
> > cc@s1:~ % cat /var/log/siftr.log
> > enable_time_secs=1682437111 enable_time_usecs=121115
> siftrver=1.3.0 sysname=FreeBSD sysver=1400088 ipmode=4
> >
> o,0x00000000,1682437125.907343,10.1.1.2,33817,10.1.1.3,5001,1073725440,1073725440,2,0,0,0,0,2,536,0,1,672,1000000,32768,0,65536,0,0,0,0,0
> >
> i,0x00000000,1682437126.106759,10.1.1.2,33817,10.1.1.3,5001,1073725440,1073725440,2,0,0,0,0,2,536,0,1,672,1000000,32768,0,65536,0,1,0,0,0
> >
> o,0x00000000,1682437126.106767,10.1.1.2,33817,10.1.1.3,5001,1073725440,14480,2,65535,65700,9,9,4,1460,201000,1,16778209,803000,33580,0,65700,0,0,0,0,0
> >
> o,0x00000000,1682437126.107141,10.1.1.2,33817,10.1.1.3,5001,1073725440,14480,2,65535,65700,9,9,4,1460,201000,1,16778208,803000,33580,60,65700,0,0,0,0,0
> > ...
> >
> i,0x00000000,1682437127.016754,10.1.1.2,33817,10.1.1.3,5001,1073725440,606109,1030,748544,66048,9,9,9,1460,100812,1,1008,303000,475948,0,65700,0,0,0,0,0
> >
> o,0x00000000,1682437127.016759,10.1.1.2,33817,10.1.1.3,5001,1073725440,606109,1030,748544,66048,9,9,10,1460,100812,1,1011,303000,475948,0,65700,0,0,0,0,0
> > disable_time_secs=1682437131 disable_time_usecs=767582
> num_inbound_tcp_pkts=371 num_outbound_tcp_pkts=186
> total_tcp_pkts=557 num_inbound_skipped_pkts_malloc=0
> num_outbound_skipped_pkts_malloc=0 num_inbound_skipped_pkts_tcpcb=0
> num_outbound_skipped_pkts_tcpcb=0
> num_inbound_skipped_pkts_inpcb=0 num_outbound_skipped_pkts_inpcb=0
> total_skipped_tcp_pkts=0 flow_list=10.1.1.2;33817-10.1.1.3;5001,
> >
> > Reviewers: rscheff, tuexen
> > Approved by: rscheff, tuexen
> > Subscribers: imp, melifaro, glebius
> > Differential Revision: https://reviews.freebsd.org/D39803
> > ---
> > share/man/man4/siftr.4 | 21 +++------------------
> > sys/netinet/siftr.c | 34 +++++++++++++++-------------------
> > 2 files changed, 18 insertions(+), 37 deletions(-)
> >
>
> Hello,
>
> This commit message is messy, and not easily understood. In particular
> the subject line should be shorter, and the 'Test Plan' probably should
> have been removed completely.
>
> The full set of guidelines for commit messages is documented in the
> Committers Guide:
> https://docs.freebsd.org/en/articles/committers-guide/#commit-log-message
>
> Next time you will need to clean up the message before pushing.
> Unfortunately the default output of the 'arc' tool does not generate a
> message that meets the guidelines. Ask your mentors for more information
> if you need it. Now you know :)
>
> Cheers,
> Mitchell
>
> > diff --git a/share/man/man4/siftr.4 b/share/man/man4/siftr.4
> > index 0406a38779b6..78eb755ddef1 100644
> > --- a/share/man/man4/siftr.4
> > +++ b/share/man/man4/siftr.4
> > @@ -29,7 +29,7 @@
> > .\"
> > .\" $FreeBSD$
> > .\"
> > -.Dd April 24, 2023
> > +.Dd April 25, 2023
> > .Dt SIFTR 4
> > .Os
> > .Sh NAME
> > @@ -175,14 +175,6 @@ version of
> > .Nm .
> > .El
> > .Bl -tag -offset indent -width Va
> > -.It Va hz
> > -tick rate of the kernel in ticks per second.
> > -.El
> > -.Bl -tag -offset indent -width Va
> > -.It Va tcp_rtt_scale
> > -smoothed RTT estimate scaling factor.
> > -.El
> > -.Bl -tag -offset indent -width Va
> > .It Va sysname
> > operating system name.
> > .El
> > @@ -294,11 +286,7 @@ The maximum segment size for the flow, in bytes.
> > .El
> > .Bl -tag -offset indent -width Va
> > .It Va 17
> > -The current smoothed RTT estimate for the flow, in units of
> TCP_RTT_SCALE * HZ,
> > -where TCP_RTT_SCALE is a define found in tcp_var.h, and HZ is the
> kernel's tick
> > -timer.
> > -Divide by TCP_RTT_SCALE * HZ to get the RTT in secs.
> > -TCP_RTT_SCALE and HZ are reported in the enable log message.
> > +The current smoothed RTT estimate for the flow, in units of microsecond.
> > .El
> > .Bl -tag -offset indent -width Va
> > .It Va 18
> > @@ -313,10 +301,7 @@ for information about the various flags.
> > .El
> > .Bl -tag -offset indent -width Va
> > .It Va 20
> > -The current retransmission timeout length for the flow, in units of HZ,
> where HZ
> > -is the kernel's tick timer.
> > -Divide by HZ to get the timeout length in seconds.
> > -HZ is reported in the enable log message.
> > +The current retransmission timeout length for the flow, in units
> microsecond.
> > .El
> > .Bl -tag -offset indent -width Va
> > .It Va 21
> > diff --git a/sys/netinet/siftr.c b/sys/netinet/siftr.c
> > index a23980704e17..05104627910d 100644
> > --- a/sys/netinet/siftr.c
> > +++ b/sys/netinet/siftr.c
> > @@ -207,11 +207,8 @@ struct pkt_node {
> > int conn_state;
> > /* Max Segment Size (bytes). */
> > u_int max_seg_size;
> > - /*
> > - * Smoothed RTT stored as found in the TCP control block
> > - * in units of (TCP_RTT_SCALE*hz).
> > - */
> > - int smoothed_rtt;
> > + /* Smoothed RTT (usecs). */
> > + uint32_t srtt;
> > /* Is SACK enabled? */
> > u_char sack_enabled;
> > /* Window scaling for snd window. */
> > @@ -220,8 +217,8 @@ struct pkt_node {
> > u_char rcv_scale;
> > /* TCP control block flags. */
> > u_int flags;
> > - /* Retransmit timeout length. */
> > - int rxt_length;
> > + /* Retransmission timeout (usec). */
> > + uint32_t rto;
> > /* Size of the TCP send buffer in bytes. */
> > u_int snd_buf_hiwater;
> > /* Current num bytes in the send socket buffer. */
> > @@ -453,7 +450,7 @@ siftr_process_pkt(struct pkt_node * pkt_node)
> > MAX_LOG_MSG_LEN,
> >
> "%c,0x%08x,%zd.%06ld,%x:%x:%x:%x:%x:%x:%x:%x,%u,%x:%x:%x:"
> > "%x:%x:%x:%x:%x,%u,%u,%u,%u,%u,%u,%u,%u,%u,%u,%u,%u,"
> > - "%u,%d,%u,%u,%u,%u,%u,%u,%u,%u\n",
> > + "%u,%u,%u,%u,%u,%u,%u,%u,%u,%u\n",
> > direction[pkt_node->direction],
> > pkt_node->hash,
> > pkt_node->tval.tv_sec,
> > @@ -485,10 +482,10 @@ siftr_process_pkt(struct pkt_node * pkt_node)
> > pkt_node->rcv_scale,
> > pkt_node->conn_state,
> > pkt_node->max_seg_size,
> > - pkt_node->smoothed_rtt,
> > + pkt_node->srtt,
> > pkt_node->sack_enabled,
> > pkt_node->flags,
> > - pkt_node->rxt_length,
> > + pkt_node->rto,
> > pkt_node->snd_buf_hiwater,
> > pkt_node->snd_buf_cc,
> > pkt_node->rcv_buf_hiwater,
> > @@ -512,7 +509,7 @@ siftr_process_pkt(struct pkt_node * pkt_node)
> > log_buf->ae_bytesused = snprintf(log_buf->ae_data,
> > MAX_LOG_MSG_LEN,
> >
> "%c,0x%08x,%jd.%06ld,%u.%u.%u.%u,%u,%u.%u.%u.%u,%u,%u,%u,"
> > -
> "%u,%u,%u,%u,%u,%u,%u,%u,%u,%u,%d,%u,%u,%u,%u,%u,%u,%u,%u\n",
> > +
> "%u,%u,%u,%u,%u,%u,%u,%u,%u,%u,%u,%u,%u,%u,%u,%u,%u,%u,%u\n",
> > direction[pkt_node->direction],
> > pkt_node->hash,
> > (intmax_t)pkt_node->tval.tv_sec,
> > @@ -536,10 +533,10 @@ siftr_process_pkt(struct pkt_node * pkt_node)
> > pkt_node->rcv_scale,
> > pkt_node->conn_state,
> > pkt_node->max_seg_size,
> > - pkt_node->smoothed_rtt,
> > + pkt_node->srtt,
> > pkt_node->sack_enabled,
> > pkt_node->flags,
> > - pkt_node->rxt_length,
> > + pkt_node->rto,
> > pkt_node->snd_buf_hiwater,
> > pkt_node->snd_buf_cc,
> > pkt_node->rcv_buf_hiwater,
> > @@ -785,10 +782,10 @@ siftr_siftdata(struct pkt_node *pn, struct inpcb
> *inp, struct tcpcb *tp,
> > pn->rcv_scale = tp->rcv_scale;
> > pn->conn_state = tp->t_state;
> > pn->max_seg_size = tp->t_maxseg;
> > - pn->smoothed_rtt = tp->t_srtt;
> > + pn->srtt = ((u_int64_t)tp->t_srtt * tick) >> TCP_RTT_SHIFT;
> > pn->sack_enabled = (tp->t_flags & TF_SACK_PERMIT) != 0;
> > pn->flags = tp->t_flags;
> > - pn->rxt_length = tp->t_rxtcur;
> > + pn->rto = tp->t_rxtcur * tick;
> > pn->snd_buf_hiwater = inp->inp_socket->so_snd.sb_hiwat;
> > pn->snd_buf_cc = sbused(&inp->inp_socket->so_snd);
> > pn->rcv_buf_hiwater = inp->inp_socket->so_rcv.sb_hiwat;
> > @@ -1270,10 +1267,9 @@ siftr_manage_ops(uint8_t action)
> >
> > sbuf_printf(s,
> > "enable_time_secs=%jd\tenable_time_usecs=%06ld\t"
> > - "siftrver=%s\thz=%u\ttcp_rtt_scale=%u\tsysname=%s\t"
> > - "sysver=%u\tipmode=%u\n",
> > - (intmax_t)tval.tv_sec, tval.tv_usec, MODVERSION_STR,
> hz,
> > - TCP_RTT_SCALE, SYS_NAME, __FreeBSD_version,
> SIFTR_IPMODE);
> > + "siftrver=%s\tsysname=%s\tsysver=%u\tipmode=%u\n",
> > + (intmax_t)tval.tv_sec, tval.tv_usec, MODVERSION_STR,
> > + SYS_NAME, __FreeBSD_version, SIFTR_IPMODE);
> >
> > sbuf_finish(s);
> > alq_writen(siftr_alq, sbuf_data(s), sbuf_len(s),
> ALQ_WAITOK);
>