Skip to content

Commit

Permalink
netdev-offload-tc: Only install recirc flows if the parent is present.
Browse files Browse the repository at this point in the history 
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]>
Signed-off-by: 0-day Robot <[email protected]>
  • Loading branch information
@chaudron @ovsrobot
chaudron authored and ovsrobot committed Dec 3, 2024
1 parent 77ac0b2 commit 9ea2e00
Show file tree
Hide file tree
Showing 2 changed files with 94 additions and 2 deletions.
51 changes: 49 additions & 2 deletions lib/netdev-offload-tc.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Expand All @@ -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
Expand All @@ -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);
}

Expand Down Expand Up @@ -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);
Expand All @@ -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;
}
Expand Down Expand Up @@ -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;
Expand All @@ -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,
Expand Down Expand Up @@ -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;
}
}
}

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",
Expand All @@ -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;
Expand Down Expand Up @@ -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);
Expand Down
45 changes: 45 additions & 0 deletions tests/system-offloads-traffic.at
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
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

0 comments on commit 9ea2e00

Please sign in to comment.