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?
> > + 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
