On 18 May 2022, at 8:54, Jianbo Liu wrote:
> On Wed, 2022-05-18 at 08:25 +0200, Eelco Chaudron wrote: >> >> >> 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). >> > > To be clarify, the cookie is per action, not per rule. We can add a > hmap to find meter_id by police index if you don't like cookie. > What do you think? As you already have the reverse, you can re-use the data structure. I would prefer this over re-purposing the cookie as we might introduce corner cases. I have a lot of cleanup items for TC, and this is one of them, so if I ever get time, I want to re-work the code to make sure we have a “dedicated” action(s) we use for the ufid cookie which should always be present. >>> 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. >> > > There should be no conflict, as ufid is stored in the last action, > which is never to be police action. Please check this: Well, I can configure it to be the last action (it would not make sense, but you never know what a user might do). > #tc -s filter show dev enp8s0f0_1 ingress > filter protocol ip pref 4 flower chain 0 > filter protocol ip pref 4 flower chain 0 handle 0x1 > eth_type ipv4 > src_ip 7.7.7.2 > ip_flags nofrag > in_hw in_hw_count 1 > action order 1: 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 > > action order 2: mirred (Egress Redirect to device enp8s0f0_0) > stolen > index 4 ref 1 bind 1 installed 10 sec used 0 sec firstused 10 > sec > Action statistics: > Sent 231456428 bytes 160511 pkt (dropped 0, overlimits 0 > requeues 0) > Sent software 44268 bytes 31 pkt > Sent hardware 231412160 bytes 160480 pkt > backlog 0b 0p requeues 0 > cookie 02813d4f594f43ef7cacd8bb277ba11f > no_percpu > used_hw_stats delayed > >>> 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
