On Fri, Dec 02, 2016 at 06:45:24AM -0800, Eric Dumazet wrote:
> On Fri, 2016-12-02 at 08:55 -0200, Marcelo Ricardo Leitner wrote:
> > There have been some reports lately about TCP connection stalls caused
> > by NIC drivers that aren't setting gso_size on aggregated packets on rx
> > path. This causes TCP to assume that the MSS is actually the size of the
> > aggregated packet, which is invalid.
> >
> > Although the proper fix is to be done at each driver, it's often hard
> > and cumbersome for one to debug, come to such root cause and report/fix
> > it.
> >
> > This patch amends this situation in two ways. First, it adds a warning
> > on when this situation occurs, so it gives a hint to those trying to
> > debug this. It also limit the maximum probed MSS to the adverised MSS,
> > as it should never be any higher than that.
> >
> > The result is that the connection may not have the best performance ever
> > but it shouldn't stall, and the admin will have a hint on what to look
> > for.
> >
> > Tested with virtio by forcing gso_size to 0.
> >
> > Cc: Jonathan Maxwell <jmaxwel...@gmail.com>
> > Signed-off-by: Marcelo Ricardo Leitner <marcelo.leit...@gmail.com>
> > ---
> > v2: Updated msg as suggested by David.
> >
> > net/ipv4/tcp_input.c | 5 ++++-
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> > index
> > a27b9c0e27c08b4e4aeaff3d0bfdf3ae561ba4d8..fd619eb93749b6de56a41669248b337c051d9fe2
> > 100644
> > --- a/net/ipv4/tcp_input.c
> > +++ b/net/ipv4/tcp_input.c
> > @@ -144,7 +144,10 @@ static void tcp_measure_rcv_mss(struct sock *sk, const
> > struct sk_buff *skb)
> > */
> > len = skb_shinfo(skb)->gso_size ? : skb->len;
> > if (len >= icsk->icsk_ack.rcv_mss) {
> > - icsk->icsk_ack.rcv_mss = len;
> > + icsk->icsk_ack.rcv_mss = min_t(unsigned int, len,
> > + tcp_sk(sk)->advmss);
> > + if (icsk->icsk_ack.rcv_mss != len)
> > + pr_warn_once("Driver has suspect GRO implementation,
> > TCP performance may be compromised.\n");
> > } else {
> > /* Otherwise, we make more careful check taking into account,
> > * that SACKs block is variable.
>
>
> skb->dev is indeed NULL, but it might be worth getting back the device
> using skb->skb_iif maybe ?
>
Yes, then it's possible. But I have to add an extra check because it
involves a search (iif -> net_device) and I can't wrap that inside
pr_warn_once(). I hope it doesn't get too cluttered then. Posting v3 in
a few.. Thanks
