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. 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
