Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Vladimir Oltean <vladimir.oltean@nxp.com>
To:     netdev@vger.kernel.org
Cc:     "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Jamal Hadi Salim <jhs@mojatatu.com>,
        Cong Wang <xiyou.wangcong@gmail.com>,
        Jiri Pirko <jiri@resnulli.us>,
        Vinicius Costa Gomes <vinicius.gomes@intel.com>,
        linux-kernel@vger.kernel.org, intel-wired-lan@lists.osuosl.org,
        Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>,
        Peilin Ye <yepeilin.cs@gmail.com>,
        Pedro Tammela <pctammela@mojatatu.com>
Subject: [PATCH RESEND net-next 2/5] net/sched: taprio: keep child Qdisc refcount elevated at 2 in offload mode
Date:   Fri,  2 Jun 2023 13:37:47 +0300
Message-Id: <20230602103750.2290132-3-vladimir.oltean@nxp.com>
In-Reply-To: <20230602103750.2290132-1-vladimir.oltean@nxp.com>
References: <20230602103750.2290132-1-vladimir.oltean@nxp.com>
Content-Transfer-Encoding: 8bit
Content-Type: text/plain
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?E2o07uJ3jjTcrO7aaXiAN2HKJMB81o4JFHM4fVjC9KYSdi8rCYue6I2Y8dcV?=
 =?us-ascii?Q?oKIF1b/AtAVJUSFx0EhUDaTY8eFXWXiWy7y7ThykVJOCQIVzGcVlzkfy5Stu?=
 =?us-ascii?Q?XxrxMp/tZbhpiw9zWSCuk39dnjHtEq6uNvNel9hLpHvM2cuR+KTfhKsuZEkR?=
 =?us-ascii?Q?ME/NNo24QI+wQXDFSoP81JnMxN2QqyrpkNkYrU+Wv2gFPJVHvV1n1y5KxPNq?=
 =?us-ascii?Q?aLpY5+9LsnzgrIhbjtTXr+pY0Vwl8gEwH4L18VsgwYyc+tlBj2M8GBpnE8gV?=
 =?us-ascii?Q?OQdxHiFQg9cRwwU+JMOnQZ+JnOq5wyL+AbD5/ZQ3inA5Dy+e72I768bvmajF?=
 =?us-ascii?Q?TcPef3wvF8gmxfiCi1qht9Amd6wsXLKXWUXfDYQOwKjKX5QQsCpZwLSgLopQ?=
 =?us-ascii?Q?uhAKnLCw6v1yD2KWEWvlEGBWLhBzZZjH+xZIA3HK/f2H8BtvSnACv1Fxtyjb?=
 =?us-ascii?Q?8EKhPgoxKmiCqtgQsjykJFHHS41VooNsXG5W1S9ke3r11BTqiVrc01XSfZ1n?=
 =?us-ascii?Q?uKDkI6lF/0qYMjmN5TcCU/E1eiiHOAB92gP1DoR0/eWTb0DCWi71kRop38I4?=
 =?us-ascii?Q?o7woSuouQDbjKXkgtas6a8Bha+809YW/Kw7POhherNU7ZlbFwJ4fGEQ0TT2J?=
 =?us-ascii?Q?u5PVALWtyH+yIISz09vy7KS9QVgXN1Z3oVqpvFJVzPtt/WmkkA8mLVXNvcPE?=
 =?us-ascii?Q?JLUAplrNVoLTSIcFiKYJezmRwrS4LgVRX6qTwc7UYpkwWoLSYIUvcLOeuJez?=
 =?us-ascii?Q?6Lfs8FUIJJbuICosoZMcfLINaILxKFjxg0fIKhiS6TjVhwJa5Qc0phe61O0g?=
 =?us-ascii?Q?SToWM4o8hUU6GtUcOoY40QQyxSpr0Ig0RU06LzwvmYWoFuOfnxd6MXZkiBZI?=
 =?us-ascii?Q?Q0N8Gw+GnCR7XGpTAFcNI9AGjmBGkEw0mEy8PFOrXqkQbznUR1dMLajDA1a9?=
 =?us-ascii?Q?VT6L90A+AXKLKYmEyZDJNqEK/awWBA6+75LdsPjrJ3mF5U92qTKPYf2ktSKh?=
 =?us-ascii?Q?mmdZPwpngKFmDXd8qI+H2V1xTTs1CqhC/ytihJVgo2nJKOGbiauLAXDBZ++r?=
 =?us-ascii?Q?3L5bRFaUnc0UAhTs48NS0JoXXd29n05xKy3kVuuR2uwOjjsTDinL3xhgCXIk?=
 =?us-ascii?Q?ZBDjbpyZ/e3GDvOyBsCDxQwMhn+u6C7nEL7tZr507uYyyPnU4+s9NqNv3YI+?=
 =?us-ascii?Q?74RjLh3AZ4F0iUVoQdiw6uNizroqhQ10FpLGGPWBjGBUjC+lOw1bY8Pnxt/2?=
 =?us-ascii?Q?yLFuRaZJxmjTOLUdkA0YowtHh3gGYlzmtBI3CoFgCwfEhlSioHWu05l/I8dO?=
 =?us-ascii?Q?A8zT8MO6Z9dHyaLWRdByjC5MUbMnJ4ree3GDaVa4Gny91T/KtWmoVNIA2CDI?=
 =?us-ascii?Q?vuxJ83Y9vNXRK9leiesmvcGNVYtwg4rPqLO54XkGro0zJiRO7/FtY92A96Dt?=
 =?us-ascii?Q?0PrL5mkK21+SHvA0ntbYbiYtSeHJZ8C3YmTQB7R46OCv48ZuWKr7QW87kVtF?=
 =?us-ascii?Q?SrUPsgLEuNYbgtqAyiXG4/JZhgcN+m1cdoDbQ37dqmawBWcBlLCgAOgXq8K5?=
 =?us-ascii?Q?2eirbWQLzqP+3cavtZiZowtr+UXpEMqxVOBdcl/qQSp+jdxBx0moB5hoOstQ?=
 =?us-ascii?Q?hg=3D=3D?=
X-OriginatorOrg: nxp.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 87f51ae4-859a-40ef-2f3e-08db63557b07
X-MS-Exchange-CrossTenant-AuthSource: AM0PR04MB6452.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Jun 2023 10:38:19.2286
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: j9mRJ/NR9Nv6PmNBzCCKyjIBucSDlEFK3ktygSgSf4A4ndf1Dq8IhjhuXfcI2o4nFf4K6j1HlAR+O8hT8Bwxkw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PAXPR04MB8926
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,T_SCC_BODY_TEXT_LINE,
        URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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(-)

diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index b1c611c72aa4..8807fc915b79 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -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);
-- 
2.34.1