Skip to content

Commit bd808ca

Browse files
kuba-mookuba-moo
committed
Merge branch 'net_sched-adapt-qdiscs-for-reentrant-enqueue-cases'
Victor Nogueira says: ==================== net_sched: Adapt qdiscs for reentrant enqueue cases As described in Gerrard's report [1], there are cases where netem can make the qdisc enqueue callback reentrant. Some qdiscs (drr, hfsc, ets, qfq) break whenever the enqueue callback has reentrant behaviour. This series addresses these issues by adding extra checks that cater for these reentrant corner cases. This series has passed all relevant test cases in the TDC suite. [1] https://lore.kernel.org/netdev/CAHcdcOm+03OD2j6R0=YHKqmy=VgJ8xEOKuP6c7mSgnp-TEJJbw@mail.gmail.com/ ==================== Link: https://patch.msgid.link/[email protected] Signed-off-by: Jakub Kicinski <[email protected]>
2 parents dfd7601 + a6e1c5a commit bd808ca

File tree

5 files changed

+206
-11
lines changed

5 files changed

+206
-11
lines changed

‎net/sched/sch_drr.c‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,11 @@ struct drr_sched {
3535
struct Qdisc_class_hash clhash;
3636
};
3737

38+
static bool cl_is_active(struct drr_class *cl)
39+
{
40+
return !list_empty(&cl->alist);
41+
}
42+
3843
static struct drr_class *drr_find_class(struct Qdisc *sch, u32 classid)
3944
{
4045
struct drr_sched *q = qdisc_priv(sch);
@@ -337,7 +342,6 @@ static int drr_enqueue(struct sk_buff *skb, struct Qdisc *sch,
337342
struct drr_sched *q = qdisc_priv(sch);
338343
struct drr_class *cl;
339344
int err = 0;
340-
bool first;
341345

342346
cl = drr_classify(skb, sch, &err);
343347
if (cl == NULL) {
@@ -347,7 +351,6 @@ static int drr_enqueue(struct sk_buff *skb, struct Qdisc *sch,
347351
return err;
348352
}
349353

350-
first = !cl->qdisc->q.qlen;
351354
err = qdisc_enqueue(skb, cl->qdisc, to_free);
352355
if (unlikely(err != NET_XMIT_SUCCESS)) {
353356
if (net_xmit_drop_count(err)) {
@@ -357,7 +360,7 @@ static int drr_enqueue(struct sk_buff *skb, struct Qdisc *sch,
357360
return err;
358361
}
359362

360-
if (first) {
363+
if (!cl_is_active(cl)) {
361364
list_add_tail(&cl->alist, &q->active);
362365
cl->deficit = cl->quantum;
363366
}

‎net/sched/sch_ets.c‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,11 @@ static const struct nla_policy ets_class_policy[TCA_ETS_MAX + 1] = {
7474
[TCA_ETS_QUANTA_BAND] = { .type = NLA_U32 },
7575
};
7676

77+
static bool cl_is_active(struct ets_class *cl)
78+
{
79+
return !list_empty(&cl->alist);
80+
}
81+
7782
static int ets_quantum_parse(struct Qdisc *sch, const struct nlattr *attr,
7883
unsigned int *quantum,
7984
struct netlink_ext_ack *extack)
@@ -416,7 +421,6 @@ static int ets_qdisc_enqueue(struct sk_buff *skb, struct Qdisc *sch,
416421
struct ets_sched *q = qdisc_priv(sch);
417422
struct ets_class *cl;
418423
int err = 0;
419-
bool first;
420424

421425
cl = ets_classify(skb, sch, &err);
422426
if (!cl) {
@@ -426,7 +430,6 @@ static int ets_qdisc_enqueue(struct sk_buff *skb, struct Qdisc *sch,
426430
return err;
427431
}
428432

429-
first = !cl->qdisc->q.qlen;
430433
err = qdisc_enqueue(skb, cl->qdisc, to_free);
431434
if (unlikely(err != NET_XMIT_SUCCESS)) {
432435
if (net_xmit_drop_count(err)) {
@@ -436,7 +439,7 @@ static int ets_qdisc_enqueue(struct sk_buff *skb, struct Qdisc *sch,
436439
return err;
437440
}
438441

439-
if (first && !ets_class_is_strict(q, cl)) {
442+
if (!cl_is_active(cl) && !ets_class_is_strict(q, cl)) {
440443
list_add_tail(&cl->alist, &q->active);
441444
cl->deficit = cl->quantum;
442445
}

‎net/sched/sch_hfsc.c‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1569,7 +1569,7 @@ hfsc_enqueue(struct sk_buff *skb, struct Qdisc *sch, struct sk_buff **to_free)
15691569
return err;
15701570
}
15711571

1572-
if (first) {
1572+
if (first && !cl->cl_nactive) {
15731573
if (cl->cl_flags & HFSC_RSC)
15741574
init_ed(cl, len);
15751575
if (cl->cl_flags & HFSC_FSC)

‎net/sched/sch_qfq.c‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -202,6 +202,11 @@ struct qfq_sched {
202202
*/
203203
enum update_reason {enqueue, requeue};
204204

205+
static bool cl_is_active(struct qfq_class *cl)
206+
{
207+
return !list_empty(&cl->alist);
208+
}
209+
205210
static struct qfq_class *qfq_find_class(struct Qdisc *sch, u32 classid)
206211
{
207212
struct qfq_sched *q = qdisc_priv(sch);
@@ -1215,7 +1220,6 @@ static int qfq_enqueue(struct sk_buff *skb, struct Qdisc *sch,
12151220
struct qfq_class *cl;
12161221
struct qfq_aggregate *agg;
12171222
int err = 0;
1218-
bool first;
12191223

12201224
cl = qfq_classify(skb, sch, &err);
12211225
if (cl == NULL) {
@@ -1237,7 +1241,6 @@ static int qfq_enqueue(struct sk_buff *skb, struct Qdisc *sch,
12371241
}
12381242

12391243
gso_segs = skb_is_gso(skb) ? skb_shinfo(skb)->gso_segs : 1;
1240-
first = !cl->qdisc->q.qlen;
12411244
err = qdisc_enqueue(skb, cl->qdisc, to_free);
12421245
if (unlikely(err != NET_XMIT_SUCCESS)) {
12431246
pr_debug("qfq_enqueue: enqueue failed %d\n", err);
@@ -1253,8 +1256,8 @@ static int qfq_enqueue(struct sk_buff *skb, struct Qdisc *sch,
12531256
++sch->q.qlen;
12541257

12551258
agg = cl->agg;
1256-
/* if the queue was not empty, then done here */
1257-
if (!first) {
1259+
/* if the class is active, then done here */
1260+
if (cl_is_active(cl)) {
12581261
if (unlikely(skb == cl->qdisc->ops->peek(cl->qdisc)) &&
12591262
list_first_entry(&agg->active, struct qfq_class, alist)
12601263
== cl && cl->deficit < len)

‎tools/testing/selftests/tc-testing/tc-tests/infra/qdiscs.json‎

Lines changed: 186 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -352,5 +352,191 @@
352352
"$TC qdisc del dev $DUMMY handle 1:0 root",
353353
"$IP addr del 10.10.10.10/24 dev $DUMMY || true"
354354
]
355+
},
356+
{
357+
"id": "90ec",
358+
"name": "Test DRR's enqueue reentrant behaviour with netem",
359+
"category": [
360+
"qdisc",
361+
"drr"
362+
],
363+
"plugins": {
364+
"requires": "nsPlugin"
365+
},
366+
"setup": [
367+
"$IP link set dev $DUMMY up || true",
368+
"$IP addr add 10.10.10.10/24 dev $DUMMY || true",
369+
"$TC qdisc add dev $DUMMY handle 1:0 root drr",
370+
"$TC class replace dev $DUMMY parent 1:0 classid 1:1 drr",
371+
"$TC qdisc add dev $DUMMY parent 1:1 handle 2:0 netem duplicate 100%",
372+
"$TC filter add dev $DUMMY parent 1:0 protocol ip prio 1 u32 match ip protocol 1 0xff flowid 1:1"
373+
],
374+
"cmdUnderTest": "ping -c 1 -I $DUMMY 10.10.10.1 > /dev/null || true",
375+
"expExitCode": "0",
376+
"verifyCmd": "$TC -j -s qdisc ls dev $DUMMY handle 1:0",
377+
"matchJSON": [
378+
{
379+
"kind": "drr",
380+
"handle": "1:",
381+
"bytes": 196,
382+
"packets": 2
383+
}
384+
],
385+
"matchCount": "1",
386+
"teardown": [
387+
"$TC qdisc del dev $DUMMY handle 1:0 root",
388+
"$IP addr del 10.10.10.10/24 dev $DUMMY || true"
389+
]
390+
},
391+
{
392+
"id": "1f1f",
393+
"name": "Test ETS's enqueue reentrant behaviour with netem",
394+
"category": [
395+
"qdisc",
396+
"ets"
397+
],
398+
"plugins": {
399+
"requires": "nsPlugin"
400+
},
401+
"setup": [
402+
"$IP link set dev $DUMMY up || true",
403+
"$IP addr add 10.10.10.10/24 dev $DUMMY || true",
404+
"$TC qdisc add dev $DUMMY handle 1:0 root ets bands 2",
405+
"$TC class replace dev $DUMMY parent 1:0 classid 1:1 ets quantum 1500",
406+
"$TC qdisc add dev $DUMMY parent 1:1 handle 2:0 netem duplicate 100%",
407+
"$TC filter add dev $DUMMY parent 1:0 protocol ip prio 1 u32 match ip protocol 1 0xff flowid 1:1"
408+
],
409+
"cmdUnderTest": "ping -c 1 -I $DUMMY 10.10.10.1 > /dev/null || true",
410+
"expExitCode": "0",
411+
"verifyCmd": "$TC -j -s class show dev $DUMMY",
412+
"matchJSON": [
413+
{
414+
"class": "ets",
415+
"handle": "1:1",
416+
"stats": {
417+
"bytes": 196,
418+
"packets": 2
419+
}
420+
}
421+
],
422+
"matchCount": "1",
423+
"teardown": [
424+
"$TC qdisc del dev $DUMMY handle 1:0 root",
425+
"$IP addr del 10.10.10.10/24 dev $DUMMY || true"
426+
]
427+
},
428+
{
429+
"id": "5e6d",
430+
"name": "Test QFQ's enqueue reentrant behaviour with netem",
431+
"category": [
432+
"qdisc",
433+
"qfq"
434+
],
435+
"plugins": {
436+
"requires": "nsPlugin"
437+
},
438+
"setup": [
439+
"$IP link set dev $DUMMY up || true",
440+
"$IP addr add 10.10.10.10/24 dev $DUMMY || true",
441+
"$TC qdisc add dev $DUMMY handle 1:0 root qfq",
442+
"$TC class replace dev $DUMMY parent 1:0 classid 1:1 qfq weight 100 maxpkt 1500",
443+
"$TC qdisc add dev $DUMMY parent 1:1 handle 2:0 netem duplicate 100%",
444+
"$TC filter add dev $DUMMY parent 1:0 protocol ip prio 1 u32 match ip protocol 1 0xff flowid 1:1"
445+
],
446+
"cmdUnderTest": "ping -c 1 -I $DUMMY 10.10.10.1 > /dev/null || true",
447+
"expExitCode": "0",
448+
"verifyCmd": "$TC -j -s qdisc ls dev $DUMMY handle 1:0",
449+
"matchJSON": [
450+
{
451+
"kind": "qfq",
452+
"handle": "1:",
453+
"bytes": 196,
454+
"packets": 2
455+
}
456+
],
457+
"matchCount": "1",
458+
"teardown": [
459+
"$TC qdisc del dev $DUMMY handle 1:0 root",
460+
"$IP addr del 10.10.10.10/24 dev $DUMMY || true"
461+
]
462+
},
463+
{
464+
"id": "bf1d",
465+
"name": "Test HFSC's enqueue reentrant behaviour with netem",
466+
"category": [
467+
"qdisc",
468+
"hfsc"
469+
],
470+
"plugins": {
471+
"requires": "nsPlugin"
472+
},
473+
"setup": [
474+
"$IP link set dev $DUMMY up || true",
475+
"$IP addr add 10.10.10.10/24 dev $DUMMY || true",
476+
"$TC qdisc add dev $DUMMY handle 1:0 root hfsc",
477+
"$TC class add dev $DUMMY parent 1:0 classid 1:1 hfsc ls m2 10Mbit",
478+
"$TC qdisc add dev $DUMMY parent 1:1 handle 2:0 netem duplicate 100%",
479+
"$TC filter add dev $DUMMY parent 1:0 protocol ip prio 1 u32 match ip dst 10.10.10.1/32 flowid 1:1",
480+
"$TC class add dev $DUMMY parent 1:0 classid 1:2 hfsc ls m2 10Mbit",
481+
"$TC qdisc add dev $DUMMY parent 1:2 handle 3:0 netem duplicate 100%",
482+
"$TC filter add dev $DUMMY parent 1:0 protocol ip prio 2 u32 match ip dst 10.10.10.2/32 flowid 1:2",
483+
"ping -c 1 10.10.10.1 -I$DUMMY > /dev/null || true",
484+
"$TC filter del dev $DUMMY parent 1:0 protocol ip prio 1",
485+
"$TC class del dev $DUMMY classid 1:1"
486+
],
487+
"cmdUnderTest": "ping -c 1 10.10.10.2 -I$DUMMY > /dev/null || true",
488+
"expExitCode": "0",
489+
"verifyCmd": "$TC -j -s qdisc ls dev $DUMMY handle 1:0",
490+
"matchJSON": [
491+
{
492+
"kind": "hfsc",
493+
"handle": "1:",
494+
"bytes": 392,
495+
"packets": 4
496+
}
497+
],
498+
"matchCount": "1",
499+
"teardown": [
500+
"$TC qdisc del dev $DUMMY handle 1:0 root",
501+
"$IP addr del 10.10.10.10/24 dev $DUMMY || true"
502+
]
503+
},
504+
{
505+
"id": "7c3b",
506+
"name": "Test nested DRR's enqueue reentrant behaviour with netem",
507+
"category": [
508+
"qdisc",
509+
"drr"
510+
],
511+
"plugins": {
512+
"requires": "nsPlugin"
513+
},
514+
"setup": [
515+
"$IP link set dev $DUMMY up || true",
516+
"$IP addr add 10.10.10.10/24 dev $DUMMY || true",
517+
"$TC qdisc add dev $DUMMY handle 1:0 root drr",
518+
"$TC class add dev $DUMMY parent 1:0 classid 1:1 drr",
519+
"$TC filter add dev $DUMMY parent 1:0 protocol ip prio 1 u32 match ip protocol 1 0xff flowid 1:1",
520+
"$TC qdisc add dev $DUMMY handle 2:0 parent 1:1 drr",
521+
"$TC class add dev $DUMMY classid 2:1 parent 2:0 drr",
522+
"$TC filter add dev $DUMMY parent 2:0 protocol ip prio 1 u32 match ip protocol 1 0xff flowid 2:1",
523+
"$TC qdisc add dev $DUMMY parent 2:1 handle 3:0 netem duplicate 100%"
524+
],
525+
"cmdUnderTest": "ping -c 1 -I $DUMMY 10.10.10.1 > /dev/null || true",
526+
"expExitCode": "0",
527+
"verifyCmd": "$TC -j -s qdisc ls dev $DUMMY handle 1:0",
528+
"matchJSON": [
529+
{
530+
"kind": "drr",
531+
"handle": "1:",
532+
"bytes": 196,
533+
"packets": 2
534+
}
535+
],
536+
"matchCount": "1",
537+
"teardown": [
538+
"$TC qdisc del dev $DUMMY handle 1:0 root",
539+
"$IP addr del 10.10.10.10/24 dev $DUMMY || true"
540+
]
355541
}
356542
]

0 commit comments

Comments
 (0)