Skip to content

Commit 6910fe9

Browse files
davem330davem330
committed
Merge branch 'taprio-Some-fixes'
Vinicius Costa Gomes says: ==================== taprio: Some fixes Changes from v3: - Replaced ENOTSUPP error code with EOPNOTSUPP (Jakub Kicinski); - Added the missing policy validation for the flags netlink argument (Jakub Kicinski); - Fixed the destroy() flow to also destroy the priority to traffic class mapping (David Miller); - Fixed dropping packets when taprio offloading is used together with ETF offloading (more on this below); Changes from v2: - Squashed commits 2/3 and 3/3 into a single one (I think a single commit is going to be easier to review); - Removed an "improvement" that was causing changes in user visible behavior; Changes from v1: - Fixed ignoring the 'flags' argument when adding a new instance (Vladimir Oltean); - Changed the order of commits; Updated cover letter: One bit that might need some attention is the fix for not dropping all packets when taprio and ETF offloading are used, patch 5/5. The behavior when the fix is applied is that packets that have a 'txtime' that would fall outside of their transmission window are now dropped by taprio. The question that might be raised is: should taprio be responsible for dropping these packets, or should it be handled lower in the stack? My opinion is: taprio has all the information, and it's able to give feeback to the user. Lower in the stack, those packets might go into the void, and the only feedback could be a hard to find counter increasing. Patch 1/5: Reported by Po Liu, is more of a improvement of usability for drivers implementing offloading features, now they can rely on the value of dev->num_tc, instead of going through some hops to get this value. Patch 2/5: Use 'q->flags' as the source of truth for the offloading flags. Tries to solidify the current behavior, while avoiding going into invalid states, one of which was causing a "rcu stall" (more information in the commit message). Patch 3/5: Adds the missing netlink attribute validation for TCA_TAPRIO_ATTR_FLAGS. Patch 4/5: Replaces the usage of netdev_set_num_tc() with netdev_reset_tc() in taprio_destroy(), taprio_destroy() is called when applying a configuration fails, making sure that the device traffic class configuration goes back to the default state. @vladimir: If possible, I would appreciate your Ack on patch 2/5. I have been looking at this code for so long that I might have missed something obvious (and my growing dislike for the word 'flags' may be affecting my judgement :-). ==================== Signed-off-by: David S. Miller <[email protected]>
2 parents de34d70 + bfabd41 commit 6910fe9

File tree

1 file changed

+57
-35
lines changed

1 file changed

+57
-35
lines changed

‎net/sched/sch_taprio.c

Lines changed: 57 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ static DEFINE_SPINLOCK(taprio_list_lock);
3131

3232
#define TXTIME_ASSIST_IS_ENABLED(flags) ((flags) & TCA_TAPRIO_ATTR_FLAG_TXTIME_ASSIST)
3333
#define FULL_OFFLOAD_IS_ENABLED(flags) ((flags) & TCA_TAPRIO_ATTR_FLAG_FULL_OFFLOAD)
34+
#define TAPRIO_FLAGS_INVALID U32_MAX
3435

3536
struct sched_entry {
3637
struct list_head list;
@@ -766,6 +767,7 @@ static const struct nla_policy taprio_policy[TCA_TAPRIO_ATTR_MAX + 1] = {
766767
[TCA_TAPRIO_ATTR_SCHED_CLOCKID] = { .type = NLA_S32 },
767768
[TCA_TAPRIO_ATTR_SCHED_CYCLE_TIME] = { .type = NLA_S64 },
768769
[TCA_TAPRIO_ATTR_SCHED_CYCLE_TIME_EXTENSION] = { .type = NLA_S64 },
770+
[TCA_TAPRIO_ATTR_FLAGS] = { .type = NLA_U32 },
769771
};
770772

771773
static int fill_sched_entry(struct nlattr **tb, struct sched_entry *entry,
@@ -1367,6 +1369,33 @@ static int taprio_mqprio_cmp(const struct net_device *dev,
13671369
return 0;
13681370
}
13691371

1372+
/* The semantics of the 'flags' argument in relation to 'change()'
1373+
* requests, are interpreted following two rules (which are applied in
1374+
* this order): (1) an omitted 'flags' argument is interpreted as
1375+
* zero; (2) the 'flags' of a "running" taprio instance cannot be
1376+
* changed.
1377+
*/
1378+
static int taprio_new_flags(const struct nlattr *attr, u32 old,
1379+
struct netlink_ext_ack *extack)
1380+
{
1381+
u32 new = 0;
1382+
1383+
if (attr)
1384+
new = nla_get_u32(attr);
1385+
1386+
if (old != TAPRIO_FLAGS_INVALID && old != new) {
1387+
NL_SET_ERR_MSG_MOD(extack, "Changing 'flags' of a running schedule is not supported");
1388+
return -EOPNOTSUPP;
1389+
}
1390+
1391+
if (!taprio_flags_valid(new)) {
1392+
NL_SET_ERR_MSG_MOD(extack, "Specified 'flags' are not valid");
1393+
return -EINVAL;
1394+
}
1395+
1396+
return new;
1397+
}
1398+
13701399
static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
13711400
struct netlink_ext_ack *extack)
13721401
{
@@ -1375,7 +1404,6 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
13751404
struct taprio_sched *q = qdisc_priv(sch);
13761405
struct net_device *dev = qdisc_dev(sch);
13771406
struct tc_mqprio_qopt *mqprio = NULL;
1378-
u32 taprio_flags = 0;
13791407
unsigned long flags;
13801408
ktime_t start;
13811409
int i, err;
@@ -1388,21 +1416,14 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
13881416
if (tb[TCA_TAPRIO_ATTR_PRIOMAP])
13891417
mqprio = nla_data(tb[TCA_TAPRIO_ATTR_PRIOMAP]);
13901418

1391-
if (tb[TCA_TAPRIO_ATTR_FLAGS]) {
1392-
taprio_flags = nla_get_u32(tb[TCA_TAPRIO_ATTR_FLAGS]);
1393-
1394-
if (q->flags != 0 && q->flags != taprio_flags) {
1395-
NL_SET_ERR_MSG_MOD(extack, "Changing 'flags' of a running schedule is not supported");
1396-
return -EOPNOTSUPP;
1397-
} else if (!taprio_flags_valid(taprio_flags)) {
1398-
NL_SET_ERR_MSG_MOD(extack, "Specified 'flags' are not valid");
1399-
return -EINVAL;
1400-
}
1419+
err = taprio_new_flags(tb[TCA_TAPRIO_ATTR_FLAGS],
1420+
q->flags, extack);
1421+
if (err < 0)
1422+
return err;
14011423

1402-
q->flags = taprio_flags;
1403-
}
1424+
q->flags = err;
14041425

1405-
err = taprio_parse_mqprio_opt(dev, mqprio, extack, taprio_flags);
1426+
err = taprio_parse_mqprio_opt(dev, mqprio, extack, q->flags);
14061427
if (err < 0)
14071428
return err;
14081429

@@ -1444,7 +1465,20 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
14441465

14451466
taprio_set_picos_per_byte(dev, q);
14461467

1447-
if (FULL_OFFLOAD_IS_ENABLED(taprio_flags))
1468+
if (mqprio) {
1469+
netdev_set_num_tc(dev, mqprio->num_tc);
1470+
for (i = 0; i < mqprio->num_tc; i++)
1471+
netdev_set_tc_queue(dev, i,
1472+
mqprio->count[i],
1473+
mqprio->offset[i]);
1474+
1475+
/* Always use supplied priority mappings */
1476+
for (i = 0; i <= TC_BITMASK; i++)
1477+
netdev_set_prio_tc_map(dev, i,
1478+
mqprio->prio_tc_map[i]);
1479+
}
1480+
1481+
if (FULL_OFFLOAD_IS_ENABLED(q->flags))
14481482
err = taprio_enable_offload(dev, mqprio, q, new_admin, extack);
14491483
else
14501484
err = taprio_disable_offload(dev, q, extack);
@@ -1464,27 +1498,14 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
14641498
q->txtime_delay = nla_get_u32(tb[TCA_TAPRIO_ATTR_TXTIME_DELAY]);
14651499
}
14661500

1467-
if (!TXTIME_ASSIST_IS_ENABLED(taprio_flags) &&
1468-
!FULL_OFFLOAD_IS_ENABLED(taprio_flags) &&
1501+
if (!TXTIME_ASSIST_IS_ENABLED(q->flags) &&
1502+
!FULL_OFFLOAD_IS_ENABLED(q->flags) &&
14691503
!hrtimer_active(&q->advance_timer)) {
14701504
hrtimer_init(&q->advance_timer, q->clockid, HRTIMER_MODE_ABS);
14711505
q->advance_timer.function = advance_sched;
14721506
}
14731507

1474-
if (mqprio) {
1475-
netdev_set_num_tc(dev, mqprio->num_tc);
1476-
for (i = 0; i < mqprio->num_tc; i++)
1477-
netdev_set_tc_queue(dev, i,
1478-
mqprio->count[i],
1479-
mqprio->offset[i]);
1480-
1481-
/* Always use supplied priority mappings */
1482-
for (i = 0; i <= TC_BITMASK; i++)
1483-
netdev_set_prio_tc_map(dev, i,
1484-
mqprio->prio_tc_map[i]);
1485-
}
1486-
1487-
if (FULL_OFFLOAD_IS_ENABLED(taprio_flags)) {
1508+
if (FULL_OFFLOAD_IS_ENABLED(q->flags)) {
14881509
q->dequeue = taprio_dequeue_offload;
14891510
q->peek = taprio_peek_offload;
14901511
} else {
@@ -1501,9 +1522,9 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
15011522
goto unlock;
15021523
}
15031524

1504-
if (TXTIME_ASSIST_IS_ENABLED(taprio_flags)) {
1505-
setup_txtime(q, new_admin, start);
1525+
setup_txtime(q, new_admin, start);
15061526

1527+
if (TXTIME_ASSIST_IS_ENABLED(q->flags)) {
15071528
if (!oper) {
15081529
rcu_assign_pointer(q->oper_sched, new_admin);
15091530
err = 0;
@@ -1528,7 +1549,7 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
15281549

15291550
spin_unlock_irqrestore(&q->current_entry_lock, flags);
15301551

1531-
if (FULL_OFFLOAD_IS_ENABLED(taprio_flags))
1552+
if (FULL_OFFLOAD_IS_ENABLED(q->flags))
15321553
taprio_offload_config_changed(q);
15331554
}
15341555

@@ -1567,7 +1588,7 @@ static void taprio_destroy(struct Qdisc *sch)
15671588
}
15681589
q->qdiscs = NULL;
15691590

1570-
netdev_set_num_tc(dev, 0);
1591+
netdev_reset_tc(dev);
15711592

15721593
if (q->oper_sched)
15731594
call_rcu(&q->oper_sched->rcu, taprio_free_sched_cb);
@@ -1597,6 +1618,7 @@ static int taprio_init(struct Qdisc *sch, struct nlattr *opt,
15971618
* and get the valid one on taprio_change().
15981619
*/
15991620
q->clockid = -1;
1621+
q->flags = TAPRIO_FLAGS_INVALID;
16001622

16011623
spin_lock(&taprio_list_lock);
16021624
list_add(&q->taprio_list, &taprio_list);

0 commit comments

Comments
 (0)