Message ID | 1464038696.5939.29.camel@edumazet-glaptop3.roam.corp.google.com |
---|---|
State | Accepted, archived |
Delegated to: | David Miller |
Headers | show |
From: Eric Dumazet <eric.dumazet@gmail.com> Date: Mon, 23 May 2016 14:24:56 -0700 > From: Eric Dumazet <edumazet@google.com> > > I found a serious performance bug in packet schedulers using hrtimers. > > sch_htb and sch_fq are definitely impacted by this problem. > > We constantly rearm high resolution timers if some packets are throttled > in one (or more) class, and other packets are flying through qdisc on > another (non throttled) class. > > hrtimer_start() does not have the mod_timer() trick of doing nothing if > expires value does not change : > > if (timer_pending(timer) && > timer->expires == expires) > return 1; > > This issue is particularly visible when multiple cpus can queue/dequeue > packets on the same qdisc, as hrtimer code has to lock a remote base. > > I used following fix : > > 1) Change htb to use qdisc_watchdog_schedule_ns() instead of open-coding > it. > > 2) Cache watchdog prior expiration. hrtimer might provide this, but I > prefer to not rely on some hrtimer internal. > > Signed-off-by: Eric Dumazet <edumazet@google.com> This looks fine, applied, thanks Eric.
diff --git a/include/net/pkt_sched.h b/include/net/pkt_sched.h index 401038d2f9b8..fea53f4d92ca 100644 --- a/include/net/pkt_sched.h +++ b/include/net/pkt_sched.h @@ -61,6 +61,7 @@ psched_tdiff_bounded(psched_time_t tv1, psched_time_t tv2, psched_time_t bound) } struct qdisc_watchdog { + u64 last_expires; struct hrtimer timer; struct Qdisc *qdisc; }; diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c index 64f71a2155f3..ddf047df5361 100644 --- a/net/sched/sch_api.c +++ b/net/sched/sch_api.c @@ -607,6 +607,10 @@ void qdisc_watchdog_schedule_ns(struct qdisc_watchdog *wd, u64 expires, bool thr if (throttle) qdisc_throttled(wd->qdisc); + if (wd->last_expires == expires) + return; + + wd->last_expires = expires; hrtimer_start(&wd->timer, ns_to_ktime(expires), HRTIMER_MODE_ABS_PINNED); diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c index f6bf5818ed4d..d4b4218af6b1 100644 --- a/net/sched/sch_htb.c +++ b/net/sched/sch_htb.c @@ -928,17 +928,10 @@ ok: } } qdisc_qstats_overlimit(sch); - if (likely(next_event > q->now)) { - if (!test_bit(__QDISC_STATE_DEACTIVATED, - &qdisc_root_sleeping(q->watchdog.qdisc)->state)) { - ktime_t time = ns_to_ktime(next_event); - qdisc_throttled(q->watchdog.qdisc); - hrtimer_start(&q->watchdog.timer, time, - HRTIMER_MODE_ABS_PINNED); - } - } else { + if (likely(next_event > q->now)) + qdisc_watchdog_schedule_ns(&q->watchdog, next_event, true); + else schedule_work(&q->work); - } fin: return skb; }