Message ID | 20230207135440.1482856-11-vladimir.oltean@nxp.com |
---|---|
State | Handled Elsewhere |
Headers | show |
Series | taprio automatic queueMaxSDU and new TXQ selection procedure | expand |
On Tue, Feb 7, 2023 at 2:55 PM Vladimir Oltean <vladimir.oltean@nxp.com> wrote: > > Some qdiscs like taprio turn out to be actually pretty reliant on a well > configured stab, to not underestimate the skb transmission time (by > properly accounting for L1 overhead). > > In a future change, taprio will need the stab, if configured by the > user, to be available at ops->init() time. It will become even more > important in upcoming work, when the overhead will be used for the > queueMaxSDU calculation that is passed to an offloading driver. > > However, rcu_assign_pointer(sch->stab, stab) is called right after > ops->init(), making it unavailable, and I don't really see a good reason > for that. > > Move it earlier, which nicely seems to simplify the error handling path > as well. Well... if you say so :) > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > Reviewed-by: Kurt Kanzenbach <kurt@linutronix.de> > If TCA_STAB attribute is malformed, we end up calling ->destroy() on a not yet initialized qdisc :/ I am going to send the following fix, unless someone disagrees. (Moving qdisc_put_stab() _after_ ops->destroy(sch) is not strictly needed for a fix, but undo should be done in reverse steps for clarity. diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c index e9780631b5b58202068e20c42ccf1197eac2194c..aba789c30a2eb50d339b8a888495b794825e1775 100644 --- a/net/sched/sch_api.c +++ b/net/sched/sch_api.c @@ -1286,7 +1286,7 @@ static struct Qdisc *qdisc_create(struct net_device *dev, stab = qdisc_get_stab(tca[TCA_STAB], extack); if (IS_ERR(stab)) { err = PTR_ERR(stab); - goto err_out4; + goto err_out3; } rcu_assign_pointer(sch->stab, stab); } @@ -1294,14 +1294,14 @@ static struct Qdisc *qdisc_create(struct net_device *dev, if (ops->init) { err = ops->init(sch, tca[TCA_OPTIONS], extack); if (err != 0) - goto err_out5; + goto err_out4; } if (tca[TCA_RATE]) { err = -EOPNOTSUPP; if (sch->flags & TCQ_F_MQROOT) { NL_SET_ERR_MSG(extack, "Cannot attach rate estimator to a multi-queue root qdisc"); - goto err_out5; + goto err_out4; } err = gen_new_estimator(&sch->bstats, @@ -1312,7 +1312,7 @@ static struct Qdisc *qdisc_create(struct net_device *dev, tca[TCA_RATE]); if (err) { NL_SET_ERR_MSG(extack, "Failed to generate new estimator"); - goto err_out5; + goto err_out4; } } @@ -1321,12 +1321,13 @@ static struct Qdisc *qdisc_create(struct net_device *dev, return sch; -err_out5: - qdisc_put_stab(rtnl_dereference(sch->stab)); err_out4: - /* ops->init() failed, we call ->destroy() like qdisc_create_dflt() */ + /* Even if ops->init() failed, we call ops->destroy() + * like qdisc_create_dflt(). + */ if (ops->destroy) ops->destroy(sch); + qdisc_put_stab(rtnl_dereference(sch->stab)); err_out3: netdev_put(dev, &sch->dev_tracker); qdisc_free(sch);
On Fri, Feb 10, 2023 at 04:07:42PM +0100, Eric Dumazet wrote: > On Tue, Feb 7, 2023 at 2:55 PM Vladimir Oltean <vladimir.oltean@nxp.com> wrote: > > Move it earlier, which nicely seems to simplify the error handling path > > as well. > > Well... if you say so :) :) > If TCA_STAB attribute is malformed, we end up calling ->destroy() on a > not yet initialized qdisc :/ Right. Sorry, I didn't pay enough attention, and the old structure with "err_out4" having a "goto err_out3" confused me. Because I was trying to match the old teardown path with the new (linear) one, I was trying to keep the order between qdisc_put_stab() and ops->destroy(). But I forgot that I need to *reverse* it, since I reversed their order in the setup path :-/ > I am going to send the following fix, unless someone disagrees. > > (Moving qdisc_put_stab() _after_ ops->destroy(sch) is not strictly > needed for a fix, > but undo should be done in reverse steps for clarity. Right, and it's a net-next patch, so a larger fix which brings the code into shape should be fine. > > diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c > index e9780631b5b58202068e20c42ccf1197eac2194c..aba789c30a2eb50d339b8a888495b794825e1775 > 100644 > --- a/net/sched/sch_api.c > +++ b/net/sched/sch_api.c > @@ -1286,7 +1286,7 @@ static struct Qdisc *qdisc_create(struct net_device *dev, > stab = qdisc_get_stab(tca[TCA_STAB], extack); > if (IS_ERR(stab)) { > err = PTR_ERR(stab); > - goto err_out4; > + goto err_out3; > } > rcu_assign_pointer(sch->stab, stab); > } > @@ -1294,14 +1294,14 @@ static struct Qdisc *qdisc_create(struct > net_device *dev, > if (ops->init) { > err = ops->init(sch, tca[TCA_OPTIONS], extack); > if (err != 0) > - goto err_out5; > + goto err_out4; > } > > if (tca[TCA_RATE]) { > err = -EOPNOTSUPP; > if (sch->flags & TCQ_F_MQROOT) { > NL_SET_ERR_MSG(extack, "Cannot attach rate > estimator to a multi-queue root qdisc"); > - goto err_out5; > + goto err_out4; > } > > err = gen_new_estimator(&sch->bstats, > @@ -1312,7 +1312,7 @@ static struct Qdisc *qdisc_create(struct net_device *dev, > tca[TCA_RATE]); > if (err) { > NL_SET_ERR_MSG(extack, "Failed to generate new > estimator"); > - goto err_out5; > + goto err_out4; > } > } > > @@ -1321,12 +1321,13 @@ static struct Qdisc *qdisc_create(struct > net_device *dev, > > return sch; > > -err_out5: > - qdisc_put_stab(rtnl_dereference(sch->stab)); > err_out4: > - /* ops->init() failed, we call ->destroy() like qdisc_create_dflt() */ > + /* Even if ops->init() failed, we call ops->destroy() > + * like qdisc_create_dflt(). > + */ > if (ops->destroy) > ops->destroy(sch); > + qdisc_put_stab(rtnl_dereference(sch->stab)); > err_out3: > netdev_put(dev, &sch->dev_tracker); > qdisc_free(sch); I applied the changes from this patch manually, and the result looks good. Thanks (and sorry)!
diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c index c14018a8052c..e9780631b5b5 100644 --- a/net/sched/sch_api.c +++ b/net/sched/sch_api.c @@ -1282,12 +1282,6 @@ static struct Qdisc *qdisc_create(struct net_device *dev, if (err) goto err_out3; - if (ops->init) { - err = ops->init(sch, tca[TCA_OPTIONS], extack); - if (err != 0) - goto err_out5; - } - if (tca[TCA_STAB]) { stab = qdisc_get_stab(tca[TCA_STAB], extack); if (IS_ERR(stab)) { @@ -1296,11 +1290,18 @@ static struct Qdisc *qdisc_create(struct net_device *dev, } rcu_assign_pointer(sch->stab, stab); } + + if (ops->init) { + err = ops->init(sch, tca[TCA_OPTIONS], extack); + if (err != 0) + goto err_out5; + } + if (tca[TCA_RATE]) { err = -EOPNOTSUPP; if (sch->flags & TCQ_F_MQROOT) { NL_SET_ERR_MSG(extack, "Cannot attach rate estimator to a multi-queue root qdisc"); - goto err_out4; + goto err_out5; } err = gen_new_estimator(&sch->bstats, @@ -1311,7 +1312,7 @@ static struct Qdisc *qdisc_create(struct net_device *dev, tca[TCA_RATE]); if (err) { NL_SET_ERR_MSG(extack, "Failed to generate new estimator"); - goto err_out4; + goto err_out5; } } @@ -1321,6 +1322,8 @@ static struct Qdisc *qdisc_create(struct net_device *dev, return sch; err_out5: + qdisc_put_stab(rtnl_dereference(sch->stab)); +err_out4: /* ops->init() failed, we call ->destroy() like qdisc_create_dflt() */ if (ops->destroy) ops->destroy(sch); @@ -1332,16 +1335,6 @@ static struct Qdisc *qdisc_create(struct net_device *dev, err_out: *errp = err; return NULL; - -err_out4: - /* - * Any broken qdiscs that would require a ops->reset() here? - * The qdisc was never in action so it shouldn't be necessary. - */ - qdisc_put_stab(rtnl_dereference(sch->stab)); - if (ops->destroy) - ops->destroy(sch); - goto err_out3; } static int qdisc_change(struct Qdisc *sch, struct nlattr **tca,