| Message ID | 1532746900-11710-2-git-send-email-laoar.shao@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | None | expand |
On Fri, Jul 27, 2018 at 8:02 PM Yafang Shao <laoar.shao@gmail.com> wrote: > > Currently the collapsed SKB doesn't propagate the GSO information to the > new SKB. > The GSO should be propagated for better tracking, i.e. when this SKB is > dropped we could know how many network segments are dropped. What is "the GSO" ? > > Signed-off-by: Yafang Shao <laoar.shao@gmail.com> > --- > net/ipv4/tcp_input.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c > index 90f83eb..af52e4e 100644 > --- a/net/ipv4/tcp_input.c > +++ b/net/ipv4/tcp_input.c > @@ -4893,6 +4893,8 @@ void tcp_rbtree_insert(struct rb_root *root, struct sk_buff *skb) > if (!nskb) > break; > > + skb_shinfo(nskb)->gso_size = skb_shinfo(skb)->gso_size; > + skb_shinfo(nskb)->gso_type = skb_shinfo(skb)->gso_type; Why gso_size and gso_type are important ? Where later in the stack these values are used ? > memcpy(nskb->cb, skb->cb, sizeof(skb->cb)); > #ifdef CONFIG_TLS_DEVICE > nskb->decrypted = skb->decrypted; > @@ -4906,18 +4908,24 @@ void tcp_rbtree_insert(struct rb_root *root, struct sk_buff *skb) > > /* Copy data, releasing collapsed skbs. */ > while (copy > 0) { > - int offset = start - TCP_SKB_CB(skb)->seq; > int size = TCP_SKB_CB(skb)->end_seq - start; > + int offset = start - TCP_SKB_CB(skb)->seq; > > BUG_ON(offset < 0); > if (size > 0) { > - size = min(copy, size); > + if (copy >= size) > + skb_shinfo(nskb)->gso_segs += > + max_t(u16, 1, skb_shinfo(skb)->gso_segs); > + else > + size = copy; > + So... what happens if copy was partial ? Your patch does not really fix the uncertainty, it merely shifts it a bit.
On Sat, Jul 28, 2018 at 11:13 AM, Eric Dumazet <edumazet@google.com> wrote: > On Fri, Jul 27, 2018 at 8:02 PM Yafang Shao <laoar.shao@gmail.com> wrote: >> >> Currently the collapsed SKB doesn't propagate the GSO information to the >> new SKB. >> The GSO should be propagated for better tracking, i.e. when this SKB is >> dropped we could know how many network segments are dropped. > > What is "the GSO" ? > I mean gso_segs, gso_type and gso_size, which are all set in GRO. >> >> Signed-off-by: Yafang Shao <laoar.shao@gmail.com> >> --- >> net/ipv4/tcp_input.c | 12 ++++++++++-- >> 1 file changed, 10 insertions(+), 2 deletions(-) >> >> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c >> index 90f83eb..af52e4e 100644 >> --- a/net/ipv4/tcp_input.c >> +++ b/net/ipv4/tcp_input.c >> @@ -4893,6 +4893,8 @@ void tcp_rbtree_insert(struct rb_root *root, struct sk_buff *skb) >> if (!nskb) >> break; >> >> + skb_shinfo(nskb)->gso_size = skb_shinfo(skb)->gso_size; >> + skb_shinfo(nskb)->gso_type = skb_shinfo(skb)->gso_type; > > Why gso_size and gso_type are important ? > > Where later in the stack these values are used ? > I'm not sure it is important or not. I just worry it may be used latter. >> memcpy(nskb->cb, skb->cb, sizeof(skb->cb)); >> #ifdef CONFIG_TLS_DEVICE >> nskb->decrypted = skb->decrypted; >> @@ -4906,18 +4908,24 @@ void tcp_rbtree_insert(struct rb_root *root, struct sk_buff *skb) >> >> /* Copy data, releasing collapsed skbs. */ >> while (copy > 0) { >> - int offset = start - TCP_SKB_CB(skb)->seq; >> int size = TCP_SKB_CB(skb)->end_seq - start; >> + int offset = start - TCP_SKB_CB(skb)->seq; > >> >> BUG_ON(offset < 0); >> if (size > 0) { >> - size = min(copy, size); >> + if (copy >= size) >> + skb_shinfo(nskb)->gso_segs += >> + max_t(u16, 1, skb_shinfo(skb)->gso_segs); >> + else >> + size = copy; >> + > > So... what happens if copy was partial ? > In the current patch, if copy was parial, the gso_segs are in the orignal SKB as it will not be freed now. If that is not ok, what about the bellow change ? else { size = copy; skb_shinfo(nskb)->gso_segs += DIV_ROUND_UP(size, skb_shinfo(nskb)->gso_size); } > Your patch does not really fix the uncertainty, it merely shifts it a bit.
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c index 90f83eb..af52e4e 100644 --- a/net/ipv4/tcp_input.c +++ b/net/ipv4/tcp_input.c @@ -4893,6 +4893,8 @@ void tcp_rbtree_insert(struct rb_root *root, struct sk_buff *skb) if (!nskb) break; + skb_shinfo(nskb)->gso_size = skb_shinfo(skb)->gso_size; + skb_shinfo(nskb)->gso_type = skb_shinfo(skb)->gso_type; memcpy(nskb->cb, skb->cb, sizeof(skb->cb)); #ifdef CONFIG_TLS_DEVICE nskb->decrypted = skb->decrypted; @@ -4906,18 +4908,24 @@ void tcp_rbtree_insert(struct rb_root *root, struct sk_buff *skb) /* Copy data, releasing collapsed skbs. */ while (copy > 0) { - int offset = start - TCP_SKB_CB(skb)->seq; int size = TCP_SKB_CB(skb)->end_seq - start; + int offset = start - TCP_SKB_CB(skb)->seq; BUG_ON(offset < 0); if (size > 0) { - size = min(copy, size); + if (copy >= size) + skb_shinfo(nskb)->gso_segs += + max_t(u16, 1, skb_shinfo(skb)->gso_segs); + else + size = copy; + if (skb_copy_bits(skb, offset, skb_put(nskb, size), size)) BUG(); TCP_SKB_CB(nskb)->end_seq += size; copy -= size; start += size; } + if (!before(start, TCP_SKB_CB(skb)->end_seq)) { skb = tcp_collapse_one(sk, skb, list, root); if (!skb ||
Currently the collapsed SKB doesn't propagate the GSO information to the new SKB. The GSO should be propagated for better tracking, i.e. when this SKB is dropped we could know how many network segments are dropped. Signed-off-by: Yafang Shao <laoar.shao@gmail.com> --- net/ipv4/tcp_input.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)