On 23 Mar 2023, at 10:28, Chris Mi wrote:
> On 3/22/2023 6:45 PM, Eelco Chaudron wrote:
>>
>> On 22 Mar 2023, at 7:15, Chris Mi wrote:
>>
>>> On 3/20/2023 6:04 PM, Eelco Chaudron wrote:
>>>> On 20 Mar 2023, at 6:44, Chris Mi wrote:
>>>>
>>>>> On 3/16/2023 5:09 PM, Eelco Chaudron wrote:
>>>>>> On 1 Mar 2023, at 8:22, Chris Mi wrote:
>>>>>>
>>>>>>> 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.
>>>>>> See some comments inline below.
>>>>>>
>>>>>> //Eelco
>>>>>>
>>>>>>> Signed-off-by: Chris Mi<c...@nvidia.com>
>>>>>>> Reviewed-by: Roi Dayan<r...@nvidia.com>
>>>>>>> ---
>>>>>>> lib/netdev-offload-tc.c | 233
>>>>>>> +++++++++++++++++++++++++++++++++++++++-
>>>>>>> lib/tc.h | 1 +
>>>>>>> 2 files changed, 232 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
>>>>>>> index 4fb9d9f21..4214337d8 100644
>>>>>>> --- a/lib/netdev-offload-tc.c
>>>>>>> +++ b/lib/netdev-offload-tc.c
>>>>>>> @@ -41,6 +41,7 @@
>>>>>>> #include "unaligned.h"
>>>>>>> #include "util.h"
>>>>>>> #include "dpif-provider.h"
>>>>>>> +#include "cmap.h"
>>>>>>>
>>>>>>> VLOG_DEFINE_THIS_MODULE(netdev_offload_tc);
>>>>>>>
>>>>>>> @@ -103,6 +104,226 @@ static void parse_tc_flower_to_stats(struct
>>>>>>> tc_flower *flower,
>>>>>>> static int get_ufid_adjust_stats(const ovs_u128 *ufid,
>>>>>>> struct dpif_flow_stats *stats);
>>>>>>>
>>>>>>> +/* 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.
>>>>>>> + */
>>>>>>> +struct offload_sflow {
>>>>>> Should we maybe already give this a more general name, like
>>>>>> offload_sample?
>>>>>> This as you need the same structure later when you add support of
>>>>>> offload sent to a controller?
>>>>> OK.
>>>>>> We might need an offload_type field.
>>>>> OK.
>>>>>>> + struct nlattr *action; /* SFlow action. Used in flow_get. */
>>>>>>> + struct nlattr *userdata; /* Struct user_action_cookie. */
>>>>>>> + struct nlattr *actions; /* All actions to get output tunnel. */
>>>>>> You should not need actions at all, see comments later. These are the
>>>>>> next actions, which are not of interest to sflow offload.
>>>>> As the comment describes, we need it to get the output tunnel. Otherwise,
>>>>> we'll lose the following info using sflowtool after offloading:
>>>>>
>>>>> extendedType out_VNI
>>>>> out_VNI 4
>>>>> flowBlock_tag 0:1023
>>>>> flowSampleType tunnel_ipv4_out_IPV4
>>>>> tunnel_ipv4_out_sampledPacketSize 0
>>>>> tunnel_ipv4_out_IPSize 0
>>>>> tunnel_ipv4_out_srcIP 0.0.0.0
>>>>> tunnel_ipv4_out_dstIP 192.168.1.66
>>>>> tunnel_ipv4_out_IPProtocol 17
>>>>> tunnel_ipv4_out_IPTOS 0
>>>>> tunnel_ipv4_out_UDPSrcPort 0
>>>>> tunnel_ipv4_out_UDPDstPort 46354
>>>>>
>>>>> I don't think we can get the output tunnel info without mapping 'actions'.
>>>> I’m confused as from the code it looks like actions are the outer actions,
>>>> and your actions should come from the inner actions, i.e., they should be
>>>> part of action.
>>> In my understanding, 'actions' is all actions, for example:
>>> "actions:userspace(pid=4294967295,sFlow(vid=0,pcp=0,output=935),actions),set(tunnel(tun_id=0x4,dst=192.168.1.66,ttl=64,tp_dst=4789,flags(df|key))),vxlan_sys_4789"
>>> Not sure what do you mean by outer actions and inner actions.
>>
>> actions: [userspace(pid=4294967295,sFlow(vid=0,pcp=0,output=935),actions)],
>> [set(tunnel(tun_id=0x4,dst=192.168.1.66,ttl=64,tp_dst=4789,flags(df|key))),vxlan_sys_4789]
>> +----------------------------------------------------------------
>> ------------------------------------------------------------------------------------------
>> |
>> \ These are the remaining (outer) actions and the sflow implementation
>> should not care about this at all.
>> \ These are the sflow actions and this is all what sflow should
>> care about!
>>
>>
>> So sflow should only use the sFlow actions, and the remaining (outer)
>> actions should not be touched by sFlow these should be implemented/using the
>> existing TC offloads.
>> If you supply these actions to the upcall they are also executed (again) in
>> userspace, which they should not.
>>
>>>> Maybe I’m not understanding this correctly, and as this is a more complex
>>>> case, we should have a test case fore it.
>>>>
>>>> If you can share the test case before the next revision of the patchset, I
>>>> can revisit it to safe another version…
>>> Our QA reported this bug using their test. It's written in python. I'm
>>> afraid it doesn't help even if I can share it.
>>> They said that sflowtool shows different results between hw-offload="true"
>>> and non-offload.
>>>
>>> I read the code and found this:
>>> dpif_sflow_received()
>>> {
>>> ...
>>> /* Output tunnel. */
>>> if (sflow_actions
>>> && sflow_actions->encap_depth == 1
>>> && !sflow_actions->tunnel_err
>>> && dpif_sflow_cookie_num_outputs(cookie) == 1) {
>>> tnlOutProto = sflow_actions->tunnel_ipproto;
>>> if (tnlOutProto == 0) {
>>> ..
>>>
>>> After mapping the 'actions', the output tunnel appeared. I'm not sure if we
>>> can create such complex test using m4.
>>> And it needshttps://github.com/sflow/sflowtool. Usually I run sflowtool
>>> manually to verify it.
>>> Hopefully this test will not block the following reviews.
>> I think we need to resolve this before you sent out the next revision, so we
>> don’t end up with more revisions.
>>
>> There are already some tunnel cases for normal SFLOW which you could use as
>> a base for your tests.
>>
>> ./ofproto-dpif.at:7523:AT_SETUP([ofproto-dpif - sFlow packet sampling -
>> tunnel set])
>> ./ofproto-dpif.at:7592:AT_SETUP([ofproto-dpif - sFlow packet sampling -
>> tunnel push])
>> ./ofproto-dpif.at:7694:AT_SETUP([ofproto-dpif - sFlow packet sampling -
>> MPLS])
>>
>> Please create a test case so I understand what’s going on, as for now, I do
>> not see it from just reading code.
> Hi Eelco,
>
> How about this test case?
>
> AT_SETUP([offloads - sflow with tunnel set - offloads enabled])
> OVS_TRAFFIC_VSWITCHD_START([], [], [-- set Open_vSwitch .
> other_config:hw-offload=true])
>
> on_exit 'kill `cat test-sflow.pid`'
> AT_CHECK([ovstest test-sflow --log-file --detach --no-chdir --pidfile
> 0:127.0.0.1 > sflow.log], [0], [], [ignore])
> AT_CAPTURE_FILE([sflow.log])
> PARSE_LISTENING_PORT([test-sflow.log], [SFLOW_PORT])
>
> AT_CHECK([ovs-appctl -t ovsdb-server exit])
> AT_CHECK([ovs-appctl -t ovs-vswitchd exit])
> AT_CHECK([rm -f ovsdb-server.pid])
> AT_CHECK([rm -f ovs-vswitchd.pid])
> AT_CHECK([ovsdb-server --detach --no-chdir --pidfile --log-file
> --remote=punix:$OVS_RUNDIR/db.sock], [0], [], [stderr])
> AT_CHECK([ovs-vswitchd --detach --no-chdir --pidfile --log-file -vvconn
> -vofproto_dpif -vunixctl], [0], [], [stderr])
I forgot to ask in previous reviews, but why do you need to restart the
process? Can you not enable the logging with the ovs-appctl commands?
> on_exit "kill `cat ovsdb-server.pid`"
> on_exit "kill_ovs_vswitchd `cat ovs-vswitchd.pid`"
>
> AT_CHECK([ovs-vsctl add-port br0 vxlan0 -- set Interface vxlan0 type=vxlan \
> options:remote_ip=192.168.1.1 options:key=4
> options:dst_port=4789 ofport_request=3])
> AT_CHECK([ovs-ofctl add-flow br0 "actions=3"])
> ADD_NAMESPACES(at_ns0, at_ns1)
> ADD_VETH(p0, at_ns0, br0, "1.1.1.1/16")
> mac=$(ip netns exec at_ns0 cat /sys/class/net/p0/address | sed 's/:/-/g')
> hdr="FF-FF-FF-FF-FF-FF-$mac-08-06"
> tunnel_hdr="tunnel4_out_length=0 tunnel4_out_protocol=17
> tunnel4_out_src=0.0.0.0 tunnel4_out_dst=192.168.1.1 tunnel4_out_src_port=0
> tunnel4_out_dst_port=46354 tunnel4_out_tcp_flags=0 tunnel4_out_tos=0
> tunnel_out_vni=4"
> 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], [arping -c 5 1.1.1.2], [1], [ignore])
>
> OVS_TRAFFIC_VSWITCHD_STOP
> OVS_APP_EXIT_AND_WAIT([test-sflow])
> count=`cat sflow.log | grep -i $hdr | grep "$tunnel_hdr" | wc -l`
> AT_CHECK([[[[ $count -eq 5 ]]]])
> AT_CLEANUP
>
> I ran it 100 times. All passed.
Visually inspecting this, it looks fine, however, is this failing without
passing the actions parts?
Also, I think for the final version I would make sure a remote VXLAN port
exists, so you can send actual traffic and verify you do not receive duplicate
packets.
>>>>>>> +
>>>>>>> +/* Allocate a unique group id for the given set of flow metadata and
>>>>>>> + * optional actions.
>>>>>>> + */
>>>>>> So assuming the ufid is part of the offload_sflow data and we only
>>>>>> support a single sample per flow we will probably not re-use the
>>>>>> metadata. So I guess this only makes sense if we support multiple
>>>>>> offloads per flow in the future am I right?
>>>>> Actually, ufid was introduced just for easy hashing and comparing. I
>>>>> didn't noticed that with this change,
>>>>> metadata can't be reused. Without ufid, arp and ip metedata can be
>>>>> reused, for example:
>>>>>
>>>>> recirc_id(0),in_port(enp8s0f0_1),eth(src=02:25:d0:65:01:02,dst=24:25:d0:e1:00:00),eth_type(0x0800),ipv4(tos=0,frag=no),
>>>>> packets:16, bytes:2330, used:0.110s,
>>>>> actions:userspace(pid=4294967295,sFlow(vid=0,pcp=0,output=2429),actions),set(tunnel(tun_id=0x4,dst=192.168.1.66,ttl=64,tp_dst=4789,flags(key))),vxlan_sys_4789
>>>>> recirc_id(0),in_port(enp8s0f0_1),eth(src=02:25:d0:65:01:02,dst=24:25:d0:e1:00:00),eth_type(0x0806),
>>>>> packets:0, bytes:0, used:never,
>>>>> actions:userspace(pid=4294967295,sFlow(vid=0,pcp=0,output=2429),actions),set(tunnel(tun_id=0x4,dst=192.168.1.66,ttl=64,tp_dst=4789,flags(key))),vxlan_sys_4789
>>>>>
>>>>> Maybe we should remove ufid to re-use meterdata. Or maybe keep it. So
>>>>> there is a 1:1 mapping for sample group id and ufid.
>>>>> What do you think, Eelco?
>>>> I’m ok with both designs if you use the 1:1 mapping you can remove the ref
>>>> counting, if you remove it you need to fix the above refcount comment.
>>> OK, I'll use 1:1 mapping.
>> ACK
>>
>> <SNIP>
>>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
