On Mon, 2022-06-20 at 12:18 +0200, Eelco Chaudron wrote:
> On 27 May 2022, at 11:00, Jianbo Liu wrote:
>
> > OVS meters are created in advance and openflow rules refer to them
> > by
> > their unique ID. New tc_police API is used to offload them. By
> > calling
> > the API, police actions are created and meters are mapped to them.
> > These actions then can be used in tc filter rules by the index.
> >
> > Signed-off-by: Jianbo Liu <[email protected]>
> > ---
> > NEWS | 2 ++
> > lib/dpif-netlink.c | 31 +++++++++++++++++----
> > tests/system-offloads-traffic.at | 48
> > ++++++++++++++++++++++++++++++++
> > 3 files changed, 76 insertions(+), 5 deletions(-)
> >
> > diff --git a/NEWS b/NEWS
> > index eece0d0b2..dfd659d4e 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -29,6 +29,8 @@ Post-v2.17.0
> > - Windows:
> > * Conntrack support for TCPv6, UDPv6, ICMPv6, FTPv6.
> > * IPv6 Geneve tunnel support.
> > + - Linux datapath:
> > + * Add offloading meter tc police.
> >
> >
> > v2.17.0 - 17 Feb 2022
> > diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> > index 06e1e8ca0..0af9ee77e 100644
> > --- a/lib/dpif-netlink.c
> > +++ b/lib/dpif-netlink.c
> > @@ -4163,11 +4163,18 @@ static int
> > dpif_netlink_meter_set(struct dpif *dpif_, ofproto_meter_id
> > meter_id,
> > struct ofputil_meter_config *config)
> > {
> > + int err;
> > +
> > if (probe_broken_meters(dpif_)) {
> > return ENOMEM;
> > }
> >
> > - return dpif_netlink_meter_set__(dpif_, meter_id, config);
> > + err = dpif_netlink_meter_set__(dpif_, meter_id, config);
> > + if (!err && netdev_is_flow_api_enabled()) {
> > + meter_offload_set(meter_id, config);
>
> Although currently we always return 0, we should still account for it
> to change in the future, so we should set err to the return value.
>
If there is err from meter_offload_set, it will be passed to the caller
of dpif_netlink_meter_set(). I don't agree with that, because the
caller thinks meter_set operation fail, but actually not. Besides, we
allow the case that dp meter_set success, but offloading fails, so the
return the error of meter_offload_set seems unnecessary.
> + err = meter_offload_set(meter_id, config);
>
> > + }
> > +
> > + return err;
> > }
> >
> > /* Retrieve statistics and/or delete meter 'meter_id'. Statistics
> > are
> > @@ -4258,16 +4265,30 @@ static int
> > dpif_netlink_meter_get(const struct dpif *dpif, ofproto_meter_id
> > meter_id,
> > struct ofputil_meter_stats *stats, uint16_t
> > max_bands)
> > {
> > - return dpif_netlink_meter_get_stats(dpif, meter_id, stats,
> > max_bands,
> > - OVS_METER_CMD_GET);
> > + int err;
> > +
> > + err = dpif_netlink_meter_get_stats(dpif, meter_id, stats,
> > max_bands,
> > + OVS_METER_CMD_GET);
> > + if (!err && netdev_is_flow_api_enabled()) {
> > + meter_offload_get(meter_id, stats, max_bands);
>
> + err = meter_offload_get(meter_id, stats, max_bands);
>
> > + }
> > +
> > + return err;
> > }
> >
> > static int
> > dpif_netlink_meter_del(struct dpif *dpif, ofproto_meter_id
> > meter_id,
> > struct ofputil_meter_stats *stats, uint16_t
> > max_bands)
> > {
> > - return dpif_netlink_meter_get_stats(dpif, meter_id, stats,
> > max_bands,
> > - OVS_METER_CMD_DEL);
> > + int err;
> > +
> > + err = dpif_netlink_meter_get_stats(dpif, meter_id, stats,
> > + max_bands,
> > OVS_METER_CMD_DEL);
> > + if (!err && netdev_is_flow_api_enabled()) {
> > + meter_offload_del(meter_id, stats, max_bands);
>
> + err = meter_offload_del(meter_id, stats, max_bands);
>
> > + }
> > +
> > + return err;
> > }
> >
> > static bool
> > diff --git a/tests/system-offloads-traffic.at b/tests/system-
> > offloads-traffic.at
> > index 80bc1dd5c..7ec75340f 100644
> > --- a/tests/system-offloads-traffic.at
> > +++ b/tests/system-offloads-traffic.at
> > @@ -168,3 +168,51 @@ matchall
> > ])
> > OVS_TRAFFIC_VSWITCHD_STOP
> > AT_CLEANUP
> > +
> > +AT_SETUP([offloads - check if meter offloading ])
>
> Can we replace if with interface, as I keep on reading it as "if".
>
Sure.
> > +AT_KEYWORDS([meter])
> > +AT_SKIP_IF([test $SUPPORT_TC_INGRESS_PPS = "no"])
> > +OVS_TRAFFIC_VSWITCHD_START()
> > +
> > +AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:hw-
> > offload=true])
> > +AT_CHECK([ovs-ofctl -O OpenFlow13 add-meter br0 'meter=1 pktps
> > bands=type=drop rate=1'])
> > +
> > +ADD_NAMESPACES(at_ns0, at_ns1)
> > +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24", "f0:00:00:01:01:01")
> > +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24", "f0:00:00:01:01:02")
> > +
> > +NS_CHECK_EXEC([at_ns0], [ip neigh add 10.1.1.2 lladdr
> > f0:00:00:01:01:02 dev p0])
> > +NS_CHECK_EXEC([at_ns1], [ip neigh add 10.1.1.1 lladdr
> > f0:00:00:01:01:01 dev p1])
> > +
> > +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0 "actions=normal"])
> > +NS_CHECK_EXEC([at_ns0], [ping -q -c 10 -i 0.1 -w 2 10.1.1.2 |
> > FORMAT_PING], [0], [dnl
> > +10 packets transmitted, 10 received, 0% packet loss, time 0ms
> > +])
> > +
> > +NETNS_DAEMONIZE([at_ns1], [nc -u -l 5678 > /dev/null ], [nc0.pid])
> > +
> > +AT_CHECK([ovs-ofctl -O OpenFlow13 del-flows br0])
> > +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0
> > "priority=10,in_port=ovs-p0,udp actions=meter:1,normal"])
> > +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0 "priority=1
> > actions=normal"])
> > +
> > +NS_CHECK_EXEC([at_ns0], [sleep 0.5; echo "mark" | nc -u 10.1.1.2
> > 5678 -p 6789])
>
> Any specific reason why you need the sleep 0.5 here? Is it to be sure
> the flow is programmed?
I remember I added this because there are failures sometimes. I don't
know why, but obviously they are related to this patchset. So I added
the sleep to avoid them. It's only 0.5s, should be no problem, right?
> If so, you might just want to do a ovs-vsctl dump-flows and checke
> the output?
>
I don't understand your qustion. I checked ovs-vsctl dump-flows below.
> > +AT_CHECK([ovs-appctl dpctl/dump-flows | grep "meter" |
> > DUMP_CLEAN_SORTED], [0], [dnl
> > +in_port(2),eth(macs),eth_type(0x0800),ipv4(proto=17,frag=no),
> > packets:0, bytes:0, used:0.001s, actions:outputmeter(0),3
> > +])
>
> Here you verify the DP rule is inserted, but should you not also wait
> a second to make sure the meter is reset?
OK. I will add.
> Because in theory your could/should sent 11 packets in 1 second, so
> 10 should be dropped!?
> This is the case in the kernel environment, but with TC the learning
> packet is not passing trough the TC meter (this might also be a
> corner case we need to document).
>
Yes, it's the reason. But where to document?
> > +for i in `seq 10`; do
> > +NS_CHECK_EXEC([at_ns0], [echo "mark" | nc -u 10.1.1.2 5678 -p
> > 6789])
> > +done
> > +
> > +AT_CHECK([ovs-appctl dpctl/dump-flows | grep "meter" |
> > DUMP_CLEAN_SORTED], [0], [dnl
> > +in_port(2),eth(macs),eth_type(0x0800),ipv4(proto=17,frag=no),
> > packets:10, bytes:330, used:0.001s, actions:outputmeter(0),3
> > +])
> > +
> > +AT_CHECK([ovs-ofctl -O OpenFlow13 meter-stats br0 | sed -e
> > 's/duration:[[0-9]].[[0-9]]*s/duration:0.001s/'], [0], [dnl
> > +OFPST_METER reply (OF1.3) (xid=0x2):
> > +meter:1 flow_count:1 packet_in_count:11 byte_in_count:377
> > duration:0.001s bands:
>
> byte_in_count:517 is a lot larger than with kernel code, do we know
> why? We should document it.
>
I think DP also count the bytes of mac layer.
> > +0: packet_count:9 byte_count:0
>
> Here in theory we should report byte_count, like this:
>
> 0: packet_count:10 byte_count:470
>
> However, TC does not support dropped byte count, only packet_count.
> So we should be ok for now, but we must add this limitation to the
> documentation somewhere that it's clear we will not report byte count
> with TC offload.
>
> If you run this test without HW offload enabled you can see the
> difference in behavior, and I think there should be none (or at least
> the corner cases should be documented).
> You could also add a "- offloads disabled" variant of this test, like
> other features do and add some additional reasoning why it's
> different there.
>
Sure, I will add.
> > +])
> > +
> > +OVS_TRAFFIC_VSWITCHD_STOP
> > +AT_CLEANUP
> > --
> > 2.26.2
>
> This concludes my review of v5.
>
> //Eelco
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
