| Message ID | 20240509024043.3532677-1-xiaolei.wang@windriver.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net/sched: Get stab before calling ops->change() | expand |
On Thu, May 09, 2024 at 10:40:43AM +0800, Xiaolei Wang wrote: > ops->change() depends on stab, there is such a situation > When no parameters are passed in for the first time, stab > is omitted, as in configuration 1 below. At this time, a > warning "Warning: sch_taprio: Size table not specified, frame > length estimates may be inaccurate" will be received. When > stab is added for the second time, parameters, like configuration > 2 below, because the stab is still empty when ops->change() > is running, you will also receive the above warning. > > 1. tc qdisc replace dev eth1 parent root handle 100 taprio \ > num_tc 5 map 0 1 2 3 4 queues 1@0 1@1 1@2 1@3 1@4 base-time 0 \ > sched-entry S 1 100000 \ > sched-entry S 2 100000 \ > sched-entry S 4 100000 \ > max-sdu 0 0 0 0 0 0 0 200 \ > flags 2 > > 2. tc qdisc replace dev eth1 parent root overhead 24 handle 100 taprio \ > num_tc 5 map 0 1 2 3 4 queues 1@0 1@1 1@2 1@3 1@4 base-time 0 \ > sched-entry S 1 100000 \ > sched-entry S 2 100000 \ > sched-entry S 4 100000 \ > max-sdu 0 0 0 0 0 0 0 200 \ > flags 2 > Hi Xiaolei Wang, If this is a fix, targeted at the net tree, then it should probably have a Fixes tag here (no blank line between it and other tags). > Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com> > --- > net/sched/sch_api.c | 20 ++++++++++---------- > 1 file changed, 10 insertions(+), 10 deletions(-) > > diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c > index 60239378d43f..fec358f497d5 100644 > --- a/net/sched/sch_api.c > +++ b/net/sched/sch_api.c > @@ -1404,6 +1404,16 @@ static int qdisc_change(struct Qdisc *sch, struct nlattr **tca, > struct qdisc_size_table *ostab, *stab = NULL; > int err = 0; > > + if (tca[TCA_STAB]) { > + stab = qdisc_get_stab(tca[TCA_STAB], extack); > + if (IS_ERR(stab)) > + return PTR_ERR(stab); > + } > + > + ostab = rtnl_dereference(sch->stab); > + rcu_assign_pointer(sch->stab, stab); > + qdisc_put_stab(ostab); > + > if (tca[TCA_OPTIONS]) { > if (!sch->ops->change) { > NL_SET_ERR_MSG(extack, "Change operation not supported by specified qdisc"); I am concerned that in this case the stab will be updated even if the change operation is rejected by the following code in the if (tca[TCA_OPTIONS]) block, just below the above hunk. if (tca[TCA_INGRESS_BLOCK] || tca[TCA_EGRESS_BLOCK]) { NL_SET_ERR_MSG(extack, "Change of blocks is not supported"); return -EOPNOTSUPP; } ...
On 5/10/24 11:19 PM, Simon Horman wrote: > CAUTION: This email comes from a non Wind River email account! > Do not click links or open attachments unless you recognize the sender and know the content is safe. > > On Thu, May 09, 2024 at 10:40:43AM +0800, Xiaolei Wang wrote: >> ops->change() depends on stab, there is such a situation >> When no parameters are passed in for the first time, stab >> is omitted, as in configuration 1 below. At this time, a >> warning "Warning: sch_taprio: Size table not specified, frame >> length estimates may be inaccurate" will be received. When >> stab is added for the second time, parameters, like configuration >> 2 below, because the stab is still empty when ops->change() >> is running, you will also receive the above warning. >> >> 1. tc qdisc replace dev eth1 parent root handle 100 taprio \ >> num_tc 5 map 0 1 2 3 4 queues 1@0 1@1 1@2 1@3 1@4 base-time 0 \ >> sched-entry S 1 100000 \ >> sched-entry S 2 100000 \ >> sched-entry S 4 100000 \ >> max-sdu 0 0 0 0 0 0 0 200 \ >> flags 2 >> >> 2. tc qdisc replace dev eth1 parent root overhead 24 handle 100 taprio \ >> num_tc 5 map 0 1 2 3 4 queues 1@0 1@1 1@2 1@3 1@4 base-time 0 \ >> sched-entry S 1 100000 \ >> sched-entry S 2 100000 \ >> sched-entry S 4 100000 \ >> max-sdu 0 0 0 0 0 0 0 200 \ >> flags 2 >> > Hi Xiaolei Wang, > > If this is a fix, targeted at the net tree, then it should probably have > a Fixes tag here (no blank line between it and other tags). > >> Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com> >> --- >> net/sched/sch_api.c | 20 ++++++++++---------- >> 1 file changed, 10 insertions(+), 10 deletions(-) >> >> diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c >> index 60239378d43f..fec358f497d5 100644 >> --- a/net/sched/sch_api.c >> +++ b/net/sched/sch_api.c >> @@ -1404,6 +1404,16 @@ static int qdisc_change(struct Qdisc *sch, struct nlattr **tca, >> struct qdisc_size_table *ostab, *stab = NULL; >> int err = 0; >> >> + if (tca[TCA_STAB]) { >> + stab = qdisc_get_stab(tca[TCA_STAB], extack); >> + if (IS_ERR(stab)) >> + return PTR_ERR(stab); >> + } >> + >> + ostab = rtnl_dereference(sch->stab); >> + rcu_assign_pointer(sch->stab, stab); >> + qdisc_put_stab(ostab); >> + >> if (tca[TCA_OPTIONS]) { >> if (!sch->ops->change) { >> NL_SET_ERR_MSG(extack, "Change operation not supported by specified qdisc"); > I am concerned that in this case the stab will be updated even if the > change operation is rejected by the following code in the if > (tca[TCA_OPTIONS]) block, just below the above hunk. > > if (tca[TCA_INGRESS_BLOCK] || tca[TCA_EGRESS_BLOCK]) { > NL_SET_ERR_MSG(extack, "Change of blocks is not supported"); > return -EOPNOTSUPP; > } > ... Well, that makes sense, I will research further thanks xiaolei > > -- > pw-bot: under-review
diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c index 60239378d43f..fec358f497d5 100644 --- a/net/sched/sch_api.c +++ b/net/sched/sch_api.c @@ -1404,6 +1404,16 @@ static int qdisc_change(struct Qdisc *sch, struct nlattr **tca, struct qdisc_size_table *ostab, *stab = NULL; int err = 0; + if (tca[TCA_STAB]) { + stab = qdisc_get_stab(tca[TCA_STAB], extack); + if (IS_ERR(stab)) + return PTR_ERR(stab); + } + + ostab = rtnl_dereference(sch->stab); + rcu_assign_pointer(sch->stab, stab); + qdisc_put_stab(ostab); + if (tca[TCA_OPTIONS]) { if (!sch->ops->change) { NL_SET_ERR_MSG(extack, "Change operation not supported by specified qdisc"); @@ -1418,16 +1428,6 @@ static int qdisc_change(struct Qdisc *sch, struct nlattr **tca, return err; } - if (tca[TCA_STAB]) { - stab = qdisc_get_stab(tca[TCA_STAB], extack); - if (IS_ERR(stab)) - return PTR_ERR(stab); - } - - ostab = rtnl_dereference(sch->stab); - rcu_assign_pointer(sch->stab, stab); - qdisc_put_stab(ostab); - if (tca[TCA_RATE]) { /* NB: ignores errors from replace_estimator because change can't be undone. */
ops->change() depends on stab, there is such a situation When no parameters are passed in for the first time, stab is omitted, as in configuration 1 below. At this time, a warning "Warning: sch_taprio: Size table not specified, frame length estimates may be inaccurate" will be received. When stab is added for the second time, parameters, like configuration 2 below, because the stab is still empty when ops->change() is running, you will also receive the above warning. 1. tc qdisc replace dev eth1 parent root handle 100 taprio \ num_tc 5 map 0 1 2 3 4 queues 1@0 1@1 1@2 1@3 1@4 base-time 0 \ sched-entry S 1 100000 \ sched-entry S 2 100000 \ sched-entry S 4 100000 \ max-sdu 0 0 0 0 0 0 0 200 \ flags 2 2. tc qdisc replace dev eth1 parent root overhead 24 handle 100 taprio \ num_tc 5 map 0 1 2 3 4 queues 1@0 1@1 1@2 1@3 1@4 base-time 0 \ sched-entry S 1 100000 \ sched-entry S 2 100000 \ sched-entry S 4 100000 \ max-sdu 0 0 0 0 0 0 0 200 \ flags 2 Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com> --- net/sched/sch_api.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)