On 18 May 2022, at 9:46, Jianbo Liu wrote:
> On Wed, 2022-05-18 at 09:19 +0200, Eelco Chaudron wrote: >> >> >> 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. >> > > OK, I will change to use hashmap. > Could you please reply my other questions, so I can prepare for the > next version? Thanks! Will try to do it early next week, as this is my last full day this week. >> 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). >> > > You can do it anyway, but the rule will be rejected by drivers :) > >>> #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
