| Message ID | 20190913193629.55201-2-tph@fb.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [v4,1/2] tcp: Add TCP_INFO counter for packets received out-of-order | expand |
On Fri, Sep 13, 2019 at 3:36 PM Thomas Higdon <tph@fb.com> wrote: > > Neal Cardwell mentioned that snd_wnd would be useful for diagnosing TCP > performance problems -- > > (1) Usually when we're diagnosing TCP performance problems, we do so > > from the sender, since the sender makes most of the > > performance-critical decisions (cwnd, pacing, TSO size, TSQ, etc). > > From the sender-side the thing that would be most useful is to see > > tp->snd_wnd, the receive window that the receiver has advertised to > > the sender. > > This serves the purpose of adding an additional __u32 to avoid the > would-be hole caused by the addition of the tcpi_rcvi_ooopack field. > > Signed-off-by: Thomas Higdon <tph@fb.com> > --- > changes from v3: > - changed from rcv_wnd to snd_wnd > > include/uapi/linux/tcp.h | 1 + > net/ipv4/tcp.c | 1 + > 2 files changed, 2 insertions(+) > > diff --git a/include/uapi/linux/tcp.h b/include/uapi/linux/tcp.h > index 20237987ccc8..240654f22d98 100644 > --- a/include/uapi/linux/tcp.h > +++ b/include/uapi/linux/tcp.h > @@ -272,6 +272,7 @@ struct tcp_info { > __u32 tcpi_reord_seen; /* reordering events seen */ > > __u32 tcpi_rcv_ooopack; /* Out-of-order packets received */ > + __u32 tcpi_snd_wnd; /* Remote peer's advertised recv window size */ > }; Thanks for adding this! My run of ./scripts/checkpatch.pl is showing a warning on this line: WARNING: line over 80 characters #19: FILE: include/uapi/linux/tcp.h:273: + __u32 tcpi_snd_wnd; /* Remote peer's advertised recv window size */ What if the comment is shortened up to fit in 80 columns and the units (bytes) are added, something like: __u32 tcpi_snd_wnd; /* peer's advertised recv window (bytes) */ neal
On Fri, Sep 13, 2019 at 2:02 PM Neal Cardwell <ncardwell@google.com> wrote: > > On Fri, Sep 13, 2019 at 3:36 PM Thomas Higdon <tph@fb.com> wrote: > > > > Neal Cardwell mentioned that snd_wnd would be useful for diagnosing TCP > > performance problems -- > > > (1) Usually when we're diagnosing TCP performance problems, we do so > > > from the sender, since the sender makes most of the > > > performance-critical decisions (cwnd, pacing, TSO size, TSQ, etc). > > > From the sender-side the thing that would be most useful is to see > > > tp->snd_wnd, the receive window that the receiver has advertised to > > > the sender. > > > > This serves the purpose of adding an additional __u32 to avoid the > > would-be hole caused by the addition of the tcpi_rcvi_ooopack field. > > > > Signed-off-by: Thomas Higdon <tph@fb.com> > > --- > > changes from v3: > > - changed from rcv_wnd to snd_wnd > > > > include/uapi/linux/tcp.h | 1 + > > net/ipv4/tcp.c | 1 + > > 2 files changed, 2 insertions(+) > > > > diff --git a/include/uapi/linux/tcp.h b/include/uapi/linux/tcp.h > > index 20237987ccc8..240654f22d98 100644 > > --- a/include/uapi/linux/tcp.h > > +++ b/include/uapi/linux/tcp.h > > @@ -272,6 +272,7 @@ struct tcp_info { > > __u32 tcpi_reord_seen; /* reordering events seen */ > > > > __u32 tcpi_rcv_ooopack; /* Out-of-order packets received */ > > + __u32 tcpi_snd_wnd; /* Remote peer's advertised recv window size */ > > }; > > Thanks for adding this! > > My run of ./scripts/checkpatch.pl is showing a warning on this line: > > WARNING: line over 80 characters > #19: FILE: include/uapi/linux/tcp.h:273: > + __u32 tcpi_snd_wnd; /* Remote peer's advertised recv > window size */ > > What if the comment is shortened up to fit in 80 columns and the units > (bytes) are added, something like: > > __u32 tcpi_snd_wnd; /* peer's advertised recv window (bytes) */ just a thought: will tcpi_peer_rcv_wnd be more self-explanatory? > > neal
On Fri, Sep 13, 2019 at 5:29 PM Yuchung Cheng <ycheng@google.com> wrote: > > What if the comment is shortened up to fit in 80 columns and the units > > (bytes) are added, something like: > > > > __u32 tcpi_snd_wnd; /* peer's advertised recv window (bytes) */ > just a thought: will tcpi_peer_rcv_wnd be more self-explanatory? Good suggestion. I'm on the fence about that one. By itself, I agree tcpi_peer_rcv_wnd sounds much more clear. But tcpi_snd_wnd has the virtue of matching both the kernel code (tp->snd_wnd) and RFC 793 (SND.WND). So they both have pros and cons. Maybe someone else feels more strongly one way or the other. neal
On Fri, Sep 13, 2019 at 2:53 PM Neal Cardwell <ncardwell@google.com> wrote: > > On Fri, Sep 13, 2019 at 5:29 PM Yuchung Cheng <ycheng@google.com> wrote: > > > What if the comment is shortened up to fit in 80 columns and the units > > > (bytes) are added, something like: > > > > > > __u32 tcpi_snd_wnd; /* peer's advertised recv window (bytes) */ > > just a thought: will tcpi_peer_rcv_wnd be more self-explanatory? > > Good suggestion. I'm on the fence about that one. By itself, I agree > tcpi_peer_rcv_wnd sounds much more clear. But tcpi_snd_wnd has the > virtue of matching both the kernel code (tp->snd_wnd) and RFC 793 > (SND.WND). So they both have pros and cons. Maybe someone else feels > more strongly one way or the other. no strong preference -- snd_wnd is just fine too with proper comment (for consistency). also it might be good to comment whether it is scaled or not. it may not be obvious if the actual value and scale are small. > > neal
diff --git a/include/uapi/linux/tcp.h b/include/uapi/linux/tcp.h index 20237987ccc8..240654f22d98 100644 --- a/include/uapi/linux/tcp.h +++ b/include/uapi/linux/tcp.h @@ -272,6 +272,7 @@ struct tcp_info { __u32 tcpi_reord_seen; /* reordering events seen */ __u32 tcpi_rcv_ooopack; /* Out-of-order packets received */ + __u32 tcpi_snd_wnd; /* Remote peer's advertised recv window size */ }; /* netlink attributes types for SCM_TIMESTAMPING_OPT_STATS */ diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c index 4cf58208270e..79c325a07ba5 100644 --- a/net/ipv4/tcp.c +++ b/net/ipv4/tcp.c @@ -3297,6 +3297,7 @@ void tcp_get_info(struct sock *sk, struct tcp_info *info) info->tcpi_dsack_dups = tp->dsack_dups; info->tcpi_reord_seen = tp->reord_seen; info->tcpi_rcv_ooopack = tp->rcv_ooopack; + info->tcpi_snd_wnd = tp->snd_wnd; unlock_sock_fast(sk, slow); } EXPORT_SYMBOL_GPL(tcp_get_info);