@@ -2138,23 +2138,20 @@ static void taprio_attach(struct Qdisc *sch)
qdisc->flags |= TCQ_F_ONETXQUEUE | TCQ_F_NOPARENT;
old = dev_graft_qdisc(dev_queue, qdisc);
+ /* Keep refcount of q->qdiscs[ntx] at 2 */
+ qdisc_refcount_inc(qdisc);
} else {
/* In software mode, attach the root taprio qdisc
* to all netdev TX queues, so that dev_qdisc_enqueue()
* goes through taprio_enqueue().
*/
old = dev_graft_qdisc(dev_queue, sch);
+ /* Keep root refcount at 1 + num_tx_queues */
qdisc_refcount_inc(sch);
}
if (old)
qdisc_put(old);
}
-
- /* access to the child qdiscs is not needed in offload mode */
- if (FULL_OFFLOAD_IS_ENABLED(q->flags)) {
- kfree(q->qdiscs);
- q->qdiscs = NULL;
- }
}
static struct netdev_queue *taprio_queue_get(struct Qdisc *sch,
@@ -2183,15 +2180,24 @@ static int taprio_graft(struct Qdisc *sch, unsigned long cl,
if (dev->flags & IFF_UP)
dev_deactivate(dev);
- if (FULL_OFFLOAD_IS_ENABLED(q->flags)) {
+ /* In software mode, the root taprio qdisc is still the one attached to
+ * all netdev TX queues, and hence responsible for taprio_enqueue() to
+ * forward the skbs to the child qdiscs from the private q->qdiscs[]
+ * array. So only attach the new qdisc to the netdev queue in offload
+ * mode, where the enqueue must bypass taprio. However, save the
+ * reference to the new qdisc in the private array in both cases, to
+ * have an up-to-date reference to our children.
+ */
+ if (FULL_OFFLOAD_IS_ENABLED(q->flags))
*old = dev_graft_qdisc(dev_queue, new);
- } else {
+ else
*old = q->qdiscs[cl - 1];
- q->qdiscs[cl - 1] = new;
- }
- if (new)
+ q->qdiscs[cl - 1] = new;
+ if (new) {
+ qdisc_refcount_inc(new);
new->flags |= TCQ_F_ONETXQUEUE | TCQ_F_NOPARENT;
+ }
if (dev->flags & IFF_UP)
dev_activate(dev);
The taprio qdisc currently lives in the following equilibrium. In the software scheduling case, taprio attaches itself to all TXQs, thus having a refcount of 1 + the number of TX queues. In this mode, q->qdiscs[] is not visible directly to the Qdisc API. The lifetime of the Qdiscs from this private array lasts until qdisc_destroy() -> taprio_destroy(). In the fully offloaded case, the root taprio has a refcount of 1, and all child q->qdiscs[] also have a refcount of 1. The child q->qdiscs[] are visible to the Qdisc API (they are attached to the netdev TXQs directly), however taprio loses a reference to them very early - during qdisc_graft(parent==NULL) -> taprio_attach(). At that time, taprio frees the q->qdiscs[] array to not leak memory, but interestingly, it does not release a reference on these qdiscs because it doesn't effectively own them - they are created by taprio but owned by the Qdisc core, and will be freed by qdisc_graft(parent==NULL, new==NULL) -> qdisc_put(old) when the Qdisc is deleted or when the child Qdisc is replaced with something else. My interest is to change this equilibrium such that taprio also owns a reference on the q->qdiscs[] child Qdiscs for the lifetime of the root Qdisc, including in full offload mode. I want this because I would like taprio_leaf(), taprio_dump_class(), taprio_dump_class_stats() to have insight into q->qdiscs[] for the software scheduling mode - currently they look at dev_queue->qdisc_sleeping, which is, as mentioned, the same as the root taprio. The following set of changes is necessary: - don't free q->qdiscs[] early in taprio_attach(), free it late in taprio_destroy() for consistency with software mode. But: - currently that's not possible, because taprio doesn't own a reference on q->qdiscs[]. So hold that reference - once during the initial attach() and once during subsequent graft() calls when the child is changed. - always keep track of the current child in q->qdiscs[], even for full offload mode, so that we free in taprio_destroy() what we should, and not something stale. Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- net/sched/sch_taprio.c | 28 +++++++++++++++++----------- 1 file changed, 17 insertions(+), 11 deletions(-)