| Message ID | 20230201161039.20714-3-ozsh@nvidia.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: flow_offload: add support for per action hw stats | expand |
On 01/02/2023 13:10, Oz Shlomo wrote: > A single tc pedit action may be translated to multiple flow_offload > actions. > Offload only actions that translate to a single pedit command value. > > Signed-off-by: Oz Shlomo <ozsh@nvidia.com> > --- > net/sched/act_pedit.c | 24 +++++++++++++++++++++++- > 1 file changed, 23 insertions(+), 1 deletion(-) > > diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c > index a0378e9f0121..abceef794f28 100644 > --- a/net/sched/act_pedit.c > +++ b/net/sched/act_pedit.c > @@ -522,7 +522,29 @@ static int tcf_pedit_offload_act_setup(struct tc_action *act, void *entry_data, > } > *index_inc = k; > } else { > - return -EOPNOTSUPP; > + struct flow_offload_action *fl_action = entry_data; > + u32 last_cmd; > + int k; > + > + for (k = 0; k < tcf_pedit_nkeys(act); k++) { > + u32 cmd = tcf_pedit_cmd(act, k); > + > + if (k && cmd != last_cmd) > + return -EOPNOTSUPP; I believe an extack message here is very valuable > + > + last_cmd = cmd; > + switch (cmd) { > + case TCA_PEDIT_KEY_EX_CMD_SET: > + fl_action->id = FLOW_ACTION_MANGLE; > + break; > + case TCA_PEDIT_KEY_EX_CMD_ADD: > + fl_action->id = FLOW_ACTION_ADD; > + break; > + default: > + NL_SET_ERR_MSG_MOD(extack, "Unsupported pedit command offload"); > + return -EOPNOTSUPP; > + } > + } Shouldn't this switch case be outside of the for-loop? > } > > return 0;
On 01/02/2023 22:59, Pedro Tammela wrote: > On 01/02/2023 13:10, Oz Shlomo wrote: >> A single tc pedit action may be translated to multiple flow_offload >> actions. >> Offload only actions that translate to a single pedit command value. >> >> Signed-off-by: Oz Shlomo <ozsh@nvidia.com> >> --- >> net/sched/act_pedit.c | 24 +++++++++++++++++++++++- >> 1 file changed, 23 insertions(+), 1 deletion(-) >> >> diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c >> index a0378e9f0121..abceef794f28 100644 >> --- a/net/sched/act_pedit.c >> +++ b/net/sched/act_pedit.c >> @@ -522,7 +522,29 @@ static int tcf_pedit_offload_act_setup(struct >> tc_action *act, void *entry_data, >> } >> *index_inc = k; >> } else { >> - return -EOPNOTSUPP; >> + struct flow_offload_action *fl_action = entry_data; >> + u32 last_cmd; >> + int k; >> + >> + for (k = 0; k < tcf_pedit_nkeys(act); k++) { >> + u32 cmd = tcf_pedit_cmd(act, k); >> + >> + if (k && cmd != last_cmd) >> + return -EOPNOTSUPP; > > I believe an extack message here is very valuable Sure thing, I will add one > >> + >> + last_cmd = cmd; >> + switch (cmd) { >> + case TCA_PEDIT_KEY_EX_CMD_SET: >> + fl_action->id = FLOW_ACTION_MANGLE; >> + break; >> + case TCA_PEDIT_KEY_EX_CMD_ADD: >> + fl_action->id = FLOW_ACTION_ADD; >> + break; >> + default: >> + NL_SET_ERR_MSG_MOD(extack, "Unsupported pedit >> command offload"); >> + return -EOPNOTSUPP; >> + } >> + } > > Shouldn't this switch case be outside of the for-loop? You are right, this can be done outside the for loop. I will refactor the code > >> } >> return 0; >
On Thu, Feb 02, 2023 at 09:21:15AM +0200, Oz Shlomo wrote: > > On 01/02/2023 22:59, Pedro Tammela wrote: > > On 01/02/2023 13:10, Oz Shlomo wrote: > > > A single tc pedit action may be translated to multiple flow_offload > > > actions. > > > Offload only actions that translate to a single pedit command value. > > > > > > Signed-off-by: Oz Shlomo <ozsh@nvidia.com> > > > --- > > > net/sched/act_pedit.c | 24 +++++++++++++++++++++++- > > > 1 file changed, 23 insertions(+), 1 deletion(-) > > > > > > diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c > > > index a0378e9f0121..abceef794f28 100644 > > > --- a/net/sched/act_pedit.c > > > +++ b/net/sched/act_pedit.c > > > @@ -522,7 +522,29 @@ static int tcf_pedit_offload_act_setup(struct > > > tc_action *act, void *entry_data, > > > } > > > *index_inc = k; > > > } else { > > > - return -EOPNOTSUPP; > > > + struct flow_offload_action *fl_action = entry_data; > > > + u32 last_cmd; > > > + int k; > > > + > > > + for (k = 0; k < tcf_pedit_nkeys(act); k++) { > > > + u32 cmd = tcf_pedit_cmd(act, k); > > > + > > > + if (k && cmd != last_cmd) > > > + return -EOPNOTSUPP; > > > > I believe an extack message here is very valuable > Sure thing, I will add one > > > > > + > > > + last_cmd = cmd; > > > + switch (cmd) { > > > + case TCA_PEDIT_KEY_EX_CMD_SET: > > > + fl_action->id = FLOW_ACTION_MANGLE; > > > + break; > > > + case TCA_PEDIT_KEY_EX_CMD_ADD: > > > + fl_action->id = FLOW_ACTION_ADD; > > > + break; > > > + default: > > > + NL_SET_ERR_MSG_MOD(extack, "Unsupported pedit > > > command offload"); > > > + return -EOPNOTSUPP; > > > + } > > > + } > > > > Shouldn't this switch case be outside of the for-loop? > > You are right, this can be done outside the for loop. To before the for-loop, that is? Because otherwise it will parse all commands and then fail, which seems heavier than how it is here. - validate the first one - ensure the rest follows > > I will refactor the code > > > > > > } > > > return 0; > > >
On 03/02/2023 17:31, Marcelo Ricardo Leitner wrote: > On Thu, Feb 02, 2023 at 09:21:15AM +0200, Oz Shlomo wrote: >> On 01/02/2023 22:59, Pedro Tammela wrote: >>> On 01/02/2023 13:10, Oz Shlomo wrote: >>>> A single tc pedit action may be translated to multiple flow_offload >>>> actions. >>>> Offload only actions that translate to a single pedit command value. >>>> >>>> Signed-off-by: Oz Shlomo <ozsh@nvidia.com> >>>> --- >>>> net/sched/act_pedit.c | 24 +++++++++++++++++++++++- >>>> 1 file changed, 23 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c >>>> index a0378e9f0121..abceef794f28 100644 >>>> --- a/net/sched/act_pedit.c >>>> +++ b/net/sched/act_pedit.c >>>> @@ -522,7 +522,29 @@ static int tcf_pedit_offload_act_setup(struct >>>> tc_action *act, void *entry_data, >>>> } >>>> *index_inc = k; >>>> } else { >>>> - return -EOPNOTSUPP; >>>> + struct flow_offload_action *fl_action = entry_data; >>>> + u32 last_cmd; >>>> + int k; >>>> + >>>> + for (k = 0; k < tcf_pedit_nkeys(act); k++) { >>>> + u32 cmd = tcf_pedit_cmd(act, k); >>>> + >>>> + if (k && cmd != last_cmd) >>>> + return -EOPNOTSUPP; >>> I believe an extack message here is very valuable >> Sure thing, I will add one >>>> + >>>> + last_cmd = cmd; >>>> + switch (cmd) { >>>> + case TCA_PEDIT_KEY_EX_CMD_SET: >>>> + fl_action->id = FLOW_ACTION_MANGLE; >>>> + break; >>>> + case TCA_PEDIT_KEY_EX_CMD_ADD: >>>> + fl_action->id = FLOW_ACTION_ADD; >>>> + break; >>>> + default: >>>> + NL_SET_ERR_MSG_MOD(extack, "Unsupported pedit >>>> command offload"); >>>> + return -EOPNOTSUPP; >>>> + } >>>> + } >>> Shouldn't this switch case be outside of the for-loop? >> You are right, this can be done outside the for loop. > To before the for-loop, that is? > Because otherwise it will parse all commands and then fail, which seems heavier > than how it is here. > > - validate the first one > - ensure the rest follows Right >> I will refactor the code >> >>>> } >>>> return 0;
diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c index a0378e9f0121..abceef794f28 100644 --- a/net/sched/act_pedit.c +++ b/net/sched/act_pedit.c @@ -522,7 +522,29 @@ static int tcf_pedit_offload_act_setup(struct tc_action *act, void *entry_data, } *index_inc = k; } else { - return -EOPNOTSUPP; + struct flow_offload_action *fl_action = entry_data; + u32 last_cmd; + int k; + + for (k = 0; k < tcf_pedit_nkeys(act); k++) { + u32 cmd = tcf_pedit_cmd(act, k); + + if (k && cmd != last_cmd) + return -EOPNOTSUPP; + + last_cmd = cmd; + switch (cmd) { + case TCA_PEDIT_KEY_EX_CMD_SET: + fl_action->id = FLOW_ACTION_MANGLE; + break; + case TCA_PEDIT_KEY_EX_CMD_ADD: + fl_action->id = FLOW_ACTION_ADD; + break; + default: + NL_SET_ERR_MSG_MOD(extack, "Unsupported pedit command offload"); + return -EOPNOTSUPP; + } + } } return 0;
A single tc pedit action may be translated to multiple flow_offload actions. Offload only actions that translate to a single pedit command value. Signed-off-by: Oz Shlomo <ozsh@nvidia.com> --- net/sched/act_pedit.c | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-)