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;
Message-ID: <6bce1c55e1cd4295a3f36cb4b37398d951ead07b.camel@redhat.com>
Subject: Re: [PATCH RESEND net-next 2/5] net/sched: taprio: keep child Qdisc
 refcount elevated at 2 in offload mode
From:   Paolo Abeni <pabeni@redhat.com>
To:     Vladimir Oltean <vladimir.oltean@nxp.com>, netdev@vger.kernel.org
Cc:     "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        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>
Date:   Tue, 06 Jun 2023 12:27:09 +0200
In-Reply-To: <20230602103750.2290132-3-vladimir.oltean@nxp.com>
References: <20230602103750.2290132-1-vladimir.oltean@nxp.com>
         <20230602103750.2290132-3-vladimir.oltean@nxp.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.46.4 (3.46.4-1.fc37) 
MIME-Version: 1.0
Precedence: bulk

On Fri, 2023-06-02 at 13:37 +0300, Vladimir Oltean wrote:
> The taprio qdisc currently lives in the following equilibrium.
>=20
> 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().
>=20
> 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=3D=3DNULL) -> taprio_attach(). At that time, taprio fr=
ees
> 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=3D=3DNULL, new=3D=3DNULL) -> qdisc_put(old=
) when
> the Qdisc is deleted or when the child Qdisc is replaced with something
> else.
>=20
> 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.
>=20
> 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.
>=20
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  net/sched/sch_taprio.c | 28 +++++++++++++++++-----------
>  1 file changed, 17 insertions(+), 11 deletions(-)
>=20
> 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)
> =20
>  			qdisc->flags |=3D TCQ_F_ONETXQUEUE | TCQ_F_NOPARENT;
>  			old =3D 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 =3D 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 =3D NULL;
> -	}
>  }
> =20
>  static struct netdev_queue *taprio_queue_get(struct Qdisc *sch,
> @@ -2183,15 +2180,24 @@ static int taprio_graft(struct Qdisc *sch, unsign=
ed long cl,
>  	if (dev->flags & IFF_UP)
>  		dev_deactivate(dev);
> =20
> -	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 =3D dev_graft_qdisc(dev_queue, new);
> -	} else {
> +	else
>  		*old =3D q->qdiscs[cl - 1];
> -		q->qdiscs[cl - 1] =3D new;
> -	}
> =20
> -	if (new)
> +	q->qdiscs[cl - 1] =3D new;
> +	if (new) {
> +		qdisc_refcount_inc(new);
>  		new->flags |=3D TCQ_F_ONETXQUEUE | TCQ_F_NOPARENT;
> +	}
> =20
Isn't the above leaking a reference to old with something alike:

tc qdisc replace dev eth0 handle 8001: parent root stab overhead 24 taprio =
flags 0x2 #...
	# each q in q->qdiscs has refcnt =3D=3D 2
tc qdisc replace dev eth0 parent 8001:8 cbs #...
	# -> taprio_graft(..., cbs, ...)
	# cbs refcnt is 2
	# 'old' refcnt decreases by 1, refcount will not reach 0?!?

kmemleak should be able to see that.

BTW, what about including your tests from the cover letter somewhere under =
tc-testing?

Thanks!

Paolo