On 12 Dec 2024, at 23:59, Ilya Maximets wrote:
> On 12/3/24 17:24, Eelco Chaudron wrote: >> When flows are added through TC, only the match and actions are >> verified to determine if they can be handled by TC. If they can, >> the TC flow is installed. >> >> However, when the flow is a continuation of a previously recirculated flow, >> it can happen that the flow performing the recirculation is installed >> in the kernel. This may occur, for example, if it includes an action that >> cannot be handled by TC. >> >> If the kernel module has the first flow but not the second one (missing >> because it is programmed in TC), the flow is sent to userspace via an >> upcall. >> >> This patch tracks which recirculation goto actions are handled by TC. >> A matching TC rule is installed only if the corresponding recirculation >> ID is confirmed to be handled by TC. >> >> Signed-off-by: Eelco Chaudron <[email protected]> >> --- >> lib/netdev-offload-tc.c | 51 ++++++++++++++++++++++++++++++-- >> tests/system-offloads-traffic.at | 45 ++++++++++++++++++++++++++++ >> 2 files changed, 94 insertions(+), 2 deletions(-) >> >> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c >> index 74e60d0f7..eb7c0886e 100644 >> --- a/lib/netdev-offload-tc.c >> +++ b/lib/netdev-offload-tc.c >> @@ -19,6 +19,7 @@ >> #include <errno.h> >> #include <linux/if_ether.h> >> >> +#include "ccmap.h" >> #include "dpif.h" >> #include "hash.h" >> #include "id-pool.h" >> @@ -78,6 +79,10 @@ struct policer_node { >> uint32_t police_idx; >> }; >> >> +/* ccmap and protective mutex for counting recirculation id (chain) usage. >> */ >> +static struct ovs_mutex used_chains_mutex = OVS_MUTEX_INITIALIZER; >> +static struct ccmap used_chains OVS_GUARDED; >> + >> /* Protects below meter police ids pool. */ >> static struct ovs_mutex meter_police_ids_mutex = OVS_MUTEX_INITIALIZER; >> static struct id_pool *meter_police_ids >> OVS_GUARDED_BY(meter_police_ids_mutex); >> @@ -204,6 +209,10 @@ static struct ovs_mutex ufid_lock = >> OVS_MUTEX_INITIALIZER; >> * @adjust_stats: When flow gets updated with new actions, we need to adjust >> * the reported stats to include previous values as the >> hardware >> * rule is removed and re-added. This stats copy is used for >> it. >> + * @chain_goto: If a TC jump action exists for the flow, the target chain it >> + * jumps to is stored here. Only a single goto action is >> stored, >> + * as TC supports only one goto action per flow (there is no >> + * return mechanism). >> */ >> struct ufid_tc_data { >> struct hmap_node ufid_to_tc_node; >> @@ -212,6 +221,7 @@ struct ufid_tc_data { >> struct tcf_id id; >> struct netdev *netdev; >> struct dpif_flow_stats adjust_stats; >> + uint32_t chain_goto; >> }; >> >> static void >> @@ -233,6 +243,13 @@ del_ufid_tc_mapping_unlocked(const ovs_u128 *ufid) >> hmap_remove(&ufid_to_tc, &data->ufid_to_tc_node); >> hmap_remove(&tc_to_ufid, &data->tc_to_ufid_node); >> netdev_close(data->netdev); >> + >> + if (data->chain_goto) { >> + ovs_mutex_lock(&used_chains_mutex); >> + ccmap_dec(&used_chains, data->chain_goto); >> + ovs_mutex_unlock(&used_chains_mutex); >> + } >> + >> free(data); >> } >> >> @@ -288,7 +305,8 @@ del_filter_and_ufid_mapping(struct tcf_id *id, const >> ovs_u128 *ufid, >> /* Add ufid entry to ufid_to_tc hashmap. */ >> static void >> add_ufid_tc_mapping(struct netdev *netdev, const ovs_u128 *ufid, >> - struct tcf_id *id, struct dpif_flow_stats *stats) >> + struct tcf_id *id, struct dpif_flow_stats *stats, >> + uint32_t chain_goto) >> { >> struct ufid_tc_data *new_data = xzalloc(sizeof *new_data); >> size_t ufid_hash = hash_bytes(ufid, sizeof *ufid, 0); >> @@ -300,6 +318,7 @@ add_ufid_tc_mapping(struct netdev *netdev, const >> ovs_u128 *ufid, >> new_data->ufid = *ufid; >> new_data->id = *id; >> new_data->netdev = netdev_ref(netdev); >> + new_data->chain_goto = chain_goto; >> if (stats) { >> new_data->adjust_stats = *stats; >> } >> @@ -2261,6 +2280,7 @@ netdev_tc_flow_put(struct netdev *netdev, struct match >> *match, >> struct flow_tnl *tnl_mask = &mask->tunnel; >> struct dpif_flow_stats adjust_stats; >> bool recirc_act = false; >> + uint32_t chain_goto = 0; >> uint32_t block_id = 0; >> struct tcf_id id; >> uint32_t chain; >> @@ -2280,6 +2300,17 @@ netdev_tc_flow_put(struct netdev *netdev, struct >> match *match, >> chain = key->recirc_id; >> mask->recirc_id = 0; >> >> + if (chain) { >> + /* If we match on a recirculation ID, we must ensure the previous >> + * flow is also in the TC datapath; otherwise, the entry is useless, >> + * as the related packets will be handled by upcalls. */ >> + if (!ccmap_find(&used_chains, chain)) { >> + VLOG_DBG_RL(&rl, "match for chain %u failed due to non-existing >> " >> + "goto chain action", chain); >> + return EOPNOTSUPP; >> + } >> + } >> + >> if (flow_tnl_dst_is_set(&key->tunnel) || >> flow_tnl_src_is_set(&key->tunnel)) { >> VLOG_DBG_RL(&rl, >> @@ -2594,6 +2625,20 @@ netdev_tc_flow_put(struct netdev *netdev, struct >> match *match, >> return EOPNOTSUPP; >> } >> >> + if (recirc_act) { >> + struct tc_action *action = flower.actions; >> + >> + for (int i = 0; i < flower.action_count; i++, action++) { >> + if (action->type == TC_ACT_GOTO && action->chain) { >> + chain_goto = action->chain; >> + ovs_mutex_lock(&used_chains_mutex); >> + ccmap_inc(&used_chains, chain_goto); >> + ovs_mutex_unlock(&used_chains_mutex); >> + break; >> + } >> + } >> + } > > Shouldn't we also undo this if the tc_replace_flower ultimately fails > or if we get some other error along the way? ACK, will move this so it only gets inserted on successful addition of the flow. >> + >> memset(&adjust_stats, 0, sizeof adjust_stats); >> if (get_ufid_tc_mapping(ufid, &id) == 0) { >> VLOG_DBG_RL(&rl, "updating old handle: %d prio: %d", > > What happens if we have flow1 --> flow2 --> flow3, all offloaded. > Now, we modify actions of flow2 slightly, but in a way that is no longer > offloadable. flow2 will be removed from TC and installed to the kernel. > Next when packet comes it matches flow1 in TC, then goes to the kernel > and matches flow2, then it gets upcalled. We will now try to install > the flow3 again. Normally, it would be the same datapath and it will > be a simple EEXIST. However, now we'll fail in TC and install flow3 > also into the kernel. Now we have the same datapath flow in both the > kernel and TC and only one ukey. It's probably fine for the datapath, > but I'm not sure what will happen in control plane since we'll be > dumping the same flow from both with different statistics. > > Do you have any thoughts on this? This is an interesting case. Actually, we do get the EEXIST on the lookup for flow3, but as the flow itself is not processing any traffic, it will time out. Then, a new flow gets installed in the kernel DP, and it all works as expected. However, I’m not sure how to solve this. We do not know who generated the upcall, i.e., was it already in the kernel DP, or was it straight from TC? For now, this patch knows about the behavior (with added tests), but it has no fix. I will send a v2 soon. //Eelco >> @@ -2619,7 +2664,7 @@ netdev_tc_flow_put(struct netdev *netdev, struct match >> *match, >> memset(stats, 0, sizeof *stats); >> netdev_tc_adjust_stats(stats, &adjust_stats); >> } >> - add_ufid_tc_mapping(netdev, ufid, &id, &adjust_stats); >> + add_ufid_tc_mapping(netdev, ufid, &id, &adjust_stats, chain_goto); >> } >> >> return err; >> @@ -3096,6 +3141,8 @@ netdev_tc_init_flow_api(struct netdev *netdev) >> tc_add_del_qdisc(ifindex, false, 0, hook); >> >> if (ovsthread_once_start(&once)) { >> + ccmap_init(&used_chains); >> + >> probe_tc_block_support(ifindex); >> /* Need to re-fetch block id as it depends on feature availability. >> */ >> block_id = get_block_id_from_netdev(netdev); >> diff --git a/tests/system-offloads-traffic.at >> b/tests/system-offloads-traffic.at >> index 78c6f5d7e..f890ced68 100644 >> --- a/tests/system-offloads-traffic.at >> +++ b/tests/system-offloads-traffic.at >> @@ -1015,5 +1015,50 @@ AT_CHECK( >> [grep -q -F "set(tunnel(dst=172.31.1.1,ttl=64,tp_dst=4789,flags(csum)))" \ >> stdout]) >> >> +OVS_TRAFFIC_VSWITCHD_STOP >> +AT_CLEANUP >> + >> +AT_SETUP([offloads - split kernel vs offload datapath rules]) >> +OVS_TRAFFIC_VSWITCHD_START([], [], [-- set Open_vSwitch . >> other_config:hw-offload=true]) >> + >> +ADD_NAMESPACES(at_ns0, at_ns1) >> + >> +ADD_VETH(p1, at_ns0, br0, "10.0.0.1/24") >> +ADD_VETH(p2, at_ns1, br0, "10.0.0.2/24") >> + >> +AT_CHECK([ovs-vsctl -- set interface ovs-p1 ofport_request=1 \ >> + -- set interface ovs-p2 ofport_request=2]) >> + >> +AT_DATA([groups.txt], [dnl >> +group_id=1,type=select,selection_method=dp_hash,bucket=bucket_id:0,weight:100,actions=ct(commit,table=2) >> +]) >> +AT_DATA([flows.txt], [dnl >> +table=0,arp,actions=NORMAL >> +table=0,priority=100,cookie=0x12345678,in_port=ovs-p1,ip,nw_dst=10.0.0.2,actions=resubmit(,1) >> +table=0,priority=100,cookie=0xabcedf,in_port=ovs-p2,ip,nw_dst=10.0.0.1,actions=ct(table=3) >> +table=1,priority=200,ip,actions=group:1 >> +table=2,ip,actions=ovs-p2 >> +table=3,ip,actions=ovs-p1 >> +]) >> +AT_CHECK([ovs-ofctl add-groups br0 groups.txt]) >> +AT_CHECK([ovs-ofctl add-flows br0 flows.txt]) >> + >> +NS_CHECK_EXEC([at_ns0], [ping -q -c 10 -i 0.1 -W 2 10.0.0.2 | FORMAT_PING], >> [0], [dnl >> +10 packets transmitted, 10 received, 0% packet loss, time 0ms >> +]) >> + >> +AT_CHECK([ovs-appctl revalidator/wait]) >> + >> +dnl In this test we should not have the first recirculation(s) in the kernel >> +dnl datapath, and the final flow in tc. They should all be in the kernel >> +dnl datapath, as the dp_hash() action is currently not supported by TC. > > Nit: 'tc' vs 'TC' in two sentenses nearby. > >> +dnl The command below ensures they are all handled in the kernel datapath. >> +AT_CHECK([ovs-appctl dpctl/dump-flows --names type=ovs >> filter='in_port(ovs-p1),ipv4' | \ >> + strip_recirc | strip_dp_hash | DUMP_CLEAN_SORTED], [0], [dnl >> +recirc_id(<recirc>),in_port(ovs-p1),eth(),eth_type(0x0800),ipv4(dst=10.0.0.2,frag=no), >> packets:9, bytes:882, used:0.001s, actions:hash(l4(0)),recirc(<recirc>) >> +recirc_id(<recirc>),in_port(ovs-p1),eth(),eth_type(0x0800),ipv4(frag=no), >> packets:9, bytes:882, used:0.001s, actions:ct(commit),recirc(<recirc>) >> +recirc_id(<recirc>),in_port(ovs-p1),eth(),eth_type(0x0800),ipv4(frag=no), >> packets:9, bytes:882, used:0.001s, actions:ovs-p2 >> +]) >> + >> OVS_TRAFFIC_VSWITCHD_STOP >> AT_CLEANUP >> \ No newline at end of file _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
