Date: Tue, 25 Apr 2023 18:52:56 -0300 From: Mitchell Horne <mhorne@freebsd.org> To: Cheng Cui <cc@FreeBSD.org>, src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: 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. Message-ID: <1ead933c-2567-7b79-8761-2f900ccf9b76@freebsd.org> In-Reply-To: <202304252126.33PLQsKe024634@gitrepo.freebsd.org> References: <202304252126.33PLQsKe024634@gitrepo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1ead933c-2567-7b79-8761-2f900ccf9b76>