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