On Tue, 2022-06-28 at 13:15 +0200, Eelco Chaudron wrote: > > > On 28 Jun 2022, at 10:28, Jianbo Liu wrote: > > > On Tue, 2022-06-28 at 09:53 +0200, Eelco Chaudron wrote: > > > > > > > > > On 27 Jun 2022, at 16:58, Jianbo Liu wrote: > > > > > > > On Mon, 2022-06-27 at 11:32 +0200, Eelco Chaudron wrote: > > > > > > > > > > > > > > > On 21 Jun 2022, at 10:22, Jianbo Liu wrote: > > > > > > > > > > > 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. > > > > > > > > > > If this is the case, we should change the > > > > > dpif_netlink_meter_set() > > > > > API to return void. > > > > > And add a comment to the function why it would not return an > > > > > error: > > > > > > > > > > > > > OK. > > > > > > > > > --- a/lib/netdev-offload.c > > > > > +++ b/lib/netdev-offload.c > > > > > @@ -196,7 +196,7 @@ netdev_assign_flow_api(struct netdev > > > > > *netdev) > > > > > return -1; > > > > > } > > > > > > > > > > -int > > > > > +void > > > > > meter_offload_set(ofproto_meter_id meter_id, > > > > > struct ofputil_meter_config *config) > > > > > { > > > > > @@ -212,8 +212,8 @@ meter_offload_set(ofproto_meter_id > > > > > meter_id, > > > > > } > > > > > } > > > > > } > > > > > - > > > > > - return 0; > > > > > + /* Offload APIs could fail, for example, because the > > > > > offload > > > > > is > > > > > not > > > > > + * supported. This is fine, as the offload API should > > > > > take > > > > > care > > > > > of this. */ > > > > > } > > > > > > > > > > > > > > > > > > > > > > + err = meter_offload_set(meter_id, config); > > > > > > > > > > > > > > > + } > > > > > > > > + > > > > > > > > > > <SNIP> > > > > > > > > > > > > > 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? > > > > > > > > > > I did a lot of runs, but could not get it to fail without it. > > > > > So > > > > > if > > > > > it fails in your case it would be good to investigate. > > > > > > > > I can't reproduce today, though I run many times. It's related > > > > to > > > > my > > > > setup, I don't test on physical machine, but a virtual machine. > > > > > > > > > > > > > > > > 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. > > > > > > > > If I remember correctly, the used is "never", not "0.001s". > > > > > > > > > Forgot to answer this ;) > > > > > > The kernel always reports “never” for this test case, that’s why > > > I > > > suggested the offload disabled test to see all the differences > > > and > > > understand why. > > > > > > > Yes, it's "never" for the case with offload disabled. I have no > > idea > > about the reason which causes differences, but I don't think it's > > related to this patchset. Do you have any clue? Thanks! > > I have no proof, just a theory :) The first packet is handled by slow > path, so the rule is not hit. The succeeding packets are dropped by > the kernel, so they will not reach OVS’s flow processing. >
You are right, I tested. I will add the none-offload tests to show the difference. Thanks! _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
