On 3 May 2022, at 5:08, Jianbo Liu via dev wrote:
> When offloading rule, tc should be filled with police index, instead
> of meter id. As meter is mapped to police action, and the mapping is
> inserted into meter_id_to_police_idx hmap, this hmap is used to find
> the police index. Besides, an action cookie is added to save meter id
> on rule creation, so meter id can be retrieved from the cookie, and
> pass to dpif while dumping rules.
>
> Signed-off-by: Jianbo Liu <[email protected]>
> ---
> lib/netdev-offload-tc.c | 16 +++++++++++++++-
> lib/tc.c | 25 ++++++++++++++++++++++++-
> 2 files changed, 39 insertions(+), 2 deletions(-)
>
> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> index bded4bc8c..fa3e8113b 100644
> --- a/lib/netdev-offload-tc.c
> +++ b/lib/netdev-offload-tc.c
> @@ -169,6 +169,8 @@ static struct netlink_field set_flower_map[][4] = {
> },
> };
>
> +static int meter_id_lookup(uint32_t meter_id, uint32_t *police_idx);
> +
Maybe move this up to the place where you also define the meter mutex/ids?
> static struct ovs_mutex ufid_lock = OVS_MUTEX_INITIALIZER;
>
> /**
> @@ -1038,7 +1040,8 @@ parse_tc_flower_to_match(struct tc_flower *flower,
> }
> break;
> case TC_ACT_POLICE: {
> - /* Not supported yet */
> + nl_msg_put_u32(buf, OVS_ACTION_ATTR_METER,
> + action->police.meter_id);
> }
> break;
> }
> @@ -1961,6 +1964,17 @@ netdev_tc_flow_put(struct netdev *netdev, struct match
> *match,
> action->type = TC_ACT_GOTO;
> action->chain = 0; /* 0 is reserved and not used by recirc. */
> flower.action_count++;
> + } else if (nl_attr_type(nla) == OVS_ACTION_ATTR_METER) {
> + uint32_t police_index;
> +
> + action->type = TC_ACT_POLICE;
> + action->police.meter_id = nl_attr_get_u32(nla);
> + if (meter_id_lookup(action->police.meter_id, &police_index)) {
> + return EOPNOTSUPP;
> + }
> +
> + action->police.index = police_index;
> + flower.action_count++;
> } else {
> VLOG_DBG_RL(&rl, "unsupported put action type: %d",
> nl_attr_type(nla));
> diff --git a/lib/tc.c b/lib/tc.c
> index 16d4ae3da..ecdcb676d 100644
> --- a/lib/tc.c
> +++ b/lib/tc.c
> @@ -2421,6 +2421,22 @@ nl_msg_put_act_gact(struct ofpbuf *request, uint32_t
> chain)
> nl_msg_end_nested(request, offset);
> }
>
> +static void
> +nl_msg_put_act_police_index(struct ofpbuf *request, uint32_t police_idx)
> +{
> + struct tc_police police;
> + size_t offset;
> +
> + memset(&police, 0, sizeof police);
> + police.index = police_idx;
> +
> + nl_msg_put_string(request, TCA_ACT_KIND, "police");
> + offset = nl_msg_start_nested(request, TCA_ACT_OPTIONS);
> + nl_msg_put_unspec(request, TCA_POLICE_TBF, &police, sizeof police);
> + nl_msg_put_u32(request, TCA_POLICE_RESULT, TC_ACT_PIPE);
> + nl_msg_end_nested(request, offset);
> +}
> +
> static void
> nl_msg_put_act_ct(struct ofpbuf *request, struct tc_action *action)
> {
> @@ -2911,7 +2927,14 @@ nl_msg_put_flower_acts(struct ofpbuf *request, struct
> tc_flower *flower)
> }
> break;
> case TC_ACT_POLICE: {
> - /* Not supported yet */
> + struct tc_cookie act_cookie;
> +
> + act_offset = nl_msg_start_nested(request, act_index++);
> + nl_msg_put_act_police_index(request, action->police.index);
> + act_cookie.data = &action->police.meter_id;
> + act_cookie.len = sizeof(action->police.meter_id);
> + nl_msg_put_act_cookie(request, &act_cookie);
I think we should only set the action cookie to flower->act_cookie as the
entire infrastructure relies on this being the ufid. If you need to store some
meter_id, you might need to do it in tcf_id somehow. See your colleague's sflow
patch he has the same problem as he needs to store the sflow id. Guess this
also requires changes in nl_parse_act_police() as you can not use the
action_cookie.
> + nl_msg_end_nested(request, act_offset);
> }
> break;
> }
> --
> 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
