* [PATCH net-next] tcp: ensure to use the most recently sent skb when filling the rate sample
@ 2022-04-13 10:54 Pengcheng Yang
2022-04-14 16:11 ` Neal Cardwell
0 siblings, 1 reply; 3+ messages in thread
From: Pengcheng Yang @ 2022-04-13 10:54 UTC (permalink / raw)
To: Eric Dumazet, Yuchung Cheng, Neal Cardwell, netdev; +Cc: Pengcheng Yang
If an ACK (s)acks multiple skbs, we favor the information
from the most recently sent skb by choosing the skb with
the highest prior_delivered count.
But prior_delivered may be equal, because tp->delivered only
changes when receiving, which requires further comparison of
skb timestamp.
Signed-off-by: Pengcheng Yang <yangpc@wangsu.com>
---
net/ipv4/tcp_rate.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/net/ipv4/tcp_rate.c b/net/ipv4/tcp_rate.c
index 617b818..ad893ad 100644
--- a/net/ipv4/tcp_rate.c
+++ b/net/ipv4/tcp_rate.c
@@ -86,7 +86,9 @@ void tcp_rate_skb_delivered(struct sock *sk, struct sk_buff *skb,
return;
if (!rs->prior_delivered ||
- after(scb->tx.delivered, rs->prior_delivered)) {
+ after(scb->tx.delivered, rs->prior_delivered) ||
+ (scb->tx.delivered == rs->prior_delivered &&
+ tcp_skb_timestamp_us(skb) > tp->first_tx_mstamp)) {
rs->prior_delivered_ce = scb->tx.delivered_ce;
rs->prior_delivered = scb->tx.delivered;
rs->prior_mstamp = scb->tx.delivered_mstamp;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH net-next] tcp: ensure to use the most recently sent skb when filling the rate sample
2022-04-13 10:54 [PATCH net-next] tcp: ensure to use the most recently sent skb when filling the rate sample Pengcheng Yang
@ 2022-04-14 16:11 ` Neal Cardwell
2022-04-15 16:44 ` Pengcheng Yang
0 siblings, 1 reply; 3+ messages in thread
From: Neal Cardwell @ 2022-04-14 16:11 UTC (permalink / raw)
To: Pengcheng Yang; +Cc: Eric Dumazet, Yuchung Cheng, netdev
On Wed, Apr 13, 2022 at 6:54 AM Pengcheng Yang <yangpc@wangsu.com> wrote:
>
> If an ACK (s)acks multiple skbs, we favor the information
> from the most recently sent skb by choosing the skb with
> the highest prior_delivered count.
> But prior_delivered may be equal, because tp->delivered only
> changes when receiving, which requires further comparison of
> skb timestamp.
>
> Signed-off-by: Pengcheng Yang <yangpc@wangsu.com>
> ---
> net/ipv4/tcp_rate.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/net/ipv4/tcp_rate.c b/net/ipv4/tcp_rate.c
> index 617b818..ad893ad 100644
> --- a/net/ipv4/tcp_rate.c
> +++ b/net/ipv4/tcp_rate.c
> @@ -86,7 +86,9 @@ void tcp_rate_skb_delivered(struct sock *sk, struct sk_buff *skb,
> return;
>
> if (!rs->prior_delivered ||
> - after(scb->tx.delivered, rs->prior_delivered)) {
> + after(scb->tx.delivered, rs->prior_delivered) ||
> + (scb->tx.delivered == rs->prior_delivered &&
> + tcp_skb_timestamp_us(skb) > tp->first_tx_mstamp)) {
> rs->prior_delivered_ce = scb->tx.delivered_ce;
> rs->prior_delivered = scb->tx.delivered;
> rs->prior_mstamp = scb->tx.delivered_mstamp;
> --
Thank you for posting this patch! I agree there is a bug there, and
your patch is an improvement. However, I think this patch is not a
complete solution, since it does not handle the case where there are
multiple skbs with the tcp_skb_timestamp_us() (which can happen if a
outgoing buffered TSO/GSO skb is later split into multiple skbs with
the same timestamp).
RACK has to deal with the same question "which skb was sent first?",
and already has a solution in tcp_rack_sent_after(). I suggest we
share code between RACK and tcp_rate_skb_delivered() to make this
check. This might involve making a copy of tcp_rack_sent_after() in
include/net/tcp.h, naming the .h copy to tcp_skb_sent_after(), and
reworking this logic to save and use the sequence number and timestamp
so that it can use the new tcp_skb_sent_after() helper. After this fix
propagates to net-next we could later then change RACK to use the new
tcp_skb_sent_after() function, so we have a single helper used in two
places.
If you want to post a version of this patch that uses some approach
like that, IMHO that would be welcome. If you do not have cycles, I am
happy to post one when I get a moment.
thanks,
neal
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH net-next] tcp: ensure to use the most recently sent skb when filling the rate sample
2022-04-14 16:11 ` Neal Cardwell
@ 2022-04-15 16:44 ` Pengcheng Yang
0 siblings, 0 replies; 3+ messages in thread
From: Pengcheng Yang @ 2022-04-15 16:44 UTC (permalink / raw)
To: 'Neal Cardwell'
Cc: 'Eric Dumazet', 'Yuchung Cheng', netdev
On Fri, Apr 15, 2022 at 12:12 AM Neal Cardwell <ncardwell@google.com> wrote:
>
> On Wed, Apr 13, 2022 at 6:54 AM Pengcheng Yang <yangpc@wangsu.com> wrote:
> >
> > If an ACK (s)acks multiple skbs, we favor the information
> > from the most recently sent skb by choosing the skb with
> > the highest prior_delivered count.
> > But prior_delivered may be equal, because tp->delivered only
> > changes when receiving, which requires further comparison of
> > skb timestamp.
> >
> > Signed-off-by: Pengcheng Yang <yangpc@wangsu.com>
>
> Thank you for posting this patch! I agree there is a bug there, and
> your patch is an improvement. However, I think this patch is not a
> complete solution, since it does not handle the case where there are
> multiple skbs with the tcp_skb_timestamp_us() (which can happen if a
> outgoing buffered TSO/GSO skb is later split into multiple skbs with
> the same timestamp).
>
My initial thought was that this case would not affect the correctness of rate_sample,
since the timestamp of these skbs are the same.
So I am confused whether we have to find the *real* most recently (with the highest seq) skb
(at the cost of a little extra cost)?
> RACK has to deal with the same question "which skb was sent first?",
> and already has a solution in tcp_rack_sent_after(). I suggest we
> share code between RACK and tcp_rate_skb_delivered() to make this
> check. This might involve making a copy of tcp_rack_sent_after() in
> include/net/tcp.h, naming the .h copy to tcp_skb_sent_after(), and
> reworking this logic to save and use the sequence number and timestamp
> so that it can use the new tcp_skb_sent_after() helper. After this fix
> propagates to net-next we could later then change RACK to use the new
> tcp_skb_sent_after() function, so we have a single helper used in two
> places.
>
Ok. I will send the V2 later according to your suggestion. Thanks neal.
> If you want to post a version of this patch that uses some approach
> like that, IMHO that would be welcome. If you do not have cycles, I am
> happy to post one when I get a moment.
>
> thanks,
> neal
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-04-15 16:44 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-13 10:54 [PATCH net-next] tcp: ensure to use the most recently sent skb when filling the rate sample Pengcheng Yang
2022-04-14 16:11 ` Neal Cardwell
2022-04-15 16:44 ` Pengcheng Yang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.