On 16 Mar 2023, at 7:51, Chris Mi wrote:
> On 3/16/2023 11:37 AM, Chris Mi via dev wrote:
>> On 3/16/2023 12:40 AM, Eelco Chaudron wrote:
>>>
>>> On 15 Mar 2023, at 11:07, Chris Mi wrote:
>>>
>>>> On 3/15/2023 5:28 PM, Eelco Chaudron wrote:
>>>>> On 15 Mar 2023, at 4:40, Chris Mi wrote:
>>>>>
>>>>>> On 3/10/2023 10:02 PM, Eelco Chaudron wrote:
>>>>>>> On 10 Mar 2023, at 13:11, Eelco Chaudron wrote:
>>>>>>>
>>>>>>>> On 1 Mar 2023, at 8:22, Chris Mi wrote:
>>>>>>>>
>>>>>>>>> This patch set adds offload support for sFlow.
>>>>>>>>>
>>>>>>>>> Psample is a genetlink channel for packet sampling. TC action
>>>>>>>>> act_sample
>>>>>>>>> uses psample to send sampled packets to userspace.
>>>>>>>>>
>>>>>>>>> When offloading sample action to TC, userspace creates a unique ID to
>>>>>>>>> map sFlow action and tunnel info and passes this ID to kernel instead
>>>>>>>>> of the sFlow info. psample will send this ID and sampled packet to
>>>>>>>>> userspace. Using the ID, userspace can recover the sFlow info and send
>>>>>>>>> sampled packet to the right sFlow monitoring host.
>>>>>>>> I started to review, but the test cases are failing in my setups.
>>>>>>>> Maybe you need to re-base your build as some stuff has changed related
>>>>>>>> to flow statistic updates.
>>>>>>>>
>>>>>>>> On RHEL9/Fedora 37:
>>>>>>>>
>>>>>>>> 3: sflow offloads with sampling=1 - ping between two ports -
>>>>>>>> offloads enabled FAILED (system-offloads-traffic.at:133)
>>>>>>>> 4: sflow offloads with sampling=2 - ping between two ports -
>>>>>>>> offloads enabled FAILED (system-offloads-traffic.at:191)
>>>>>>>>
>>>>>>>>
>>>>>>>> 3:
>>>>>>>>
>>>>>>>> ./system-offloads-traffic.at:133: ovs-appctl dpctl/dump-flows
>>>>>>>> type=tc,offloaded | grep "in_port(2)" | grep "eth_type(0x0800)" | grep
>>>>>>>> "actions:userspace" | grep "sFlow" | sed -e
>>>>>>>> "s/used:[0-9].[0-9]*s/used:0.001s/;s/eth(src=[a-z0-9:]*,dst=
>>>>>>>> [a-z0-9:]*)/eth(macs)/;s/pid=[0-9]*/pid=1/;s/output=$P1_IFINDEX/output=1/"
>>>>>>>> --- - 2023-03-10 12:59:37.008456662 +0100
>>>>>>>> +++
>>>>>>>> /root/Documents/reviews/ovs_cmi_tc_sflow/OVS_master_DPDK_v22.11/ovs_github/tests/system-offloads-testsuite.dir/at-groups/3/stdout
>>>>>>>> 2023-03-10 12:59:37.007186662 +0100
>>>>>>>> @@ -1,2 +1,2 @@
>>>>>>>> -recirc_id(0),in_port(2),eth(macs),eth_type(0x0800),ipv4(frag=no),
>>>>>>>> packets:10, bytes:840, used:0.001s,
>>>>>>>> actions:userspace(pid=1,sFlow(vid=0,pcp=0,output=1),actions),3
>>>>>>>> +recirc_id(0),in_port(2),eth(macs),eth_type(0x0800),ipv4(frag=no),
>>>>>>>> packets:19, bytes:1596, used:0.001s,
>>>>>>>> actions:userspace(pid=1,sFlow(vid=0,pcp=0,output=1),actions),3
>>>>>>>>
>>>>>>>>
>>>>>>>> 4:
>>>>>>>>
>>>>>>>> ./system-offloads-traffic.at:191: check_logs
>>>>>>>> --- /dev/null 2023-03-08 13:28:57.628323765 +0100
>>>>>>>> +++
>>>>>>>> /root/Documents/reviews/ovs_cmi_tc_sflow/OVS_master_DPDK_v22.11/ovs_github/tests/system-offloads-testsuite.dir/at-groups/4/stdout
>>>>>>>> 2023-03-10 13:04:47.167727980 +0100
>>>>>>>> @@ -0,0 +1 @@
>>>>>>>> +2023-03-10T12:04:35.938Z|00001|dpif(handler1)|WARN|system@ovs-system:
>>>>>>>> recv failed (No such file or directory)
>>>>>>>>
>>>>>>>>
>>>>>>>> On Fedora 37:
>>>>>>>>
>>>>>>>> 3: sflow offloads with sampling=1 - ping between two ports -
>>>>>>>> offloads enabled FAILED (system-offloads-traffic.at:133)
>>>>>>>> 4: sflow offloads with sampling=2 - ping between two ports -
>>>>>>>> offloads enabled FAILED (system-offloads-traffic.at:191)
>>>>>>> More tests are failing with your patch applied in offloads, it looks
>>>>>>> like due to the same error message as above:
>>>>>>>
>>>>>>> 1: offloads - ping between two ports - offloads disabled FAILED
>>>>>>> (system-offloads-traffic.at:60)
>>>>>>> 5: offloads - set ingress_policing_rate and ingress_policing_burst
>>>>>>> - offloads disabled FAILED (system-offloads-traffic.at:217)
>>>>>>> 7: offloads - set ingress_policing_kpkts_rate and
>>>>>>> ingress_policing_kpkts_burst - offloads disabled FAILED
>>>>>>> (system-offloads-traffic.at:265)
>>>>>>> 9: offloads - check interface meter offloading - offloads disabled
>>>>>>> FAILED (system-offloads-traffic.at:339)
>>>>>>> 11: offloads - check_pkt_len action - offloads disabled FAILED
>>>>>>> (system-offloads-traffic.at:439)
>>>>>>>
>>>>>>>
>>>>>>> Even worse (almost) all kernel tests (make check-kernel) are failing
>>>>>>> due to this error, which is not a problem with out your patches :(
>>>>>>>
>>>>>>> system-kmod-testsuite: 1 2 3 4 5 6 7 10 11 12 13 14 15 16 17 18 19 20
>>>>>>> 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45
>>>>>>> 46 47 48 49 50 51 52 53 54 56 57 58 59 60 61 62 63 64 65 66 67 68 69 70
>>>>>>> 71 72 73 74 75 76 77 78 79 80 81 82 83 84 87 88 89 90 91 92 93 94 95 96
>>>>>>> 97 98 99 100 101 102 103 104 105 106 107 108 109 110 111 112 113 114
>>>>>>> 115 116 117 118 119 120 121 122 123 124 125 126 133 134 135 136 137 138
>>>>>>> 139 140 141 142 143 144 145 146 147 148 149 150 151 152 153 154 155 156
>>>>>>> 157 158 159 160 161 162 163 164 165 167 168 169 170 171 failed
>>>>>>>
>>>>>>> Can you please run all unit tests before submitting a new revision? Let
>>>>>>> me know what the change will be to fix all of this, so I can
>>>>>>> incorporate it in the review.
>>>>>>>
>>>>>>> //Eelco
>>>>>> I thought github actions would run all the tests. So I didn't run 'make
>>>>>> check-offloads' manually.
>>>>> No, the datapath tests are not run. So before you submit a patch like
>>>>> this you need to run make
>>>>> check/check-kerne/check-offloads/check-system-userspace.
>>>> OK.
>>>>>> I reproduced the "recv failed" error. The following diff can fix it:
>>>>>>
>>>>>> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
>>>>>> index 510ba922d..70ba45f99 100644
>>>>>> --- a/lib/netdev-offload-tc.c
>>>>>> +++ b/lib/netdev-offload-tc.c
>>>>>> @@ -3451,7 +3451,7 @@ offload_recv(struct dpif_upcall *upcall, struct
>>>>>> ofpbuf *buf,
>>>>>> }
>>>>>>
>>>>>> if (!psample_sock) {
>>>>>> - return ENOENT;
>>>>>> + return EAGAIN;
>>>>>> }
>>>>>>
>>>>>> for (;;) {
>>>>>>
>>>>>> Test 3 "sflow offloads with sampling=1" also failed. I changed the test
>>>>>> based on test 4.
>>>>>> Now it sends 1000 packets instead of 10. After this change, the test
>>>>>> result is stable.
>>>>> Can you send me the diff also, I can test/review it as part of v24.
>>>> Sure.
>>>>
>>>> diff --git a/tests/system-offloads-traffic.at
>>>> b/tests/system-offloads-traffic.at
>>>> index e99ea7720..ba9b5d076 100644
>>>> --- a/tests/system-offloads-traffic.at
>>>> +++ b/tests/system-offloads-traffic.at
>>>> @@ -120,30 +120,25 @@ 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-appctl dpctl/dump-flows], [0], [ignore])
>>>>
>>>> -NS_CHECK_EXEC([at_ns0], [ping -c 10 -w 12 10.1.1.2 | FORMAT_PING], [0],
>>>> [dnl
>>>> -10 packets transmitted, 10 received, 0% packet loss, time 0ms
>>>> -])
>>>> AT_CHECK([ovs-vsctl -- --id=@sflow create sflow agent=lo
>>>> target=\"127.0.0.1:$SFLOW_PORT\" header=128 sampling=1 polling=100 -- set
>>>> bridge br0 sflow=@sflow], [0], [ignore])
>>>> -NS_CHECK_EXEC([at_ns0], [ping -c 10 -w 12 10.1.1.2 | FORMAT_PING], [0],
>>>> [dnl
>>>> -10 packets transmitted, 10 received, 0% packet loss, time 0ms
>>>> -])
>>>> +NS_CHECK_EXEC([at_ns0], [ping -c 1000 -i 0.01 -w 12 10.1.1.2], [0],
>>>> [ignore])
>>> This chance should not be needed, if you send 10 pings, or 1000 you should
>>> get 0% packet loss or else something is wrong. In other test cases these
>>> pings work just info.
>> OK, now the new diff is:
>>
>> diff --git a/tests/system-offloads-traffic.at
>> b/tests/system-offloads-traffic.at
>> index e99ea7720..6e8439042 100644
>> --- a/tests/system-offloads-traffic.at
>> +++ b/tests/system-offloads-traffic.at
>> @@ -120,7 +120,7 @@ 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-appctl dpctl/dump-flows], [0], [ignore])
>>
>> -NS_CHECK_EXEC([at_ns0], [ping -c 10 -w 12 10.1.1.2 | FORMAT_PING], [0], [dnl
>> +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-vsctl -- --id=@sflow create sflow agent=lo
>> target=\"127.0.0.1:$SFLOW_PORT\" header=128 sampling=1 polling=100 -- set
>> bridge br0 sflow=@sflow], [0], [ignore])
>> @@ -131,19 +131,18 @@ NS_CHECK_EXEC([at_ns0], [ping -c 10 -w 12 10.1.1.2 |
>> FORMAT_PING], [0], [dnl
>> m4_define([DUMP_SFLOW], [sed -e
>> "s/used:[[0-9]].[[0-9]]*s/used:0.001s/;s/eth(src=[[a-z0-9:]]*,dst=[[a-z0-9:]]*)/eth(macs)/;s/pid=[[0-9]]*/pid=1/;s/output=$P1_IFINDEX/output=1/"])
>> P1_IFINDEX=$(cat /sys/class/net/ovs-p1/ifindex)
>> AT_CHECK([ovs-appctl dpctl/dump-flows type=tc,offloaded | grep "in_port(2)"
>> | grep "eth_type(0x0800)" | grep "actions:userspace" | grep "sFlow" |
>> DUMP_SFLOW], [0], [dnl
>> -recirc_id(0),in_port(2),eth(macs),eth_type(0x0800),ipv4(frag=no),
>> packets:10, bytes:840, used:0.001s,
>> actions:userspace(pid=1,sFlow(vid=0,pcp=0,output=1),actions),3
>> +recirc_id(0),in_port(2),eth(macs),eth_type(0x0800),ipv4(frag=no),
>> packets:19, bytes:1596, used:0.001s,
>> actions:userspace(pid=1,sFlow(vid=0,pcp=0,output=1),actions),3
>> ])
>>
>> m4_define([DUMP_SFLOW], [sed -e
>> "s/used:[[0-9]].[[0-9]]*s/used:0.001s/;s/eth(src=[[a-z0-9:]]*,dst=[[a-z0-9:]]*)/eth(macs)/;s/pid=[[0-9]]*/pid=1/;s/output=$P0_IFINDEX/output=1/"])
>> P0_IFINDEX=$(cat /sys/class/net/ovs-p0/ifindex)
>> AT_CHECK([ovs-appctl dpctl/dump-flows type=tc,offloaded | grep "in_port(3)"
>> | grep "eth_type(0x0800)" | grep "actions:userspace" | grep "sFlow" |
>> DUMP_SFLOW], [0], [dnl
>> -recirc_id(0),in_port(3),eth(macs),eth_type(0x0800),ipv4(frag=no),
>> packets:10, bytes:840, used:0.001s,
>> actions:userspace(pid=1,sFlow(vid=0,pcp=0,output=1),actions),2
>> +recirc_id(0),in_port(3),eth(macs),eth_type(0x0800),ipv4(frag=no),
>> packets:19, bytes:1596, used:0.001s,
>> actions:userspace(pid=1,sFlow(vid=0,pcp=0,output=1),actions),2
>> ])
>>
>> OVS_TRAFFIC_VSWITCHD_STOP
>> OVS_APP_EXIT_AND_WAIT([test-sflow])
>> -count=`cat sflow.log | wc -l`
>> -AT_CHECK([[[[ $count -le 22 && $count -ge 18 ]]]])
>> +AT_CHECK([[[[ `grep -v 33-33-00-00-00 sflow.log | wc -l` -eq 20 ]]]])
> Maybe we should remove above check. The sampled packets include multicast,
> arp and normal ping traffic.
> The datapath flows already sort them out. It seems redundant to check again
> here.
The diff looks fine to me. Not sure which part you want to leave out? We do
need to make sure we have all the sflow samples for the above packet.
I’m done with the full review, I will send out the individual comments soon.
//Eelco
>> AT_CLEANUP
>>
>> AT_SETUP([sflow offloads with sampling=2 - ping between two ports -
>> offloads enabled])
>>>> -m4_define([DUMP_SFLOW], [sed -e
>>>> "s/used:[[0-9]].[[0-9]]*s/used:0.001s/;s/eth(src=[[a-z0-9:]]*,dst=[[a-z0-9:]]*)/eth(macs)/;s/pid=[[0-9]]*/pid=1/;s/output=$P1_IFINDEX/output=1/"])
>>>> P1_IFINDEX=$(cat /sys/class/net/ovs-p1/ifindex)
>>>> +m4_define([DUMP_SFLOW], [sed -e
>>>> "s/packets:[[0-9]]*/packets:1/;s/bytes:[[0-9]]*/bytes:1/;s/used:[[0-9]].[[0-9]]*s/used:0.001s/;s/eth(src=[[a-z0-9:]]*,dst=[[a-z0-9:]]*)/eth(macs)/;s/pid=[[0-9]]*/pid=1/;s/output=$P1_IFINDEX/output=1/"])
>>>> AT_CHECK([ovs-appctl dpctl/dump-flows type=tc,offloaded | grep
>>>> "in_port(2)" | grep "eth_type(0x0800)" | grep "actions:userspace" | grep
>>>> "sFlow" | DUMP_SFLOW], [0], [dnl
>>>> -recirc_id(0),in_port(2),eth(macs),eth_type(0x0800),ipv4(frag=no),
>>>> packets:10, bytes:840, used:0.001s,
>>>> actions:userspace(pid=1,sFlow(vid=0,pcp=0,output=1),actions),3
>>>> +recirc_id(0),in_port(2),eth(macs),eth_type(0x0800),ipv4(frag=no),
>>>> packets:1, bytes:1, used:0.001s,
>>>> actions:userspace(pid=1,sFlow(vid=0,pcp=0,output=1),actions),3
>>> Same here we should keep packet and byte counts in the results.
>>>
>>>> ])
>>>>
>>>> -m4_define([DUMP_SFLOW], [sed -e
>>>> "s/used:[[0-9]].[[0-9]]*s/used:0.001s/;s/eth(src=[[a-z0-9:]]*,dst=[[a-z0-9:]]*)/eth(macs)/;s/pid=[[0-9]]*/pid=1/;s/output=$P0_IFINDEX/output=1/"])
>>>> P0_IFINDEX=$(cat /sys/class/net/ovs-p0/ifindex)
>>>> +m4_define([DUMP_SFLOW], [sed -e
>>>> "s/packets:[[0-9]]*/packets:1/;s/bytes:[[0-9]]*/bytes:1/;s/used:[[0-9]].[[0-9]]*s/used:0.001s/;s/eth(src=[[a-z0-9:]]*,dst=[[a-z0-9:]]*)/eth(macs)/;s/pid=[[0-9]]*/pid=1/;s/output=$P0_IFINDEX/output=1/"])
>>>> AT_CHECK([ovs-appctl dpctl/dump-flows type=tc,offloaded | grep
>>>> "in_port(3)" | grep "eth_type(0x0800)" | grep "actions:userspace" | grep
>>>> "sFlow" | DUMP_SFLOW], [0], [dnl
>>>> -recirc_id(0),in_port(3),eth(macs),eth_type(0x0800),ipv4(frag=no),
>>>> packets:10, bytes:840, used:0.001s,
>>>> actions:userspace(pid=1,sFlow(vid=0,pcp=0,output=1),actions),2
>>>> +recirc_id(0),in_port(3),eth(macs),eth_type(0x0800),ipv4(frag=no),
>>>> packets:1, bytes:1, used:0.001s,
>>>> actions:userspace(pid=1,sFlow(vid=0,pcp=0,output=1),actions),2
>>>> ])
>>>>
>>>> OVS_TRAFFIC_VSWITCHD_STOP
>>>> OVS_APP_EXIT_AND_WAIT([test-sflow])
>>>> count=`cat sflow.log | wc -l`
>>>> -AT_CHECK([[[[ $count -le 22 && $count -ge 18 ]]]])
>>>> +AT_CHECK([[[[ $count -le 2100 && $count -ge 1900 ]]]])
>>>> AT_CLEANUP
>>>>
>>>> AT_SETUP([sflow offloads with sampling=2 - ping between two ports -
>>>> offloads enabled])
>>>>>> Do you want me to submit a new version to continue the review?
>>>>> Hi Chris,
>>>>>
>>>>> I’m halfway through v24, so let’s wait so you can incorporate my other
>>>>> comments. I’ll apply these patches and reply to this email later with any
>>>>> comments.
>>>>>
>>>>> //Eelco
>>>>>
>>>> And there 2 other changes in file lib/netdev-offload-tc.c.
>>>> The first change is to avoid an assertion: "nla->nla_len >= NLA_HDRLEN +
>>>> size") at lib/util.c:89".
>>>> The second one is for Ilya's latest comment.
>>>>
>>>> --- a/lib/netdev-offload-tc.c
>>>> +++ b/lib/netdev-offload-tc.c
>>>> @@ -3425,8 +3655,10 @@ psample_parse_packet(struct offload_psample
>>>> *psample,
>>>> return ENOENT;
>>>> }
>>>>
>>>> - upcall->key = NULL;
>>>> upcall->key_len = 0;
>>>> + upcall->key = NULL;
>>>> + upcall->mru = NULL;
>>>> + upcall->hash = NULL;
>>>> upcall->ufid = sflow->ufid;
>>>> upcall->userdata = sflow->userdata;
>>>> upcall->actions = sflow->actions;
>>>>
>>>> @@ -3475,11 +3707,8 @@ offload_recv(struct dpif_upcall *upcall, struct
>>>> ofpbuf *buf,
>>>> }
>>>>
>>>> error = psample_from_ofpbuf(&psample, buf);
>>>> - if (!error) {
>>>> - return psample_parse_packet(&psample, upcall);
>>>> - } else if (error) {
>>>> - return error;
>>>> - }
>>>> +
>>>> + return error ? error : psample_parse_packet(&psample, upcall);
>>>> }
>>>>
>>>> return EAGAIN;
>> _______________________________________________
>> dev mailing list
>> d...@openvswitch.org
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fmailman%2Flistinfo%2Fovs-dev&data=05%7C01%7Ccmi%40nvidia.com%7Cf36f6fe83ed348602a8508db25cfd3d4%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C638145346792096577%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=wEJ88l2quqwPyqJhGrl83MnnU%2FwTwBp9h2dRcDat630%3D&reserved=0
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
