When tc_netdev_flow_put() rejects a flow because the upstream
recirc chain is not yet registered (used_chains count is zero),
the flow is permanently stranded in the kernel. Return EAGAIN
instead of EOPNOTSUPP so the revalidator retries the offload
once a sibling flow registers the chain.
A new bool offload_deferred on struct dpif_op and struct udpif_key
propagates the rejection to the revalidator, which issues a retry
on the next cycle. Retries are bounded to
OFFLOAD_DEFERRED_RETRY_MSEC (2000ms) from the first failed attempt.
Fixes: 273a4fce951a ("netdev-offload-tc: Only install recirc flows if the
parent is present.")
Reported-by: Numan Siddique <[email protected]>
Signed-off-by: Eelco Chaudron <[email protected]>
---
lib/dpif-offload-tc-netdev.c | 27 +++++++------
lib/dpif-offload-tc.c | 10 ++++-
lib/dpif.c | 7 ++++
lib/dpif.h | 3 ++
ofproto/ofproto-dpif-upcall.c | 27 +++++++++++++
tests/system-offloads-traffic.at | 68 ++++++++++++++++++++++++++++++++
6 files changed, 129 insertions(+), 13 deletions(-)
diff --git a/lib/dpif-offload-tc-netdev.c b/lib/dpif-offload-tc-netdev.c
index a2f4ab8b5..3a521aee8 100644
--- a/lib/dpif-offload-tc-netdev.c
+++ b/lib/dpif-offload-tc-netdev.c
@@ -2355,17 +2355,6 @@ tc_netdev_flow_put(struct dpif *dpif, struct netdev
*netdev,
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,
@@ -2687,6 +2676,22 @@ tc_netdev_flow_put(struct dpif *dpif, struct netdev
*netdev,
return EOPNOTSUPP;
}
+ if (chain && !ccmap_find(&used_chains, 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.
+ *
+ * Return EAGAIN rather than EOPNOTSUPP: the chain may not be
+ * registered yet because the upstream flow install is still in
+ * progress. Unlike EOPNOTSUPP, EAGAIN tells the revalidator to
+ * retry the TC offload on a subsequent cycle. The flow is still
+ * installed in the kernel datapath (dp:ovs) in either case. */
+ VLOG_DBG_RL(&rl, "match for chain %u failed due to non-existing "
+ "goto chain action",
+ chain);
+ return EAGAIN;
+ }
+
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",
diff --git a/lib/dpif-offload-tc.c b/lib/dpif-offload-tc.c
index 3b84d9f20..cb996fa4a 100644
--- a/lib/dpif-offload-tc.c
+++ b/lib/dpif-offload-tc.c
@@ -579,7 +579,8 @@ tc_parse_flow_put(struct tc_offload *offload_tc, struct
dpif *dpif,
}
netdev_set_hw_info(oor_netdev, HW_INFO_TYPE_OOR, true);
}
- level = (err == ENOSPC || err == EOPNOTSUPP) ? VLL_DBG : VLL_ERR;
+ level = (err == ENOSPC || err == EOPNOTSUPP
+ || err == EAGAIN) ? VLL_DBG : VLL_ERR;
VLOG_RL(&rl, level, "failed to offload flow: %s: %s",
ovs_strerror(err),
(oor_netdev ? netdev_get_name(oor_netdev) :
@@ -705,7 +706,12 @@ tc_operate(struct dpif *dpif, const struct dpif_offload
*offload_,
break;
} /* End of 'switch (op->type)'. */
- if (error != EOPNOTSUPP && error != ENOENT) {
+ if (error == EAGAIN) {
+ /* The recirc ID matched by this flow is not yet registered in
+ * TC (no upstream flow has a goto action for this chain).
+ * Defer the offload so the revalidator retries next cycle. */
+ op->offload_deferred = true;
+ } else if (error != EOPNOTSUPP && error != ENOENT) {
/* If the operation is unsupported or the entry was not found,
* we are skipping this flow operation. Otherwise, it was
* processed and we should report the result. */
diff --git a/lib/dpif.c b/lib/dpif.c
index 1afc7c662..0b93b0fae 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -1354,10 +1354,17 @@ dpif_operate(struct dpif *dpif, struct dpif_op **ops,
size_t n_ops,
for (i = 0; i < n_ops; i++) {
struct dpif_op *op = ops[i];
op->error = EINVAL;
+ op->offload_deferred = false;
}
return;
}
+ /* Initialize offload_deferred for all ops; the offload provider sets
+ * it to true if the flow cannot be offloaded yet. */
+ for (size_t i = 0; i < n_ops; i++) {
+ ops[i]->offload_deferred = false;
+ }
+
while (n_ops > 0) {
size_t chunk;
diff --git a/lib/dpif.h b/lib/dpif.h
index 3e6a34a25..79621887c 100644
--- a/lib/dpif.h
+++ b/lib/dpif.h
@@ -782,6 +782,9 @@ int dpif_execute(struct dpif *, struct dpif_execute *);
struct dpif_op {
enum dpif_op_type type;
int error;
+ bool offload_deferred; /* True if the offload provider requires a
+ * prerequisite flow to be programmed first.
+ * The revalidator will retry on the next cycle. */
union {
struct dpif_flow_put flow_put;
struct dpif_flow_del flow_del;
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 8e4897202..85071e852 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -50,6 +50,7 @@
#define UPCALL_MAX_BATCH 64
#define REVALIDATE_MAX_BATCH 50
#define UINT64_THREE_QUARTERS (UINT64_MAX / 4 * 3)
+#define OFFLOAD_DEFERRED_RETRY_MSEC 2000
VLOG_DEFINE_THIS_MODULE(ofproto_dpif_upcall);
@@ -347,6 +348,8 @@ struct udpif_key {
#define OFFL_REBAL_INTVL_MSEC 3000 /* dynamic offload rebalance freq */
struct netdev *in_netdev; /* in_odp_port's netdev */
bool offloaded; /* True if flow is offloaded */
+ bool offload_deferred; /* Deferred, retry next cycle. */
+ long long int offload_deferred_expire; /* Retry deadline in msec. */
uint64_t flow_pps_rate; /* Packets-Per-Second rate */
long long int flow_time; /* last pps update time */
uint64_t flow_packets; /* #pkts seen in interval */
@@ -1746,6 +1749,11 @@ handle_upcalls(struct udpif *udpif, struct upcall
*upcalls,
transition_ukey(ukey, UKEY_EVICTED);
} else if (ukey->state < UKEY_OPERATIONAL) {
transition_ukey(ukey, UKEY_OPERATIONAL);
+ if (ops[i].dop.offload_deferred) {
+ ukey->offload_deferred_expire =
+ time_msec() + OFFLOAD_DEFERRED_RETRY_MSEC;
+ ukey->offload_deferred = true;
+ }
}
ovs_mutex_unlock(&ukey->mutex);
}
@@ -1836,6 +1844,8 @@ ukey_create__(const struct nlattr *key, size_t key_len,
ukey->xcache = NULL;
ukey->offloaded = false;
+ ukey->offload_deferred = false;
+ ukey->offload_deferred_expire = 0;
ukey->in_netdev = NULL;
ukey->flow_packets = ukey->flow_backlog_packets = 0;
@@ -2552,6 +2562,18 @@ push_dp_ops(struct udpif *udpif, struct ukey_op *ops,
size_t n_ops)
for (i = 0; i < n_ops; i++) {
struct ukey_op *op = &ops[i];
+ if (op->ukey && op->dop.offload_deferred) {
+ long long int now = time_msec();
+
+ ovs_mutex_lock(&op->ukey->mutex);
+ if (now >= op->ukey->offload_deferred_expire) {
+ op->ukey->offload_deferred_expire =
+ now + OFFLOAD_DEFERRED_RETRY_MSEC;
+ }
+ op->ukey->offload_deferred = true;
+ ovs_mutex_unlock(&op->ukey->mutex);
+ }
+
if (op->dop.error) {
if (op->ukey) {
ovs_mutex_lock(&op->ukey->mutex);
@@ -2984,6 +3006,11 @@ revalidate(struct revalidator *revalidator)
/* Takes ownership of 'recircs'. */
reval_op_init(&ops[n_ops++], result, udpif, ukey, &recircs,
&odp_actions);
+ } else if (ukey->offload_deferred) {
+ ukey->offload_deferred = false;
+ if (time_msec() < ukey->offload_deferred_expire) {
+ put_op_init(&ops[n_ops++], ukey, DPIF_FP_MODIFY);
+ }
}
ovs_mutex_unlock(&ukey->mutex);
}
diff --git a/tests/system-offloads-traffic.at b/tests/system-offloads-traffic.at
index 55c26a0ce..484654cb1 100644
--- a/tests/system-offloads-traffic.at
+++ b/tests/system-offloads-traffic.at
@@ -1200,6 +1200,74 @@ OVS_WAIT_WHILE(
OVS_TRAFFIC_VSWITCHD_STOP
AT_CLEANUP
+AT_SETUP([offloads - split recirc rules kernel vs offload])
+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")
+
+dnl Two source IPs on the same veth guarantee the same in_port and therefore
+dnl the same recirc ID R1 for the second datapath flow.
+NS_CHECK_EXEC([at_ns0], [ip addr add 10.0.0.3/24 dev p1])
+
+AT_CHECK([ovs-vsctl -- set interface ovs-p1 ofport_request=1 \
+ -- set interface ovs-p2 ofport_request=2])
+
+AT_CHECK([ovs-appctl vlog/set dpif_offload_tc_netdev:dbg])
+
+dnl controller() is not TC-offloadable, so both datapath flows land in dp:ovs.
+AT_DATA([flows.txt], [dnl
+table=0,arp,actions=NORMAL
+table=0,priority=100,in_port=ovs-p1,ip,nw_dst=10.0.0.2,actions=controller(),ct(commit,table=1)
+table=0,priority=100,in_port=ovs-p2,ip,actions=ovs-p1
+table=1,ip,actions=ovs-p2
+])
+AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
+
+NS_CHECK_EXEC([at_ns0], [ping -q -c 3 -i 0.3 -W 2 10.0.0.2 | FORMAT_PING],
[0], [dnl
+3 packets transmitted, 3 received, 0% packet loss, time 0ms
+])
+AT_CHECK([ovs-appctl revalidator/wait])
+
+AT_CHECK([grep -q "match for chain .* failed due to non-existing goto chain
action" \
+ ovs-vswitchd.log])
+AT_CHECK([ovs-appctl dpctl/dump-flows --names type=ovs \
+ filter='in_port(ovs-p1),ipv4' | grep -c "in_port(ovs-p1)"], [0], [2
+])
+AT_CHECK([ovs-appctl dpctl/dump-flows --names type=tc \
+ filter='in_port(ovs-p1),ipv4'], [0], [dnl
+])
+
+dnl Replace the rule with plain ct (TC-offloadable). Traffic from 10.0.0.3
+dnl offloads its first datapath flow to dp:tc, which registers the recirc ID.
+dnl After the fix, the second datapath flow should also be offloaded to dp:tc.
+AT_CHECK([ovs-ofctl del-flows br0 "table=0,in_port=ovs-p1,ip,nw_dst=10.0.0.2"])
+AT_DATA([flows2.txt], [dnl
+table=0,priority=100,in_port=ovs-p1,ip,nw_dst=10.0.0.2,actions=ct(commit,table=1)
+])
+AT_CHECK([ovs-ofctl add-flows br0 flows2.txt])
+
+NS_CHECK_EXEC([at_ns0], [ping -q -c 3 -i 0.3 -W 2 -I 10.0.0.3 10.0.0.2 |
FORMAT_PING], [0], [dnl
+3 packets transmitted, 3 received, 0% packet loss, time 0ms
+])
+
+AT_CHECK([ovs-appctl revalidator/wait])
+OVS_WAIT_UNTIL([ovs-appctl dpctl/dump-flows --names type=tc \
+ filter='in_port(ovs-p1),ipv4' | grep -q "actions:ovs-p2"])
+
+AT_CHECK([ovs-appctl dpctl/dump-flows --names type=tc \
+ filter='in_port(ovs-p1),ipv4' | grep -c "in_port(ovs-p1)"], [0], [2
+])
+AT_CHECK([ovs-appctl dpctl/dump-flows --names type=ovs \
+ filter='in_port(ovs-p1),ipv4' | grep "actions:ovs-p2"], [1])
+AT_CHECK([ovs-appctl dpctl/dump-flows --names type=tc \
+ filter='in_port(ovs-p1),ipv4' | grep "actions:ovs-p2"], [0],
[ignore])
+
+OVS_TRAFFIC_VSWITCHD_STOP
+AT_CLEANUP
+
AT_SETUP([offloads - ovs-appctl dpif/offload/show - offloads enabled])
AT_KEYWORDS([dpif-offload])
OVS_TRAFFIC_VSWITCHD_START([], [],
--
2.54.0
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
