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. //EelcoSigned-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.Some additional information, the kernel passes on OVS_USERSPACE_ATTR_ACTIONS to upcall.actions. https://elixir.bootlin.com/linux/latest/source/net/openvswitch/actions.c#L988
OK, I'll create a test case for it.
+ +/* Allocate a unique group id for the given set of flow metadata and + * optional actions. + */So assuming the ufid is part of the offload_sflow data and we only support a single sample per flow we will probably not re-use the metadata. So I guess this only makes sense if we support multiple offloads per flow in the future am I right?Actually, ufid was introduced just for easy hashing and comparing. I didn't noticed that with this change, metadata can't be reused. Without ufid, arp and ip metedata can be reused, for example: recirc_id(0),in_port(enp8s0f0_1),eth(src=02:25:d0:65:01:02,dst=24:25:d0:e1:00:00),eth_type(0x0800),ipv4(tos=0,frag=no), packets:16, bytes:2330, used:0.110s, actions:userspace(pid=4294967295,sFlow(vid=0,pcp=0,output=2429),actions),set(tunnel(tun_id=0x4,dst=192.168.1.66,ttl=64,tp_dst=4789,flags(key))),vxlan_sys_4789 recirc_id(0),in_port(enp8s0f0_1),eth(src=02:25:d0:65:01:02,dst=24:25:d0:e1:00:00),eth_type(0x0806), packets:0, bytes:0, used:never, actions:userspace(pid=4294967295,sFlow(vid=0,pcp=0,output=2429),actions),set(tunnel(tun_id=0x4,dst=192.168.1.66,ttl=64,tp_dst=4789,flags(key))),vxlan_sys_4789 Maybe we should remove ufid to re-use meterdata. Or maybe keep it. So there is a 1:1 mapping for sample group id and ufid. What do you think, Eelco?I’m ok with both designs if you use the 1:1 mapping you can remove the ref counting, if you remove it you need to fix the above refcount comment.OK, I'll use 1:1 mapping.ACK <SNIP>
_______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
