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. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
