Re: [ovs-dev] [PATCH v24 3/8] netdev-offload-tc: Introduce group ID management API
On 3/29/2023 7:49 PM, Eelco Chaudron wrote:
On 29 Mar 2023, at 13:41, Chris Mi wrote:
On 3/29/2023 6:03 PM, Eelco Chaudron wrote:
On 23 Mar 2023, at 13:07, Eelco Chaudron wrote:
On 23 Mar 2023, at 12:24, Chris Mi wrote:
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!
I was a bit surprised that you already sent out a v25, I was hoping to first
wrap up the discussion. Anyhow, I’ll ignore the v25 and wait for v26 based on
my comments below.
So after some testing, I realized what was going on here, and we do need those
actions in the offload_sflow structure (getting them from the kernel each time
we need this might add additional delay). However, to give more clarity I would
rename it from actions to userspace_actions. So it's clear what they are.
In parse_sample_action() and parse_userspace_action() I would rename “const
struct nlattr *actions, size_t actions_len” to next_actions, next_actions_len
so it’s more clear what they are (or remaining_actions).
Sorry for the hurry.
Will try to look at the v26 next week.
OK, thanks.
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])
I think we should also add a test that utilizes the “struct flow_tnl *tunnel;”
data in struct offload_sflow.
Will add the following lines in this test:
in_tunnel_hdr="tunnel4_in_protocol=17 tunnel4_in_src=172.31.1.1
tunnel4_in_dst=172.31.1.100"
in_count=`grep "$in_tunnel_hdr" sflow.log | wc -l`
AT_CHECK( $in_count -ge 999 )
Will do some testing, but are you sure this will fill the struct flow_tnl
*tunnel attribute?
Yes, after removing the following lines, this test will fail.
if (sample->tunnel) {
memcpy(>tunnel, sample->tunnel, sizeof flow->tunnel);
}
//eelco
+
+/* 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
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v24 3/8] netdev-offload-tc: Introduce group ID management API
On 29 Mar 2023, at 13:41, Chris Mi wrote: > On 3/29/2023 6:03 PM, Eelco Chaudron wrote: >> >> On 23 Mar 2023, at 13:07, Eelco Chaudron wrote: >> >>> On 23 Mar 2023, at 12:24, Chris Mi wrote: >> >> >> 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! >> I was a bit surprised that you already sent out a v25, I was hoping to first >> wrap up the discussion. Anyhow, I’ll ignore the v25 and wait for v26 based >> on my comments below. >> >> So after some testing, I realized what was going on here, and we do need >> those actions in the offload_sflow structure (getting them from the kernel >> each time we need this might add additional delay). However, to give more >> clarity I would rename it from actions to userspace_actions. So it's clear >> what they are. >> >> In parse_sample_action() and parse_userspace_action() I would rename “const >> struct nlattr *actions, size_t actions_len” to next_actions, >> next_actions_len so it’s more clear what they are (or remaining_actions). > Sorry for the hurry. Will try to look at the v26 next week. > 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]) >> I think we should also add a test that utilizes the “struct flow_tnl >> *tunnel;” data in struct offload_sflow. > Will add the following lines in this test: > > in_tunnel_hdr="tunnel4_in_protocol=17 tunnel4_in_src=172.31.1.1 > tunnel4_in_dst=172.31.1.100" > in_count=`grep "$in_tunnel_hdr" sflow.log | wc -l` > AT_CHECK( $in_count -ge 999 ) Will do some testing, but are you sure this will fill the struct flow_tnl *tunnel attribute? //eelco + +/* 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 >>> >>> >>> ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v24 3/8] netdev-offload-tc: Introduce group ID management API
On 3/29/2023 6:03 PM, Eelco Chaudron wrote: On 23 Mar 2023, at 13:07, Eelco Chaudron wrote: On 23 Mar 2023, at 12:24, Chris Mi wrote: 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! I was a bit surprised that you already sent out a v25, I was hoping to first wrap up the discussion. Anyhow, I’ll ignore the v25 and wait for v26 based on my comments below. So after some testing, I realized what was going on here, and we do need those actions in the offload_sflow structure (getting them from the kernel each time we need this might add additional delay). However, to give more clarity I would rename it from actions to userspace_actions. So it's clear what they are. In parse_sample_action() and parse_userspace_action() I would rename “const struct nlattr *actions, size_t actions_len” to next_actions, next_actions_len so it’s more clear what they are (or remaining_actions). Sorry for the hurry. Done. 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]) I think we should also add a test that utilizes the “struct flow_tnl *tunnel;” data in struct offload_sflow. Will add the following lines in this test: in_tunnel_hdr="tunnel4_in_protocol=17 tunnel4_in_src=172.31.1.1 tunnel4_in_dst=172.31.1.100" in_count=`grep "$in_tunnel_hdr" sflow.log | wc -l` AT_CHECK( $in_count -ge 999 ) + +/* 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 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v24 3/8] netdev-offload-tc: Introduce group ID management API
On 23 Mar 2023, at 13:07, Eelco Chaudron wrote: > On 23 Mar 2023, at 12:24, Chris Mi wrote: 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! I was a bit surprised that you already sent out a v25, I was hoping to first wrap up the discussion. Anyhow, I’ll ignore the v25 and wait for v26 based on my comments below. So after some testing, I realized what was going on here, and we do need those actions in the offload_sflow structure (getting them from the kernel each time we need this might add additional delay). However, to give more clarity I would rename it from actions to userspace_actions. So it's clear what they are. In parse_sample_action() and parse_userspace_action() I would rename “const struct nlattr *actions, size_t actions_len” to next_actions, next_actions_len so it’s more clear what they are (or remaining_actions). >>> 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]) I think we should also add a test that utilizes the “struct flow_tnl *tunnel;” data in struct offload_sflow. >> + >> +/* 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 > > > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v24 3/8] netdev-offload-tc: Introduce group ID management API
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
Reviewed-by: Roi Dayan
---
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
Re: [ovs-dev] [PATCH v24 3/8] netdev-offload-tc: Introduce group ID management API
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
Reviewed-by: Roi Dayan
---
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
Re: [ovs-dev] [PATCH v24 3/8] netdev-offload-tc: Introduce group ID management API
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
> Reviewed-by: Roi Dayan
> ---
> 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
Re: [ovs-dev] [PATCH v24 3/8] netdev-offload-tc: Introduce group ID management API
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
Reviewed-by: Roi Dayan
---
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
&&
Re: [ovs-dev] [PATCH v24 3/8] netdev-offload-tc: Introduce group ID management API
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
Reviewed-by: Roi Dayan
---
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
&&
Re: [ovs-dev] [PATCH v24 3/8] netdev-offload-tc: Introduce group ID management API
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
>>> Reviewed-by: Roi Dayan
>>> ---
>>> 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
Re: [ovs-dev] [PATCH v24 3/8] netdev-offload-tc: Introduce group ID management API
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
Reviewed-by: Roi Dayan
---
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
Re: [ovs-dev] [PATCH v24 3/8] netdev-offload-tc: Introduce group ID management API
On 3/22/2023 7:10 PM, Eelco Chaudron wrote:
On 22 Mar 2023, at 11:45, 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
Reviewed-by: Roi Dayan
---
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 =
Re: [ovs-dev] [PATCH v24 3/8] netdev-offload-tc: Introduce group ID management API
On 22 Mar 2023, at 11:45, 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
>> Reviewed-by: Roi Dayan
>> ---
>>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
Re: [ovs-dev] [PATCH v24 3/8] netdev-offload-tc: Introduce group ID management API
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
> Reviewed-by: Roi Dayan
> ---
>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
Re: [ovs-dev] [PATCH v24 3/8] netdev-offload-tc: Introduce group ID management API
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
Reviewed-by: Roi Dayan
---
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.
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 needs https://github.com/sflow/sflowtool. Usually I run sflowtool
manually to verify it.
Hopefully this test will not block the following reviews.
+struct flow_tnl *tunnel; /* Input tunnel. */
+ovs_u128 ufid; /* Flow ufid. */
+};
+
+/* This maps a psample group ID to struct offload_sflow. */
+struct sgid_node {
+struct cmap_node metadata_node;
+struct cmap_node id_node;
+struct ovs_refcount refcount;
+uint32_t hash;
+uint32_t id;
+struct offload_sflow sflow;
+};
+
+static struct ovs_mutex sgid_lock = OVS_MUTEX_INITIALIZER;
+static struct cmap sgid_map = CMAP_INITIALIZER;
+static struct cmap sgid_metadata_map = CMAP_INITIALIZER;
+static struct id_pool *sample_group_ids OVS_GUARDED_BY(sgid_lock);
+
+static void
+sgid_node_free(struct sgid_node *node)
+{
+if (node) {
+if (node->sflow.tunnel) {
Re: [ovs-dev] [PATCH v24 3/8] netdev-offload-tc: Introduce group ID management API
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
>>> Reviewed-by: Roi Dayan
>>> ---
>>> 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.
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…
>>
>>> +struct flow_tnl *tunnel; /* Input tunnel. */
>>> +ovs_u128 ufid; /* Flow ufid. */
>>> +};
>>> +
>>> +/* This maps a psample group ID to struct offload_sflow. */
>>> +struct sgid_node {
>>> +struct cmap_node metadata_node;
>>> +struct cmap_node id_node;
>>> +struct ovs_refcount refcount;
>>> +uint32_t hash;
>>> +uint32_t id;
>>> +struct offload_sflow sflow;
>>> +};
>>> +
>>> +static struct ovs_mutex sgid_lock = OVS_MUTEX_INITIALIZER;
>>> +static struct cmap sgid_map = CMAP_INITIALIZER;
>>> +static struct cmap sgid_metadata_map = CMAP_INITIALIZER;
>>> +static struct id_pool *sample_group_ids OVS_GUARDED_BY(sgid_lock);
>>> +
>>> +static void
>>> +sgid_node_free(struct sgid_node *node)
>>> +{
>>> +if (node) {
>>> +if (node->sflow.tunnel) {
>> This additional null check (and below) is not needed as free() will do this
>> internally.
> OK.
>>
>>> +free(node->sflow.tunnel);
>>> +}
>>> +if (node->sflow.action) {
>>> +free(node->sflow.action);
>>> +}
>>> +if (node->sflow.actions) {
>>> +free(node->sflow.actions);
>>> +}
>>> +if (node->sflow.userdata) {
>>> +free(node->sflow.userdata);
>>> +}
>>> +free(node);
>>> +}
>>> +}
>>> +
>>> +static struct sgid_node *
>>> +sgid_find(uint32_t id)
>>> +{
>>> +const struct cmap_node *node = cmap_find(_map, id);
>>> +
>>> +return node ? CONTAINER_OF(node, struct sgid_node, id_node) : NULL;
>>> +}
>>> +
>>> +static uint32_t
>>> +offload_sflow_hash(const struct offload_sflow *sflow)
>>> +{
>>> +return
Re: [ovs-dev] [PATCH v24 3/8] netdev-offload-tc: Introduce group ID management API
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
Reviewed-by: Roi Dayan
---
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'.
+struct flow_tnl *tunnel; /* Input tunnel. */
+ovs_u128 ufid; /* Flow ufid. */
+};
+
+/* This maps a psample group ID to struct offload_sflow. */
+struct sgid_node {
+struct cmap_node metadata_node;
+struct cmap_node id_node;
+struct ovs_refcount refcount;
+uint32_t hash;
+uint32_t id;
+struct offload_sflow sflow;
+};
+
+static struct ovs_mutex sgid_lock = OVS_MUTEX_INITIALIZER;
+static struct cmap sgid_map = CMAP_INITIALIZER;
+static struct cmap sgid_metadata_map = CMAP_INITIALIZER;
+static struct id_pool *sample_group_ids OVS_GUARDED_BY(sgid_lock);
+
+static void
+sgid_node_free(struct sgid_node *node)
+{
+if (node) {
+if (node->sflow.tunnel) {
This additional null check (and below) is not needed as free() will do this
internally.
OK.
+free(node->sflow.tunnel);
+}
+if (node->sflow.action) {
+free(node->sflow.action);
+}
+if (node->sflow.actions) {
+free(node->sflow.actions);
+}
+if (node->sflow.userdata) {
+free(node->sflow.userdata);
+}
+free(node);
+}
+}
+
+static struct sgid_node *
+sgid_find(uint32_t id)
+{
+const struct cmap_node *node = cmap_find(_map, id);
+
+return node ? CONTAINER_OF(node, struct sgid_node, id_node) : NULL;
+}
+
+static uint32_t
+offload_sflow_hash(const struct offload_sflow *sflow)
+{
+return hash_bytes(>ufid, sizeof sflow->ufid, 0);
+}
+static bool
+offload_sflow_equal(const struct offload_sflow *a,
+const struct offload_sflow *b)
+{
+if (!ovs_u128_equals(a->ufid, b->ufid)) {
+return false;
+}
+/* Action can't be NULL. */
+if (!a->action || !b->action) {
+return false;
+}
+if (a->action->nla_len != b->action->nla_len
+|| memcmp(a->action, b->action, a->action->nla_len)) {
+return false;
+}
+/* Actions is used to get output tunnel. It can be NULL. */
+if ((a->actions && !b->actions) || (!a->actions && b->actions)) {
+return false;
+}
+if ((a->actions && b->actions &&
+a->actions->nla_len != b->actions->nla_len)
+|| memcmp(a->actions, b->actions, a->actions->nla_len)) {
In theory, a->actions and b->actions can be NULL in this case, if
Re: [ovs-dev] [PATCH v24 3/8] netdev-offload-tc: Introduce group ID management API
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
> Reviewed-by: Roi Dayan
> ---
> 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?
We might need an offload_type field.
> +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.
> +struct flow_tnl *tunnel; /* Input tunnel. */
> +ovs_u128 ufid; /* Flow ufid. */
> +};
> +
> +/* This maps a psample group ID to struct offload_sflow. */
> +struct sgid_node {
> +struct cmap_node metadata_node;
> +struct cmap_node id_node;
> +struct ovs_refcount refcount;
> +uint32_t hash;
> +uint32_t id;
> +struct offload_sflow sflow;
> +};
> +
> +static struct ovs_mutex sgid_lock = OVS_MUTEX_INITIALIZER;
> +static struct cmap sgid_map = CMAP_INITIALIZER;
> +static struct cmap sgid_metadata_map = CMAP_INITIALIZER;
> +static struct id_pool *sample_group_ids OVS_GUARDED_BY(sgid_lock);
> +
> +static void
> +sgid_node_free(struct sgid_node *node)
> +{
> +if (node) {
> +if (node->sflow.tunnel) {
This additional null check (and below) is not needed as free() will do this
internally.
> +free(node->sflow.tunnel);
> +}
> +if (node->sflow.action) {
> +free(node->sflow.action);
> +}
> +if (node->sflow.actions) {
> +free(node->sflow.actions);
> +}
> +if (node->sflow.userdata) {
> +free(node->sflow.userdata);
> +}
> +free(node);
> +}
> +}
> +
> +static struct sgid_node *
> +sgid_find(uint32_t id)
> +{
> +const struct cmap_node *node = cmap_find(_map, id);
> +
> +return node ? CONTAINER_OF(node, struct sgid_node, id_node) : NULL;
> +}
> +
> +static uint32_t
> +offload_sflow_hash(const struct offload_sflow *sflow)
> +{
> +return hash_bytes(>ufid, sizeof sflow->ufid, 0);
> +}
> +static bool
> +offload_sflow_equal(const struct offload_sflow *a,
> +const struct offload_sflow *b)
> +{
> +if (!ovs_u128_equals(a->ufid, b->ufid)) {
> +return false;
> +}
> +/* Action can't be NULL. */
> +if (!a->action || !b->action) {
> +return false;
> +}
> +if (a->action->nla_len != b->action->nla_len
> +|| memcmp(a->action, b->action, a->action->nla_len)) {
> +return false;
> +}
> +/* Actions is used to get output tunnel. It can be NULL. */
> +if ((a->actions && !b->actions) || (!a->actions && b->actions)) {
> +return false;
> +}
> +if ((a->actions && b->actions &&
> +a->actions->nla_len != b->actions->nla_len)
> +|| memcmp(a->actions, b->actions, a->actions->nla_len)) {
In theory, a->actions and b->actions can be NULL in this case, if
a->actions->nla_len != 0 for some odd reason, it will lead to a crash.
> +return false;
> +}
> +/* Tunnel is used to get input tunnel. It can be NULL. */
> +if (!a->tunnel && !b->tunnel) {
> +return true;
> +}
> +if (!a->tunnel || !b->tunnel) {
> +return false;
> +}
> +return !memcmp(a->tunnel, b->tunnel, sizeof *a->tunnel);
> +}
> +
> +static struct
[ovs-dev] [PATCH v24 3/8] netdev-offload-tc: Introduce group ID management API
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.
Signed-off-by: Chris Mi
Reviewed-by: Roi Dayan
---
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 {
+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. */
+struct flow_tnl *tunnel; /* Input tunnel. */
+ovs_u128 ufid; /* Flow ufid. */
+};
+
+/* This maps a psample group ID to struct offload_sflow. */
+struct sgid_node {
+struct cmap_node metadata_node;
+struct cmap_node id_node;
+struct ovs_refcount refcount;
+uint32_t hash;
+uint32_t id;
+struct offload_sflow sflow;
+};
+
+static struct ovs_mutex sgid_lock = OVS_MUTEX_INITIALIZER;
+static struct cmap sgid_map = CMAP_INITIALIZER;
+static struct cmap sgid_metadata_map = CMAP_INITIALIZER;
+static struct id_pool *sample_group_ids OVS_GUARDED_BY(sgid_lock);
+
+static void
+sgid_node_free(struct sgid_node *node)
+{
+if (node) {
+if (node->sflow.tunnel) {
+free(node->sflow.tunnel);
+}
+if (node->sflow.action) {
+free(node->sflow.action);
+}
+if (node->sflow.actions) {
+free(node->sflow.actions);
+}
+if (node->sflow.userdata) {
+free(node->sflow.userdata);
+}
+free(node);
+}
+}
+
+static struct sgid_node *
+sgid_find(uint32_t id)
+{
+const struct cmap_node *node = cmap_find(_map, id);
+
+return node ? CONTAINER_OF(node, struct sgid_node, id_node) : NULL;
+}
+
+static uint32_t
+offload_sflow_hash(const struct offload_sflow *sflow)
+{
+return hash_bytes(>ufid, sizeof sflow->ufid, 0);
+}
+
+static bool
+offload_sflow_equal(const struct offload_sflow *a,
+const struct offload_sflow *b)
+{
+if (!ovs_u128_equals(a->ufid, b->ufid)) {
+return false;
+}
+/* Action can't be NULL. */
+if (!a->action || !b->action) {
+return false;
+}
+if (a->action->nla_len != b->action->nla_len
+|| memcmp(a->action, b->action, a->action->nla_len)) {
+return false;
+}
+/* Actions is used to get output tunnel. It can be NULL. */
+if ((a->actions && !b->actions) || (!a->actions && b->actions)) {
+return false;
+}
+if ((a->actions && b->actions &&
+a->actions->nla_len != b->actions->nla_len)
+|| memcmp(a->actions, b->actions, a->actions->nla_len)) {
+return false;
+}
+/* Tunnel is used to get input tunnel. It can be NULL. */
+if (!a->tunnel && !b->tunnel) {
+return true;
+}
+if (!a->tunnel || !b->tunnel) {
+return false;
+}
+return !memcmp(a->tunnel, b->tunnel, sizeof *a->tunnel);
+}
+
+static struct sgid_node *
+sgid_find_equal(const struct offload_sflow *target, uint32_t hash)
+{
+struct sgid_node *node;
+
+CMAP_FOR_EACH_WITH_HASH (node, metadata_node, hash, _metadata_map) {
+if (offload_sflow_equal(>sflow, target)) {
+return node;
+}
+}
+return NULL;
+}
+
+static struct sgid_node *
+sgid_ref_equal(const struct offload_sflow *target, uint32_t hash)
+{
+struct sgid_node *node;
+
+do {
+node = sgid_find_equal(target, hash);
+/* Try again if the node was released before we get the reference. */
+} while (node && !ovs_refcount_try_ref_rcu(>refcount));
+
+return node;
+}
+
+static void
+offload_sflow_clone(struct offload_sflow *new, const struct offload_sflow *old)
+{
+new->action = xmemdup(old->action, old->action->nla_len);
+new->actions = old->actions
+ ? xmemdup(old->actions,
