On 3/23/2023 8:07 PM, Eelco Chaudron wrote:
On 23 Mar 2023, at 12:24, Chris Mi wrote:
On 3/23/2023 5:47 PM, Eelco Chaudron wrote:
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?
Yes. Only the first sampled packet has tunnel header without passing actions.
And without offload. This test also passes.
ACK, ok will do some investigation next week, and let you know!
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.
That's not duplicate packets. arping sends 5 packets. So we receive 5 sampled
packets. It seems it doesn't resend.
Do we need a remote vxlan port? Is there any existing test to do that?
I want to make sure we receive exactly five packets on the other side of the
tunnel.
I guess there is this one:
AT_SETUP([datapath - ping over vxlan tunnel])
New test is done:
AT_SETUP([offloads - ping over vxlan tunnel with sflow - offloads enabled])
OVS_CHECK_TUNNEL_TSO()
OVS_CHECK_VXLAN()
OVS_TRAFFIC_VSWITCHD_START([], [], [-- set Open_vSwitch .
other_config:hw-offload=true])
ADD_BR([br-underlay])
AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"])
AT_CHECK([ovs-ofctl add-flow br-underlay "actions=normal"])
ADD_NAMESPACES(at_ns0)
dnl Set up underlay link from host into the namespace using veth pair.
ADD_VETH(p0, at_ns0, br-underlay, "172.31.1.1/24")
AT_CHECK([ip addr add dev br-underlay "172.31.1.100/24"])
AT_CHECK([ip link set dev br-underlay up])
dnl Set up tunnel endpoints on OVS outside the namespace and with a native
dnl linux device inside the namespace.
ADD_OVS_TUNNEL([vxlan], [br0], [at_vxlan0], [172.31.1.1], [10.1.1.100/24])
ADD_NATIVE_TUNNEL([vxlan], [at_vxlan1], [at_ns0], [172.31.1.100],
[10.1.1.1/24],
[id 0 dstport 4789])
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-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])
dnl First, check the underlay
NS_CHECK_EXEC([at_ns0], [ping -q -c 3 -i 0.3 -w 2 172.31.1.100 |
FORMAT_PING], [0], [dnl
3 packets transmitted, 3 received, 0% packet loss, time 0ms
])
dnl Okay, now check the overlay with different packet sizes
NS_CHECK_EXEC([at_ns0], [ping -q -c 1000 -i 0.01 10.1.1.100 |
FORMAT_PING], [0], [dnl
1000 packets transmitted, 1000 received, 0% packet loss, time 0ms
])
dnl Vni is 0, test-sflow doesn't show it
tunnel_hdr="tunnel4_out_protocol=17 tunnel4_out_src=0.0.0.0
tunnel4_out_dst=172.31.1.1 tunnel4_out_src_port=0
tunnel4_out_dst_port=46354"
count=`grep "$tunnel_hdr" sflow.log | wc -l`
AT_CHECK([[[[ $count -ge 999 ]]]])
OVS_TRAFFIC_VSWITCHD_STOP
OVS_APP_EXIT_AND_WAIT([test-sflow])
AT_CLEANUP
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
