netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net] net_sched: sch_fq: properly set sk->sk_pacing_status
@ 2019-12-23 19:13 Eric Dumazet
  2019-12-23 19:18 ` Soheil Hassas Yeganeh
  2019-12-26 23:35 ` David Miller
  0 siblings, 2 replies; 3+ messages in thread
From: Eric Dumazet @ 2019-12-23 19:13 UTC (permalink / raw)
  To: David S . Miller
  Cc: netdev, Eric Dumazet, Eric Dumazet, Soheil Hassas Yeganeh, Neal Cardwell

If fq_classify() recycles a struct fq_flow because
a socket structure has been reallocated, we do not
set sk->sk_pacing_status immediately, but later if the
flow becomes detached.

This means that any flow requiring pacing (BBR, or SO_MAX_PACING_RATE)
might fallback to TCP internal pacing, which requires a per-socket
high resolution timer, and therefore more cpu cycles.

Fixes: 218af599fa63 ("tcp: internal implementation for pacing")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Soheil Hassas Yeganeh <soheil@google.com>
Cc: Neal Cardwell <ncardwell@google.com>
---
 net/sched/sch_fq.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/net/sched/sch_fq.c b/net/sched/sch_fq.c
index b1c7e726ce5d1ae139f765c5b92dfdaea9bee258..ff4c5e9d0d7778d86f20f4bd67cc627eed0713d9 100644
--- a/net/sched/sch_fq.c
+++ b/net/sched/sch_fq.c
@@ -301,6 +301,9 @@ static struct fq_flow *fq_classify(struct sk_buff *skb, struct fq_sched_data *q)
 				     f->socket_hash != sk->sk_hash)) {
 				f->credit = q->initial_quantum;
 				f->socket_hash = sk->sk_hash;
+				if (q->rate_enable)
+					smp_store_release(&sk->sk_pacing_status,
+							  SK_PACING_FQ);
 				if (fq_flow_is_throttled(f))
 					fq_flow_unset_throttled(q, f);
 				f->time_next_packet = 0ULL;
@@ -322,8 +325,12 @@ static struct fq_flow *fq_classify(struct sk_buff *skb, struct fq_sched_data *q)
 
 	fq_flow_set_detached(f);
 	f->sk = sk;
-	if (skb->sk == sk)
+	if (skb->sk == sk) {
 		f->socket_hash = sk->sk_hash;
+		if (q->rate_enable)
+			smp_store_release(&sk->sk_pacing_status,
+					  SK_PACING_FQ);
+	}
 	f->credit = q->initial_quantum;
 
 	rb_link_node(&f->fq_node, parent, p);
@@ -428,17 +435,9 @@ static int fq_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 	f->qlen++;
 	qdisc_qstats_backlog_inc(sch, skb);
 	if (fq_flow_is_detached(f)) {
-		struct sock *sk = skb->sk;
-
 		fq_flow_add_tail(&q->new_flows, f);
 		if (time_after(jiffies, f->age + q->flow_refill_delay))
 			f->credit = max_t(u32, f->credit, q->quantum);
-		if (sk && q->rate_enable) {
-			if (unlikely(smp_load_acquire(&sk->sk_pacing_status) !=
-				     SK_PACING_FQ))
-				smp_store_release(&sk->sk_pacing_status,
-						  SK_PACING_FQ);
-		}
 		q->inactive_flows--;
 	}
 
-- 
2.24.1.735.g03f4e72817-goog


^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH net] net_sched: sch_fq: properly set sk->sk_pacing_status
  2019-12-23 19:13 [PATCH net] net_sched: sch_fq: properly set sk->sk_pacing_status Eric Dumazet
@ 2019-12-23 19:18 ` Soheil Hassas Yeganeh
  2019-12-26 23:35 ` David Miller
  1 sibling, 0 replies; 3+ messages in thread
From: Soheil Hassas Yeganeh @ 2019-12-23 19:18 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: David S . Miller, netdev, Eric Dumazet, Neal Cardwell

On Mon, Dec 23, 2019 at 2:13 PM Eric Dumazet <edumazet@google.com> wrote:
>
> If fq_classify() recycles a struct fq_flow because
> a socket structure has been reallocated, we do not
> set sk->sk_pacing_status immediately, but later if the
> flow becomes detached.
>
> This means that any flow requiring pacing (BBR, or SO_MAX_PACING_RATE)
> might fallback to TCP internal pacing, which requires a per-socket
> high resolution timer, and therefore more cpu cycles.
>
> Fixes: 218af599fa63 ("tcp: internal implementation for pacing")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Soheil Hassas Yeganeh <soheil@google.com>
> Cc: Neal Cardwell <ncardwell@google.com>

Acked-by: Soheil Hassas Yeganeh <soheil@google.com>

Nice catch! Thanks for the fix!

> ---
>  net/sched/sch_fq.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/net/sched/sch_fq.c b/net/sched/sch_fq.c
> index b1c7e726ce5d1ae139f765c5b92dfdaea9bee258..ff4c5e9d0d7778d86f20f4bd67cc627eed0713d9 100644
> --- a/net/sched/sch_fq.c
> +++ b/net/sched/sch_fq.c
> @@ -301,6 +301,9 @@ static struct fq_flow *fq_classify(struct sk_buff *skb, struct fq_sched_data *q)
>                                      f->socket_hash != sk->sk_hash)) {
>                                 f->credit = q->initial_quantum;
>                                 f->socket_hash = sk->sk_hash;
> +                               if (q->rate_enable)
> +                                       smp_store_release(&sk->sk_pacing_status,
> +                                                         SK_PACING_FQ);
>                                 if (fq_flow_is_throttled(f))
>                                         fq_flow_unset_throttled(q, f);
>                                 f->time_next_packet = 0ULL;
> @@ -322,8 +325,12 @@ static struct fq_flow *fq_classify(struct sk_buff *skb, struct fq_sched_data *q)
>
>         fq_flow_set_detached(f);
>         f->sk = sk;
> -       if (skb->sk == sk)
> +       if (skb->sk == sk) {
>                 f->socket_hash = sk->sk_hash;
> +               if (q->rate_enable)
> +                       smp_store_release(&sk->sk_pacing_status,
> +                                         SK_PACING_FQ);
> +       }
>         f->credit = q->initial_quantum;
>
>         rb_link_node(&f->fq_node, parent, p);
> @@ -428,17 +435,9 @@ static int fq_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>         f->qlen++;
>         qdisc_qstats_backlog_inc(sch, skb);
>         if (fq_flow_is_detached(f)) {
> -               struct sock *sk = skb->sk;
> -
>                 fq_flow_add_tail(&q->new_flows, f);
>                 if (time_after(jiffies, f->age + q->flow_refill_delay))
>                         f->credit = max_t(u32, f->credit, q->quantum);
> -               if (sk && q->rate_enable) {
> -                       if (unlikely(smp_load_acquire(&sk->sk_pacing_status) !=
> -                                    SK_PACING_FQ))
> -                               smp_store_release(&sk->sk_pacing_status,
> -                                                 SK_PACING_FQ);
> -               }
>                 q->inactive_flows--;
>         }
>
> --
> 2.24.1.735.g03f4e72817-goog
>

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH net] net_sched: sch_fq: properly set sk->sk_pacing_status
  2019-12-23 19:13 [PATCH net] net_sched: sch_fq: properly set sk->sk_pacing_status Eric Dumazet
  2019-12-23 19:18 ` Soheil Hassas Yeganeh
@ 2019-12-26 23:35 ` David Miller
  1 sibling, 0 replies; 3+ messages in thread
From: David Miller @ 2019-12-26 23:35 UTC (permalink / raw)
  To: edumazet; +Cc: netdev, eric.dumazet, soheil, ncardwell

From: Eric Dumazet <edumazet@google.com>
Date: Mon, 23 Dec 2019 11:13:24 -0800

> If fq_classify() recycles a struct fq_flow because
> a socket structure has been reallocated, we do not
> set sk->sk_pacing_status immediately, but later if the
> flow becomes detached.
> 
> This means that any flow requiring pacing (BBR, or SO_MAX_PACING_RATE)
> might fallback to TCP internal pacing, which requires a per-socket
> high resolution timer, and therefore more cpu cycles.
> 
> Fixes: 218af599fa63 ("tcp: internal implementation for pacing")
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Applied and queued up for -stable, thanks Eric.

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-12-26 23:36 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-23 19:13 [PATCH net] net_sched: sch_fq: properly set sk->sk_pacing_status Eric Dumazet
2019-12-23 19:18 ` Soheil Hassas Yeganeh
2019-12-26 23:35 ` David Miller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).