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<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 needs https://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
>>>>>> +
>>>>>> +/* 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
