On 17 May 2022, at 11:25, Jianbo Liu wrote:
> On Fri, 2022-05-13 at 14:28 +0200, Eelco Chaudron wrote: >> >> >> On 3 May 2022, at 5:08, Jianbo Liu via dev wrote: >> >>> Add function to parse police action from netlink message, and meter >>> id >>> can be retrieved from action cockie as it will be saved there in >>> later >>> patch. >> >> See comments inline below… >> >>> Signed-off-by: Jianbo Liu <[email protected]> >>> --- >>> lib/netdev-offload-tc.c | 4 +++ >>> lib/tc.c | 59 >>> +++++++++++++++++++++++++++++++++++++++++ >>> lib/tc.h | 6 +++++ >>> 3 files changed, 69 insertions(+) >>> >>> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c >>> index a41b62758..83d57c63b 100644 >>> --- a/lib/netdev-offload-tc.c >>> +++ b/lib/netdev-offload-tc.c >>> @@ -1017,6 +1017,10 @@ parse_tc_flower_to_match(struct tc_flower >>> *flower, >>> nl_msg_put_u32(buf, OVS_ACTION_ATTR_RECIRC, >>> action->chain); >>> } >>> break; >>> + case TC_ACT_POLICE: { >>> + /* Not supported yet */ >>> + } >>> + break; >>> } >>> } >>> } >>> diff --git a/lib/tc.c b/lib/tc.c >>> index df73a43d4..af7a7bc6d 100644 >>> --- a/lib/tc.c >>> +++ b/lib/tc.c >>> @@ -1339,6 +1339,59 @@ nl_parse_act_gact(struct nlattr *options, >>> struct tc_flower *flower) >>> return 0; >>> } >>> >>> +static const struct nl_policy police_policy[] = { >>> + [TCA_POLICE_TBF] = { .type = NL_A_UNSPEC, >>> + .min_len = sizeof(struct tc_police), >>> + .optional = false, }, >>> + [TCA_POLICE_RATE] = { .type = NL_A_UNSPEC, >>> + .min_len = 1024, >>> + .optional = true, }, >>> + [TCA_POLICE_PEAKRATE] = { .type = NL_A_UNSPEC, >>> + .min_len = 1024, >>> + .optional = true, }, >>> + [TCA_POLICE_AVRATE] = { .type = NL_A_U32, >>> + .optional = true, }, >>> + [TCA_POLICE_RESULT] = { .type = NL_A_U32, >>> + .optional = true, }, >>> + [TCA_POLICE_TM] = { .type = NL_A_UNSPEC, >>> + .min_len = sizeof(struct tcf_t), >>> + .optional = true, }, >>> +}; >>> + >>> +static int >>> +nl_parse_act_police(const struct nlattr *options, struct tc_flower >>> *flower, >>> + struct nlattr *act_cookie) >>> +{ >>> + struct nlattr *police_attrs[ARRAY_SIZE(police_policy)] = {}; >>> + struct tc_action *action; >>> + const struct tc_police *police; >> >> For the new code, I review I try to stick to reverse Christmas tree >> style, so as you might need to make changes may also swap the two- >> line? >> >>> + struct nlattr *police_tm; >>> + const struct tcf_t *tm; >>> + >>> + if (!nl_parse_nested(options, police_policy, police_attrs, >>> + ARRAY_SIZE(police_policy))) { >>> + VLOG_ERR_RL(&error_rl, "failed to parse police action >>> options"); >> >> Change failed to a capital, so “Failed to parse..." >> >>> + return EPROTO; >>> + } >>> + >>> + police = nl_attr_get(police_attars[TCA_POLICE_TBF]); >> >> police = nl_attr_get_unspec(police_attrs[TCA_POLICE_TBF], sizeof >> *police); >> >>> + action = &flower->actions[flower->action_count++]; >>> + action->type = TC_ACT_POLICE; >>> + action->police.index = police->index; >>> + >>> + if (act_cookie) { >>> + action->police.meter_id = nl_attr_get_u32(act_cookie); >>> + } >>> + >>> + police_tm = police_attrs[TCA_POLICE_TM]; >> >> If you use optional attributes, you have to make sure they are >> present. Guess the fix would be to set .optional = false for >> TCA_POLICE_TM. >> > > I checked police_tm at below line, to make sure they are present. So > why need to set .optional=false? ACK you are right! I was miss reading this! >>> + if (police_tm) { >>> + tm = nl_attr_get_unspec(police_tm, sizeof *tm); >>> + nl_parse_tcf(tm, flower); >>> + } >>> + >>> + return 0; >>> +} >>> + >>> static const struct nl_policy mirred_policy[] = { >>> [TCA_MIRRED_PARMS] = { .type = NL_A_UNSPEC, >>> .min_len = sizeof(struct tc_mirred), >>> @@ -1761,6 +1814,8 @@ nl_parse_single_action(struct nlattr *action, >>> struct tc_flower *flower, >>> /* Added for TC rule only (not in OvS rule) so ignore. */ >>> } else if (!strcmp(act_kind, "ct")) { >>> nl_parse_act_ct(act_options, flower); >>> + } else if (!strcmp(act_kind, "police")) { >>> + nl_parse_act_police(act_options, flower, act_cookie); >>> } else { >>> VLOG_ERR_RL(&error_rl, "unknown tc action kind: %s", >>> act_kind); >>> err = EINVAL; >>> @@ -2773,6 +2828,10 @@ nl_msg_put_flower_acts(struct ofpbuf >>> *request, struct tc_flower *flower) >>> nl_msg_end_nested(request, act_offset); >>> } >>> break; >>> + case TC_ACT_POLICE: { >>> + /* Not supported yet */ >>> + } >>> + break; >>> } >>> } >>> } >>> diff --git a/lib/tc.h b/lib/tc.h >>> index d6cdddd16..201345672 100644 >>> --- a/lib/tc.h >>> +++ b/lib/tc.h >>> @@ -174,6 +174,7 @@ enum tc_action_type { >>> TC_ACT_MPLS_SET, >>> TC_ACT_GOTO, >>> TC_ACT_CT, >>> + TC_ACT_POLICE, >>> }; >>> >>> enum nat_type { >>> @@ -261,6 +262,11 @@ struct tc_action { >>> struct tc_flower_key key; >>> struct tc_flower_key mask; >>> } rewrite; >>> + >>> + struct { >>> + uint32_t index; >>> + uint32_t meter_id; >>> + } police; >>> }; >>> >>> enum tc_action_type type; >>> -- >>> 2.26.2 >>> >>> _______________________________________________ >>> dev mailing list >>> [email protected] >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
