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). 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. 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. 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 +]) + + +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 +#]) + +# 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 +#]) + +# 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
