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?
> > 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:
#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
