On 05/01/2026 15:08, Eelco Chaudron wrote:
External email: Use caution opening links or attachments
On 4 Jan 2026, at 8:23, Eli Britstein wrote:
On 02/01/2026 13:02, Eelco Chaudron wrote:
External email: Use caution opening links or attachments
On 1 Jan 2026, at 15:34, Eli Britstein via dev wrote:
On 02/12/2025 16:04, Eelco Chaudron wrote:
Introduce per-offload-provider implementation types and remove the
synced_dp abstraction from the dpif level. The new API allows
querying the offload implementation type and which provider offloads
a given port.
Signed-off-by: Eelco Chaudron <[email protected]>
---
[...]
diff --git a/lib/dpif-offload.h b/lib/dpif-offload.h
index 541b59679..29211c96a 100644
--- a/lib/dpif-offload.h
+++ b/lib/dpif-offload.h
@@ -30,6 +30,23 @@ struct dpif_offload_dump {
void *state;
};
+/* Definition of the DPIF offload implementation type.
+ *
+ * The 'DPIF_OFFLOAD_IMPL_SYNC' implementation has a single view, the offload
+ * provider is responsible for synchronizing flows and statistics through the
+ * dpif flow operations. An example of this is DPDK's rte_flow.
+ *
+ * The 'DPIF_OFFLOAD_IMPL_HW_ONLY' implementation tries to install a hardware
+ * flow first. If successful, no dpif-layer software flow will be installed.
+ * Offload-specific callbacks are then used to manage the flow and query
+ * statistics. An example of this is kernel TC.
+ */
+enum dpif_offload_impl_type {
+ DPIF_OFFLOAD_IMPL_NONE,
+ DPIF_OFFLOAD_IMPL_SYNC,
+ DPIF_OFFLOAD_IMPL_HW_ONLY,
+};
+
I don't understand neither the purpose of this property, nor the naming. For naming, it is not
"SYNC/HW_ONLY" but maybe "INTERNAL/EXTERNAL". INTERNAL means the source of
truth is OVS, and all the state is managed by OVS, while EXTERNAL means it's in the kernel domain.
I can't actually think of another use-case that it won't be either
userspace/kernelspace.
Regarding purpose, it is not something that is changing, surely not in the
class level (per-flow, yes).
You’re right that with the current implementations this effectively maps to a
userspace vs. kernel distinction. However, that is an artifact of today’s
implementations, not a fundamental constraint.
While not currently supported, it is possible that a future kernel-based HW
offload implementation would not have a kernel flow representation like TC does
today. In such a case, even though the offload provider lives in the kernel
domain, a dpif software flow might still be required, similar to how userspace
offloads behave now.
The current infrastructure does not support this model, but if we ever need it,
the decision cannot be hard-coded to “kernel vs. userspace.” It needs to be
dpif-agnostic, which is why this property belongs at the dpif-offload-provider
level.
This is also not something that changes dynamically: all flows managed by a
given provider follow the same model. While the outcome may differ per flow
(e.g., whether HW installation succeeds), the management model itself is
provider-wide, which makes a class-level attribute appropriate.
From a naming perspective;
SYNC means that HW flows are kept in sync with dpif software flows: OVS remains
the source of truth.
HW_ONLY means that flows are fully managed by the HW offload provider. If a
flow is successfully offloaded, no dpif-layer software flow exists.
If you have a better name suggestion that captures this distinction without
tying it to userspace/kernel semantics, I’m happy to consider it.
Naming suggestions:
SYNC: "OVS"/"INTERNAL"/"USERSPACE"
HW_ONLY: "PROVIDER"/"EXTERNAL"/"KERNEL"
I think "INTERNAL"/"EXTERNAL" are my primary.
I don't rule out "USERSPACE"/"KERNEL". It is correct that this is the "current"
implementation and it has a kind of "implementation coupling".
However, it makes it more readable/understandable for the reader, and if this
coupling ever breaks, we can think of better names then.
I still feel we should avoid user vs. kernel naming, as that implicitly ties
the model to a specific dpif, which is what I want to avoid.
Thinking about it again, the naming should focus on where flow state is owned
and managed, rather than on the execution domain. Based on that, I’ve updated
the names to:
DPIF_OFFLOAD_IMPL_FLOWS_DPIF_SYNCED
DPIF_OFFLOAD_IMPL_FLOWS_PROVIDER_ONLY
Let me know what you think, and we can continue the discussion here. In the
meantime, I’ll send out a v4 with this name change.
OK.
Cheers,
Eelco
[...]
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 363314ad9..2aa77a4de 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -826,12 +826,8 @@ udpif_get_n_flows(struct udpif *udpif)
atomic_store_relaxed(&udpif->n_flows_timestamp, now);
dpif_get_dp_stats(udpif->dpif, &stats);
flow_count = stats.n_flows;
-
- if (!dpif_synced_dp_layers(udpif->dpif)) {
- /* If the dpif layer does not sync the flows, we need to include
- * the hardware offloaded flows separately. */
- flow_count += dpif_offload_flow_get_n_offloaded(udpif->dpif);
- }
+ flow_count += dpif_offload_flow_get_n_offloaded_by_impl(
+ udpif->dpif, DPIF_OFFLOAD_IMPL_HW_ONLY);
Why does ofproto layer cares about offload? It should be agnostic to it.
This was added earlier because HW-offloaded flows were not included in the
total flow count. I agree that ofproto should be agnostic to offload details,
and this logic does not really belong here.
The right place for this is dpif statistics, so ofproto can rely on complete
counters without knowing about offload. Since this cleanup is not directly
related to this patch series, I suggest handling it separately in a follow-up
patch once this series is merged.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
