| Message ID | 20220503030836.28783-3-jianbol@nvidia.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for ovs metering with tc offload | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
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 <jianbol@nvidia.com> > --- > 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. > + 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
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 <jianbol@nvidia.com> > > --- > > 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? > > + 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 > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
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 <jianbol@nvidia.com> >>> --- >>> 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 >>> dev@openvswitch.org >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>
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; + 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"); + return EPROTO; + } + + police = nl_attr_get(police_attrs[TCA_POLICE_TBF]); + 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 (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;
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. Signed-off-by: Jianbo Liu <jianbol@nvidia.com> --- lib/netdev-offload-tc.c | 4 +++ lib/tc.c | 59 +++++++++++++++++++++++++++++++++++++++++ lib/tc.h | 6 +++++ 3 files changed, 69 insertions(+)