[ovs-dev] [PATCH v15 0/7] Add offload support for sFlow

[Eelco Chaudron]

On 1 Oct 2021, at 11:35, Eelco Chaudron wrote:

> Hi Chris,
>
> I just started to review this patchset, but as some of the v14 discussions have not finished, I’ll copy them over to v15. This way, all the open items are contained in a single thread/revision. I would suggest clear up the open items before you send a new revision.
>
> This is only a visual review, as I did not test the code, assuming we need a new revision anyway with the test cases integrated.
>
> Cheers,
>
> Eelco


Forgot about this issue (see below), was this fixed in v15? I did not see any reply to the email chain?


> Here is some debug info, it seems related to an update to an existing handle, which does not update the tc part:
>
>  Here the flow gets added:
>
>  2021-09-09T14:01:12.278Z|00001|netdev_offload_tc(handler1)|ERR|EC_DBG: Enter b13915aa-c7a8-4574-8dcf-439f03929b8e.
>
>  2021-09-09T14:01:12.278Z|00002|netdev_offload_tc(handler1)|ERR|EC_DEBUG: port -2147483646
>
>  2021-09-09T14:01:12.285Z|00003|netdev_offload_tc(handler1)|ERR|EC_DBG: EXIT ok b13915aa-c7a8-4574-8dcf-439f03929b8e.
>
>  Here the revalidator updates it with new port information:
>
>  2021-09-09T14:01:12.537Z|00001|netdev_offload_tc(revalidator13)|ERR|EC_DBG: Enter b13915aa-c7a8-4574-8dcf-439f03929b8e.
>
>  2021-09-09T14:01:12.537Z|00002|netdev_offload_tc(revalidator13)|ERR|EC_DEBUG: port 3
>
>  2021-09-09T14:01:12.537Z|00004|netdev_offload_tc(revalidator13)|DBG|updating old handle: 1 prio: 2
>
>  2021-09-09T14:01:12.553Z|00005|netdev_offload_tc(revalidator13)|ERR|EC_DBG: EXIT ok b13915aa-c7a8-4574-8dcf-439f03929b8e.
>
>  Here is a dp dump showing the wrong/old port:
>
>  $ ovs-appctl dpctl/dump-flows -m system at ovs-system type=tc
>
>  ufid:b13915aa-c7a8-4574-8dcf-439f03929b8e, skb_priority(0/0),skb_mark(0/0),ct_state(0/0),ct_zone(0/0),ct_mark(0/0),ct_label(0/0),recirc_id(0),dp_hash(0/0),in_port(vnet4),packet_type(ns=0/0,id=0/0),eth(src=52:54:00:88:51:38,dst=04:f4:bc:28:57:01),eth_type(0x0800),ipv4(src=0.0.0.0/0.0.0.0,dst=0.0.0.0/0.0.0.0,proto=0/0,tos=0/0,ttl=0/0,frag=no), packets:180, bytes:15120, used:0.230s, dp:tc, actions:sample(sample=10.0%,actions(userspace(pid=2771969963,sFlow(vid=0,pcp=0,output=2147483650),actions))),enp5s0f0
>
>  Guess this is the area you need to look at del_filter_and_ufid_mapping():
>
>  2389     if (get_ufid_tc_mapping(ufid, &id) == 0) {
>
>  2390         VLOG_DBG_RL(&rl, "updating old handle: %d prio: %d",
>
>  2391                     id.handle, id.prio);
>
>  2392         info->tc_modify_flow_deleted = !del_filter_and_ufid_mapping(&id, ufid);
>
>  2393     }
>
>  There might also be a problem in the sgid mapping as it’s also using the uuid as the hash, and there could be a collision during the update phase.
>
>  I’m working on some escalations so, please take a look and see if you can fix this.
>
>  I need to restart OVS, and then send a ping, and I see the problem once, I need to restart OVS each time to see it.
>
>  I’ll wait for v15 to see how you fixed it ;)