| Message ID | 1cd15c879d51e38f6b189d41553e67a8a1de0250.1683326865.git.peilin.ye@bytedance.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net/sched: Fixes for sch_ingress and sch_clsact | expand |
On Fri, May 5, 2023 at 8:15 PM Peilin Ye <yepeilin.cs@gmail.com> wrote: > > Grafting ingress and clsact Qdiscs does not need a for-loop in > qdisc_graft(). Refactor it. No functional changes intended. > > Signed-off-by: Peilin Ye <peilin.ye@bytedance.com> Fixed John's email address. This one i am not so sure; num_q = 1 implies it will run on the for loop only once. I am not sure it improves readability either. Anyways for the effort you put into it i am tossing a coin and saying: Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com> Acked-by: Jamal Hadi Salim <jhs@mojatatu.com> cheers, jamal > 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 49b9c1bbfdd9..f72a581666a2 100644 > --- a/net/sched/sch_api.c > +++ b/net/sched/sch_api.c > @@ -1073,12 +1073,12 @@ static int qdisc_graft(struct net_device *dev, struct Qdisc *parent, > > if (parent == NULL) { > unsigned int i, num_q, ingress; > + struct netdev_queue *dev_queue; > > ingress = 0; > num_q = dev->num_tx_queues; > if ((q && q->flags & TCQ_F_INGRESS) || > (new && new->flags & TCQ_F_INGRESS)) { > - num_q = 1; > ingress = 1; > if (!dev_ingress_queue(dev)) { > NL_SET_ERR_MSG(extack, "Device does not have an ingress queue"); > @@ -1094,18 +1094,18 @@ static int qdisc_graft(struct net_device *dev, struct Qdisc *parent, > if (new && new->ops->attach && !ingress) > goto skip; > > - for (i = 0; i < num_q; i++) { > - struct netdev_queue *dev_queue = dev_ingress_queue(dev); > - > - if (!ingress) > + if (!ingress) { > + for (i = 0; i < num_q; i++) { > dev_queue = netdev_get_tx_queue(dev, i); > + old = dev_graft_qdisc(dev_queue, new); > > - old = dev_graft_qdisc(dev_queue, new); > - if (new && i > 0) > - qdisc_refcount_inc(new); > - > - if (!ingress) > + if (new && i > 0) > + qdisc_refcount_inc(new); > qdisc_put(old); > + } > + } else { > + dev_queue = dev_ingress_queue(dev); > + old = dev_graft_qdisc(dev_queue, new); > } > > skip: > -- > 2.20.1 >
On 05/05/2023 21:15, Peilin Ye wrote: > Grafting ingress and clsact Qdiscs does not need a for-loop in > qdisc_graft(). Refactor it. No functional changes intended. > > Signed-off-by: Peilin Ye <peilin.ye@bytedance.com> Just a FYI: If you decide to keep this refactoring, it will need to be back ported together with the subsequent fix. I would personally leave it to net-next. Thanks for chasing this! Tested-by: Pedro Tammela <pctammela@mojatatu.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 49b9c1bbfdd9..f72a581666a2 100644 > --- a/net/sched/sch_api.c > +++ b/net/sched/sch_api.c > @@ -1073,12 +1073,12 @@ static int qdisc_graft(struct net_device *dev, struct Qdisc *parent, > > if (parent == NULL) { > unsigned int i, num_q, ingress; > + struct netdev_queue *dev_queue; > > ingress = 0; > num_q = dev->num_tx_queues; > if ((q && q->flags & TCQ_F_INGRESS) || > (new && new->flags & TCQ_F_INGRESS)) { > - num_q = 1; > ingress = 1; > if (!dev_ingress_queue(dev)) { > NL_SET_ERR_MSG(extack, "Device does not have an ingress queue"); > @@ -1094,18 +1094,18 @@ static int qdisc_graft(struct net_device *dev, struct Qdisc *parent, > if (new && new->ops->attach && !ingress) > goto skip; > > - for (i = 0; i < num_q; i++) { > - struct netdev_queue *dev_queue = dev_ingress_queue(dev); > - > - if (!ingress) > + if (!ingress) { > + for (i = 0; i < num_q; i++) { > dev_queue = netdev_get_tx_queue(dev, i); > + old = dev_graft_qdisc(dev_queue, new); > > - old = dev_graft_qdisc(dev_queue, new); > - if (new && i > 0) > - qdisc_refcount_inc(new); > - > - if (!ingress) > + if (new && i > 0) > + qdisc_refcount_inc(new); > qdisc_put(old); > + } > + } else { > + dev_queue = dev_ingress_queue(dev); > + old = dev_graft_qdisc(dev_queue, new); > } > > skip:
On Mon, May 08, 2023 at 07:29:26AM -0400, Jamal Hadi Salim wrote: > On Fri, May 5, 2023 at 8:15 PM Peilin Ye <yepeilin.cs@gmail.com> wrote: > > > > Grafting ingress and clsact Qdiscs does not need a for-loop in > > qdisc_graft(). Refactor it. No functional changes intended. > > This one i am not so sure; num_q = 1 implies it will run on the for > loop only once. I am not sure it improves readability either. Anyways > for the effort you put into it i am tossing a coin and saying: > Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com> > Acked-by: Jamal Hadi Salim <jhs@mojatatu.com> Yeah, it doesn't make much difference itself. I'm just afraid that, without [5/6], [6/6] would look like: for (i = 0; i < num_q; i++) { if (!ingress) { dev_queue = netdev_get_tx_queue(dev, i); old = dev_graft_qdisc(dev_queue, new); else { old = dev_graft_qdisc(dev_queue, NULL); } if (new && i > 0) qdisc_refcount_inc(new); if (!ingress) { qdisc_put(old); } else { /* {ingress,clsact}_destroy() "old" before grafting "new" to avoid * unprotected concurrent accesses to net_device::miniq_{in,e}gress * pointer(s) in mini_qdisc_pair_swap(). */ qdisc_notify(net, skb, n, classid, old, new, extack); qdisc_destroy(old); } if (ingress) dev_graft_qdisc(dev_queue, new); } The "!ingress" path doesn't share a single line with "ingress", which looks a bit weird to me. Personally I'd like to keep [5/6]. Thanks, Peilin Ye
diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c index 49b9c1bbfdd9..f72a581666a2 100644 --- a/net/sched/sch_api.c +++ b/net/sched/sch_api.c @@ -1073,12 +1073,12 @@ static int qdisc_graft(struct net_device *dev, struct Qdisc *parent, if (parent == NULL) { unsigned int i, num_q, ingress; + struct netdev_queue *dev_queue; ingress = 0; num_q = dev->num_tx_queues; if ((q && q->flags & TCQ_F_INGRESS) || (new && new->flags & TCQ_F_INGRESS)) { - num_q = 1; ingress = 1; if (!dev_ingress_queue(dev)) { NL_SET_ERR_MSG(extack, "Device does not have an ingress queue"); @@ -1094,18 +1094,18 @@ static int qdisc_graft(struct net_device *dev, struct Qdisc *parent, if (new && new->ops->attach && !ingress) goto skip; - for (i = 0; i < num_q; i++) { - struct netdev_queue *dev_queue = dev_ingress_queue(dev); - - if (!ingress) + if (!ingress) { + for (i = 0; i < num_q; i++) { dev_queue = netdev_get_tx_queue(dev, i); + old = dev_graft_qdisc(dev_queue, new); - old = dev_graft_qdisc(dev_queue, new); - if (new && i > 0) - qdisc_refcount_inc(new); - - if (!ingress) + if (new && i > 0) + qdisc_refcount_inc(new); qdisc_put(old); + } + } else { + dev_queue = dev_ingress_queue(dev); + old = dev_graft_qdisc(dev_queue, new); } skip:
Grafting ingress and clsact Qdiscs does not need a for-loop in qdisc_graft(). Refactor it. No functional changes intended. Signed-off-by: Peilin Ye <peilin.ye@bytedance.com> --- net/sched/sch_api.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)