This series contains:
- the proper protected variant of rcu_dereference() of admin and oper
schedules for accesses from the slow path
- a removal of an extra function pointer indirection for
qdisc->dequeue() and qdisc->peek()
- a removal of WARN_ON_ONCE() checks that can never trigger
- the addition of netlink extack messages to some qdisc->init() failures
These were split from an earlier patch set, hence the v2.
Vladimir Oltean (7):
net/sched: taprio: taprio_offload_config_changed() is protected by
rtnl_mutex
net/sched: taprio: taprio_dump and taprio_change are protected by
rtnl_mutex
net/sched: taprio: use rtnl_dereference for oper and admin sched in
taprio_destroy()
net/sched: taprio: remove redundant FULL_OFFLOAD_IS_ENABLED check in
taprio_enqueue
net/sched: taprio: stop going through private ops for dequeue and peek
net/sched: taprio: add extack messages in taprio_init
net/sched: taprio: replace safety precautions with comments
net/sched/sch_taprio.c | 112 +++++++++++++----------------------------
1 file changed, 34 insertions(+), 78 deletions(-)
--
2.34.1
Since commit 13511704f8d7 ("net: taprio offload: enforce qdisc to netdev
queue mapping"), taprio_dequeue_soft() and taprio_peek_soft() are de
facto the only implementations for Qdisc_ops :: dequeue and Qdisc_ops ::
peek that taprio provides.
This is because in full offload mode, __dev_queue_xmit() will select a
txq->qdisc which is never root taprio qdisc. So if nothing is enqueued
in the root qdisc, it will never be run and nothing will get dequeued
from it.
Therefore, we can remove the private indirection from taprio, and always
point Qdisc_ops :: dequeue to taprio_dequeue_soft (now simply named
taprio_dequeue) and Qdisc_ops :: peek to taprio_peek_soft (now simply
named taprio_peek).
Signed-off-by: Vladimir Oltean <[email protected]>
---
v1->v2: none
net/sched/sch_taprio.c | 58 +++++++++---------------------------------
1 file changed, 12 insertions(+), 46 deletions(-)
diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index 0781fc4a2789..f3eadea101e1 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -79,8 +79,6 @@ struct taprio_sched {
struct sched_gate_list __rcu *admin_sched;
struct hrtimer advance_timer;
struct list_head taprio_list;
- struct sk_buff *(*dequeue)(struct Qdisc *sch);
- struct sk_buff *(*peek)(struct Qdisc *sch);
u32 txtime_delay;
};
@@ -492,7 +490,7 @@ static int taprio_enqueue(struct sk_buff *skb, struct Qdisc *sch,
return taprio_enqueue_one(skb, sch, child, to_free);
}
-static struct sk_buff *taprio_peek_soft(struct Qdisc *sch)
+static struct sk_buff *taprio_peek(struct Qdisc *sch)
{
struct taprio_sched *q = qdisc_priv(sch);
struct net_device *dev = qdisc_dev(sch);
@@ -501,6 +499,11 @@ static struct sk_buff *taprio_peek_soft(struct Qdisc *sch)
u32 gate_mask;
int i;
+ if (unlikely(FULL_OFFLOAD_IS_ENABLED(q->flags))) {
+ WARN_ONCE(1, "Trying to peek into the root of a taprio qdisc configured with full offload\n");
+ return NULL;
+ }
+
rcu_read_lock();
entry = rcu_dereference(q->current_entry);
gate_mask = entry ? entry->gate_mask : TAPRIO_ALL_GATES_OPEN;
@@ -536,20 +539,6 @@ static struct sk_buff *taprio_peek_soft(struct Qdisc *sch)
return NULL;
}
-static struct sk_buff *taprio_peek_offload(struct Qdisc *sch)
-{
- WARN_ONCE(1, "Trying to peek into the root of a taprio qdisc configured with full offload\n");
-
- return NULL;
-}
-
-static struct sk_buff *taprio_peek(struct Qdisc *sch)
-{
- struct taprio_sched *q = qdisc_priv(sch);
-
- return q->peek(sch);
-}
-
static void taprio_set_budget(struct taprio_sched *q, struct sched_entry *entry)
{
atomic_set(&entry->budget,
@@ -557,7 +546,7 @@ static void taprio_set_budget(struct taprio_sched *q, struct sched_entry *entry)
atomic64_read(&q->picos_per_byte)));
}
-static struct sk_buff *taprio_dequeue_soft(struct Qdisc *sch)
+static struct sk_buff *taprio_dequeue(struct Qdisc *sch)
{
struct taprio_sched *q = qdisc_priv(sch);
struct net_device *dev = qdisc_dev(sch);
@@ -566,6 +555,11 @@ static struct sk_buff *taprio_dequeue_soft(struct Qdisc *sch)
u32 gate_mask;
int i;
+ if (unlikely(FULL_OFFLOAD_IS_ENABLED(q->flags))) {
+ WARN_ONCE(1, "Trying to dequeue from the root of a taprio qdisc configured with full offload\n");
+ return NULL;
+ }
+
rcu_read_lock();
entry = rcu_dereference(q->current_entry);
/* if there's no entry, it means that the schedule didn't
@@ -645,20 +639,6 @@ static struct sk_buff *taprio_dequeue_soft(struct Qdisc *sch)
return skb;
}
-static struct sk_buff *taprio_dequeue_offload(struct Qdisc *sch)
-{
- WARN_ONCE(1, "Trying to dequeue from the root of a taprio qdisc configured with full offload\n");
-
- return NULL;
-}
-
-static struct sk_buff *taprio_dequeue(struct Qdisc *sch)
-{
- struct taprio_sched *q = qdisc_priv(sch);
-
- return q->dequeue(sch);
-}
-
static bool should_restart_cycle(const struct sched_gate_list *oper,
const struct sched_entry *entry)
{
@@ -1557,17 +1537,6 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
q->advance_timer.function = advance_sched;
}
- if (FULL_OFFLOAD_IS_ENABLED(q->flags)) {
- q->dequeue = taprio_dequeue_offload;
- q->peek = taprio_peek_offload;
- } else {
- /* Be sure to always keep the function pointers
- * in a consistent state.
- */
- q->dequeue = taprio_dequeue_soft;
- q->peek = taprio_peek_soft;
- }
-
err = taprio_get_start_time(sch, new_admin, &start);
if (err < 0) {
NL_SET_ERR_MSG(extack, "Internal error: failed get start time");
@@ -1682,9 +1651,6 @@ static int taprio_init(struct Qdisc *sch, struct nlattr *opt,
hrtimer_init(&q->advance_timer, CLOCK_TAI, HRTIMER_MODE_ABS);
q->advance_timer.function = advance_sched;
- q->dequeue = taprio_dequeue_soft;
- q->peek = taprio_peek_soft;
-
q->root = sch;
/* We only support static clockids. Use an invalid value as default
--
2.34.1
Sparse complains that taprio_destroy() dereferences q->oper_sched and
q->admin_sched without rcu_dereference(), since they are marked as __rcu
in the taprio private structure.
1671:28: warning: incorrect type in argument 1 (different address spaces)
1671:28: expected struct callback_head *head
1671:28: got struct callback_head [noderef] __rcu *
1674:28: warning: incorrect type in argument 1 (different address spaces)
1674:28: expected struct callback_head *head
1674:28: got struct callback_head [noderef] __rcu *
To silence that build warning, do actually use rtnl_dereference(), since
we know the rtnl_mutex is held at the time of q->destroy().
Signed-off-by: Vladimir Oltean <[email protected]>
---
v1->v2: resend to net-next, use rtnl_dereference() instead of opening an
rcu_read_lock()
net/sched/sch_taprio.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index 63cbf856894a..6113c6646559 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -1636,6 +1636,7 @@ static void taprio_destroy(struct Qdisc *sch)
{
struct taprio_sched *q = qdisc_priv(sch);
struct net_device *dev = qdisc_dev(sch);
+ struct sched_gate_list *oper, *admin;
unsigned int i;
spin_lock(&taprio_list_lock);
@@ -1659,11 +1660,14 @@ static void taprio_destroy(struct Qdisc *sch)
netdev_reset_tc(dev);
- if (q->oper_sched)
- call_rcu(&q->oper_sched->rcu, taprio_free_sched_cb);
+ oper = rtnl_dereference(q->oper_sched);
+ admin = rtnl_dereference(q->admin_sched);
+
+ if (oper)
+ call_rcu(&oper->rcu, taprio_free_sched_cb);
- if (q->admin_sched)
- call_rcu(&q->admin_sched->rcu, taprio_free_sched_cb);
+ if (admin)
+ call_rcu(&admin->rcu, taprio_free_sched_cb);
}
static int taprio_init(struct Qdisc *sch, struct nlattr *opt,
--
2.34.1
Since commit 13511704f8d7 ("net: taprio offload: enforce qdisc to netdev
queue mapping"), __dev_queue_xmit() will select a txq->qdisc for the
full offload case of taprio which isn't the root taprio qdisc, so
qdisc enqueues will never pass through taprio_enqueue().
That commit already introduced one safety precaution check for
FULL_OFFLOAD_IS_ENABLED(); a second one is really not needed, so
simplify the conditional for entering into the GSO segmentation logic.
Also reword the comment a little, to appear more natural after the code
change.
Signed-off-by: Vladimir Oltean <[email protected]>
---
v1->v2: none
net/sched/sch_taprio.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index 6113c6646559..0781fc4a2789 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -455,10 +455,10 @@ static int taprio_enqueue(struct sk_buff *skb, struct Qdisc *sch,
/* Large packets might not be transmitted when the transmission duration
* exceeds any configured interval. Therefore, segment the skb into
- * smaller chunks. Skip it for the full offload case, as the driver
- * and/or the hardware is expected to handle this.
+ * smaller chunks. Drivers with full offload are expected to handle
+ * this in hardware.
*/
- if (skb_is_gso(skb) && !FULL_OFFLOAD_IS_ENABLED(q->flags)) {
+ if (skb_is_gso(skb)) {
unsigned int slen = 0, numsegs = 0, len = qdisc_pkt_len(skb);
netdev_features_t features = netif_skb_features(skb);
struct sk_buff *segs, *nskb;
--
2.34.1
Hello:
This series was applied to netdev/net-next.git (master)
by Jakub Kicinski <[email protected]>:
On Thu, 15 Sep 2022 13:50:39 +0300 you wrote:
> This series contains:
> - the proper protected variant of rcu_dereference() of admin and oper
> schedules for accesses from the slow path
> - a removal of an extra function pointer indirection for
> qdisc->dequeue() and qdisc->peek()
> - a removal of WARN_ON_ONCE() checks that can never trigger
> - the addition of netlink extack messages to some qdisc->init() failures
>
> [...]
Here is the summary with links:
- [v2,net-next,1/7] net/sched: taprio: taprio_offload_config_changed() is protected by rtnl_mutex
https://git.kernel.org/netdev/net-next/c/c8cbe123be6d
- [v2,net-next,2/7] net/sched: taprio: taprio_dump and taprio_change are protected by rtnl_mutex
https://git.kernel.org/netdev/net-next/c/18cdd2f0998a
- [v2,net-next,3/7] net/sched: taprio: use rtnl_dereference for oper and admin sched in taprio_destroy()
https://git.kernel.org/netdev/net-next/c/9af23657b336
- [v2,net-next,4/7] net/sched: taprio: remove redundant FULL_OFFLOAD_IS_ENABLED check in taprio_enqueue
https://git.kernel.org/netdev/net-next/c/fa65edde5e49
- [v2,net-next,5/7] net/sched: taprio: stop going through private ops for dequeue and peek
https://git.kernel.org/netdev/net-next/c/25becba6290b
- [v2,net-next,6/7] net/sched: taprio: add extack messages in taprio_init
https://git.kernel.org/netdev/net-next/c/026de64d7bc3
- [v2,net-next,7/7] net/sched: taprio: replace safety precautions with comments
https://git.kernel.org/netdev/net-next/c/2c08a4f898d0
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html