| Message ID | 20231128160631.663351-4-pctammela@mojatatu.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net/sched: optimizations around action binding and init | expand |
On Tue, Nov 28, 2023 at 01:06:30PM -0300, Pedro Tammela wrote: > @@ -1510,10 +1510,8 @@ int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla, > err: > tcf_action_destroy(actions, flags & TCA_ACT_FLAGS_BIND); > err_mod: > - for (i = 0; i < TCA_ACT_MAX_PRIO; i++) { > - if (ops[i]) > - module_put(ops[i]->owner); > - } > + for (i = 0; i < TCA_ACT_MAX_PRIO && ops[i]; i++) > + module_put(ops[i]->owner); > return err; I was going to say: Maybe it's time for a helper macro for this. $ git grep TCA_ACT_MAX_PRIO include/net/pkt_cls.h: for (i = 0; i < TCA_ACT_MAX_PRIO && ((a) = (exts)->actions[i]); i++) include/net/pkt_cls.h: for (i = 0; i < TCA_ACT_MAX_PRIO && ((a) = actions[i]); i++) ... net/sched/act_api.c: for (i = 0; i < TCA_ACT_MAX_PRIO && actions[i]; i++) { net/sched/act_api.c: for (i = 0; i < TCA_ACT_MAX_PRIO; i++) { net/sched/act_api.c: for (i = 0; i < TCA_ACT_MAX_PRIO && actions[i]; i++) { net/sched/act_api.c: for (i = 0; i < TCA_ACT_MAX_PRIO; i++) { ... net/sched/act_api.c: for (i = 1; i <= TCA_ACT_MAX_PRIO && tb[i]; i++) { net/sched/act_api.c: for (i = 1; i <= TCA_ACT_MAX_PRIO && tb[i]; i++) { net/sched/act_api.c: for (i = 0; i < TCA_ACT_MAX_PRIO; i++) { net/sched/act_api.c: for (i = 0; i < TCA_ACT_MAX_PRIO && actions[i]; i++) { ... But then, that's exactly what the first 2 hits are :) So AFAICT this loop can be written as: struct struct tc_action_ops *op; tcf_act_for_each_action(i, op, ops) module_put(op->owner); Thoughts? It would be iterating over struct tc_action_ops and not tc_action, as in tcf_act_for_each_action() (which is the only user of this macro today), but that seems okay. Marcelo
On 28/11/2023 16:11, Marcelo Ricardo Leitner wrote: > On Tue, Nov 28, 2023 at 01:06:30PM -0300, Pedro Tammela wrote: >> @@ -1510,10 +1510,8 @@ int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla, >> err: >> tcf_action_destroy(actions, flags & TCA_ACT_FLAGS_BIND); >> err_mod: >> - for (i = 0; i < TCA_ACT_MAX_PRIO; i++) { >> - if (ops[i]) >> - module_put(ops[i]->owner); >> - } >> + for (i = 0; i < TCA_ACT_MAX_PRIO && ops[i]; i++) >> + module_put(ops[i]->owner); >> return err; > > I was going to say: > Maybe it's time for a helper macro for this. > > $ git grep TCA_ACT_MAX_PRIO > include/net/pkt_cls.h: for (i = 0; i < TCA_ACT_MAX_PRIO && ((a) = > (exts)->actions[i]); i++) > include/net/pkt_cls.h: for (i = 0; i < TCA_ACT_MAX_PRIO && ((a) = > actions[i]); i++) > ... > net/sched/act_api.c: for (i = 0; i < TCA_ACT_MAX_PRIO && actions[i]; i++) { > net/sched/act_api.c: for (i = 0; i < TCA_ACT_MAX_PRIO; i++) { > net/sched/act_api.c: for (i = 0; i < TCA_ACT_MAX_PRIO && actions[i]; i++) { > net/sched/act_api.c: for (i = 0; i < TCA_ACT_MAX_PRIO; i++) { > ... > net/sched/act_api.c: for (i = 1; i <= TCA_ACT_MAX_PRIO && tb[i]; i++) { > net/sched/act_api.c: for (i = 1; i <= TCA_ACT_MAX_PRIO && tb[i]; i++) { > net/sched/act_api.c: for (i = 0; i < TCA_ACT_MAX_PRIO; i++) { > net/sched/act_api.c: for (i = 0; i < TCA_ACT_MAX_PRIO && actions[i]; i++) { > ... > > But then, that's exactly what the first 2 hits are :) > So AFAICT this loop can be written as: > > struct struct tc_action_ops *op; > tcf_act_for_each_action(i, op, ops) > module_put(op->owner); > > Thoughts? It would be iterating over struct tc_action_ops and not > tc_action, as in tcf_act_for_each_action() (which is the only user of > this macro today), but that seems okay. > > Marcelo > Interesting, I didn't even notice those macros. I believe it helps with code maintainability. Do note, I saw a place that the action array is expected to be not contiguous. So any sed-like replacement must be done with care. When we know for sure it's contiguous, I'm all in for macros!
diff --git a/net/sched/act_api.c b/net/sched/act_api.c index bf6f9ca15a30..8517bfbd69a6 100644 --- a/net/sched/act_api.c +++ b/net/sched/act_api.c @@ -1510,10 +1510,8 @@ int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla, err: tcf_action_destroy(actions, flags & TCA_ACT_FLAGS_BIND); err_mod: - for (i = 0; i < TCA_ACT_MAX_PRIO; i++) { - if (ops[i]) - module_put(ops[i]->owner); - } + for (i = 0; i < TCA_ACT_MAX_PRIO && ops[i]; i++) + module_put(ops[i]->owner); return err; }
The ops array is contiguous, so stop processing whenever a NULL is found Signed-off-by: Pedro Tammela <pctammela@mojatatu.com> --- net/sched/act_api.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)