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! > 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
