On 18 May 2022, at 5:27, Jianbo Liu wrote:
> On Mon, 2022-05-16 at 13:34 +0200, Eelco Chaudron wrote: >> >> >> 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. >> > > tcf_id is used to find ufid. But I don't think it's proper to put > meter_id in tcf_id, because not all rules have meter. From this logic, > meter_id can't be a part of the id or key for a rule. So by design, the TC rules were supposed to be stateless, but as with sflow offload and this patch, this is no longer true so we need some way to store this data. But the action cookie in the current design is exclusively used to store the ufid, so this is where we need to piggyback on (or completely re-design how the ufid is stored). > There are several benifits to use action_cookie. First, we don't need > to searh HMAP, either by ufid, or police index (we can add the mapping > of meter id and police index). Secondly, it's helpful for debugging, > you can check police action and its meter id with tc command. I agree with this but the problem is that the action cookie in the current design is exclusively used to store the ufid, and your change is breaking this. > For example: > #tc -s action list action police > total acts 1 > > action order 0: police 0x10000000 rate 200Mbit burst 8175b mtu > 64Kb action drop/pipe overhead 0b > ref 2 bind 1 installed 12 sec used 0 sec firstused 10 sec > Action statistics: > Sent 232494584 bytes 161238 pkt (dropped 727, overlimits 727 > requeues 0) > Sent software 1082424 bytes 758 pkt > Sent hardware 231412160 bytes 160480 pkt > backlog 0b 0p requeues 0 > cookie 00000000 > used_hw_stats delayed > > >>> + 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
