On Fri, 2022-05-13 at 11:48 +0200, Eelco Chaudron wrote: > > > On 3 May 2022, at 5:08, Jianbo Liu via dev 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. > > I always start my review with some basic testing before looking at > the code in detail. > > I was hoping the self-test below would cover all, but it only covers > one small aspect of the actual feature. > So I think it should be enhanced to test the actual feature. I tried > to start it, as I had to test it anyway (see diff below). >
Thanks for your enhancing the tests. I didn't have much time to think about the tests before. > Note that the TC part is not very stable, so you need to fix that, as > every run might give different results. > Maybe you should also include a none offload test, as the other > features do. ping may not be the best tool to test meter, as kernel does not send packets blindldy as UDP does. We have to devise the tests very carefuly to get the expected result. > > Anyhow, the results are not the same as with the kernel (especially > the packet counts), so I think you might need to look into why this > is the case. I know the byte count is due to a kernel difference in > how bytes are counted, and I think someone from Nvidia was already > looking into this. > > The more worrying part is the missing counters in the meter-stats > output. > Yes, there is something missing in meter-stats. As there are being changes in the kernel tc (we are working on it), I'm not sure about how to do now. But, what's your concern if it is missing now, and we add them in the future? > So I guess this will give you something to do while I’ll start > reviewing the actual code ;) > > //Eelco > > > diff --git a/tests/system-offloads-traffic.at b/tests/system- > offloads-traffic.at > index b6f1e8411..169e7971d 100644 > --- a/tests/system-offloads-traffic.at > +++ b/tests/system-offloads-traffic.at > @@ -176,7 +176,54 @@ 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']) > -AT_CHECK([test `tc action list action police | grep 'police > 0x10000000' | wc -l` -eq 1 ]) > + > +ADD_NAMESPACES(at_ns0, at_ns1) > +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24") > +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24") > + > +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 > +]) > + As I said, ping is not the good choice. But if you want to test like this way, maybe you should sleep for several seconds to get stabled. I added "sleep 2" here. > + > +AT_CHECK([ovs-ofctl -O OpenFlow13 del-flows br0]) > +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0 > "priority=10,in_port=ovs-p0 actions=meter:1,normal"]) > +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0 "priority=1 > actions=normal"]) > + > +# With hw-offload=true this fails often, guess we need a more stable > test/setting for the meter setting > +NS_CHECK_EXEC([at_ns0], [ping -q -c 10 -i 0.1 -w 1 10.1.1.2 | > FORMAT_PING], [0], [dnl > +10 packets transmitted, 1 received, 90% packet loss, time 0ms > +]) > + > +# With fw-offload=false > +#AT_CHECK([ovs-appctl dpctl/dump-flows | grep "eth_type(0x0800)" | > DUMP_CLEAN_SORTED], [0], [dnl > +#in_port(2),eth(macs),eth_type(0x0800),ipv4(frag=no), packets:19, > bytes:1862, used:0.001s, actions:outputmeter(0),3 > +#in_port(3),eth(macs),eth_type(0x0800),ipv4(frag=no), packets:10, > bytes:980, used:0.001s, actions:output > +#]) > + > +# With fw-offload=true [this is also not stable with tc meter, need > to figure out why] > +#AT_CHECK([ovs-appctl dpctl/dump-flows | grep "eth_type(0x0800)" | > DUMP_CLEAN_SORTED], [0], [dnl > +#in_port(2),eth(macs),eth_type(0x0800),ipv4(frag=no), packets:11, > bytes:916, used:0.001s, actions:outputmeter(0),3 > +#in_port(3),eth(macs),eth_type(0x0800),ipv4(frag=no), packets:1, > bytes:84, used:0.001s, actions:output > +#]) After I added "sleep 2", I always got "packets:10, bytes: 840". > + > +# With fw-offload=true [this is also not stable with tc meter, need > to figure out why, also no counters for #0] > +#AT_CHECK([ovs-ofctl -O OpenFlow13 meter-stats br0], [0], [dnl > +#meter:1 flow_count:1 packet_in_count:11 byte_in_count:902 > duration:2.239s bands: > +#0: packet_count:0 byte_count:0 > +#]) > + Also we can add check "packet_in_count:10", but how do you check "duration", it can be changed every time? > +# With fw-offload=false > +#AT_CHECK([ovs-ofctl -O OpenFlow13 meter-stats br0], [0], [dnl > +#meter:1 flow_count:1 packet_in_count:12 byte_in_count:1140 > duration:2.141s bands: > +#0: packet_count:11 byte_count:1042 > +#]) > + > +# Add a test here to make sure use the correct meter in the tc > output. > +# tc -s filter show dev ovs-p0 ingress > + > +# AT_CHECK([test `tc action list action police | grep 'police > 0x10000000' | wc -l` -eq 1 ]) > > OVS_TRAFFIC_VSWITCHD_STOP > AT_CLEANUP > > > Signed-off-by: Jianbo Liu <[email protected]> > > --- > > NEWS | 2 ++ > > lib/dpif-netlink.c | 31 ++++++++++++++++++++++++++-- > > --- > > tests/system-offloads-traffic.at | 12 ++++++++++++ > > 3 files changed, 40 insertions(+), 5 deletions(-) > > > > diff --git a/NEWS b/NEWS > > index 1e107340f..bed398c2c 100644 > > --- a/NEWS > > +++ b/NEWS > > @@ -26,6 +26,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 71e35ccdd..2fa639529 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); > > + } > > + > > + 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); > > + } > > + > > + 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); > > + } > > + > > + return err; > > } > > > > static bool > > diff --git a/tests/system-offloads-traffic.at b/tests/system- > > offloads-traffic.at > > index 80bc1dd5c..b6f1e8411 100644 > > --- a/tests/system-offloads-traffic.at > > +++ b/tests/system-offloads-traffic.at > > @@ -168,3 +168,15 @@ matchall > > ]) > > OVS_TRAFFIC_VSWITCHD_STOP > > AT_CLEANUP > > + > > +AT_SETUP([offloads - check if meter is offloaded ]) > > +AT_KEYWORDS([meter]) > > +AT_SKIP_IF([test $HAVE_TC = "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']) > > +AT_CHECK([test `tc action list action police | grep 'police > > 0x10000000' | wc -l` -eq 1 ]) > > + > > +OVS_TRAFFIC_VSWITCHD_STOP > > +AT_CLEANUP > > -- > > 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
