On 22 Jul 2026, at 18:13, Aaron Conole wrote:
> Eelco Chaudron <[email protected]> writes:
>
>> On 30 Jun 2026, at 1:20, Ilya Maximets wrote:
>>
>>> On 6/23/26 9:19 PM, Eelco Chaudron via dev wrote:
>>>> This patch introduces a new API to the offload provider framework that
>>>> allows hardware offload implementations to control hash value calculation
>>>> for the OVS_ACTION_ATTR_HASH action.
>>>>
>>>> Background and Motivation
>>>> =========================
>>>>
>>>> The OVS hash action (OVS_ACTION_ATTR_HASH) is used to compute a hash value
>>>> from packet header fields, primarily for load balancing across multiple
>>>> paths using the select group action. The hash value is stored in the
>>>> packet's metadata and used by subsequent actions to distribute flows
>>>> across multiple output ports.
>>>>
>>>> However, hardware offload implementations may require different approaches
>>>> to hash calculation:
>>>>
>>>> 1. Hardware NICs may use different hash functions or hash inputs than
>>>> the software datapath, which can lead to inconsistent load distribution
>>>> when mixing hardware and software paths.
>>>>
>>>> 2. Some hardware may support enhanced hashing mechanisms (e.g., using
>>>> symmetric hashing for bidirectional flows or hardware-specific hash
>>>> engines) that provide better load distribution than the default
>>>> software implementation.
>>>>
>>>> Design
>>>> ======
>>>>
>>>> This patch adds a new optional callback to the dpif_offload_class:
>>>>
>>>> bool (*netdev_get_dp_hash)(const struct dpif_offload *,
>>>> const struct netdev *ingress_netdev,
>>>> struct dp_packet *,
>>>> const struct ovs_action_hash *, uint32_t
>>>> *hash);
>>>>
>>>> To integrate this into the action execution path, a new optional callback
>>>> type, odp_hash_cb, is passed to odp_execute_actions() to allow per-packet
>>>> hash overrides during OVS_ACTION_ATTR_HASH processing. The dpif-netdev
>>>> datapath provides an implementation that calls the offload provider's
>>>> netdev_get_dp_hash when hardware offload is enabled and the original
>>>> ingress port is known. If the provider returns true, the returned hash
>>>> value is used; otherwise, OVS falls back to the standard hash calculation.
>>>>
>>>> Signed-off-by: Eelco Chaudron <[email protected]>
>>
>> [...]
>>
>>>> +static bool
>>>> +dp_hash_override(void *aux_, struct dp_packet *packet,
>>>> + const struct ovs_action_hash *hash_act, uint32_t *hash)
>>>> +{
>>>> + struct dp_netdev_execute_aux *aux = aux_;
>>>> + struct dp_netdev_pmd_thread *pmd = aux->pmd;
>>>> +
>>>> + if (!dpif_offload_enabled()) {
>>>> + return false;
>>>> + }
>>>> +
>>>> + if (aux->cached_in_odpp != packet->md.orig_in_port
>>>> + || !aux->cached_in_netdev) {
>>>> + struct tx_port *in_port = pmd_send_port_cache_lookup(
>>>> + pmd, packet->md.orig_in_port);
>>>> +
>>>> + if (!in_port) {
>>>> + return false;
>>>> + }
>>>> + aux->cached_in_odpp = packet->md.orig_in_port;
>>>> + aux->cached_in_netdev = in_port->port->netdev;
>>>> + }
>>>> +
>>>> + return dpif_offload_netdev_get_dp_hash(aux->cached_in_netdev, packet,
>>>> + hash_act, hash);
>>>> +}
>>>
>>> I wonder why this function is specific to userspace datapath?
>>> The only specifc thing here is the port lookup, but it should
>>> not actually be specific. We should be able to lookup a netdev
>>> by port number for any datapath type, e.g. from the offload
>>> module itself - dpif_offload_get_netdev_by_port_id().
>>>
>>> It also seems wrong that the callback is not defined for the
>>> standard path of the dpif-netlink. Since we decoupled the
>>> offload layer form the dpif implementation, generic action
>>> execution should work the same way regardless of the datapath.
>
> Well, the idea was to have the default implementation, and allow a
> datapath to override it. We could just make all the dpifs define this
> callback, though.
>
>>> And if dp_hash_override() doesn't need to be datapath-specific,
>>> there is no point for it to be a callback, it could be just a
>>> function inside the odp-execute module, right? Or am I missing
>>> something here?
>
> This could also work - just have the default swallow the behavior.
> Although, this does mean that the odp_execute implementation will
> possibly need to get adjusted in the event that a new dpif
> implementation comes along that needs something very different. Then
> again, that is maybe a good thing (tm) to require having updates since
> we can provide more scrutiny in that case? But see below for my reply
> about abstraction.
>
>>> The call into hardware/firmware to obtain the hash will be
>>> expensive, so I'm also not sure if the netdev caching is stricty
>>> Necessary here.
Might not need to be. It could just be a sw based hash like
normal, but just different. So doing the port lookup might still be
an additional overhead we would like to skip, as it's already
known at a lower layer.
>> I did some experiments to see what the alternative looks like. For
>> this to work, we need a way to look up the original netdev in the
>> datapath-independent code. So for the OVS_ACTION_ATTR_HASH case in
>> odp_execute_actions() we need to know the odp_port or the netdev.
>
> Well, there is an argument that if the orig_in_port is the same across
> the batch, doing the port lookup on each packet could be relevant. And
> in the case of dpif-netdev (especially for DPDK ports) processing all
> the packets in the batch will have the same orig_in_port. In a full
> batch, that's 32 lookups - I don't think it's insignificant, but it's
> true that it is minor compared to the HW sync.
See above.
>> If we have the original_ingress_netdev, we can call
>> dpif_offload_netdev_get_dp_hash() unmodified. We could read
>> md.orig_in_port from the packet, but to translate that quickly to a
>> netdev we need the dpif, which is not part of the generic
>> odp_execute_actions() API.
>>
>> So we are left with either passing the original_ingress_netdev or
>> the dpif to odp_execute_actions().
>>
>> In v1 I avoided this by moving OVS_ACTION_ATTR_HASH into
>> requires_datapath_assistance() and changing odp_execute_cb to return
>> bool, with dpif_execute_helper_cb returning false for HASH to fall
>> back to software handling. However, that still has your objection of
>> introducing datapath-specific logic into generic code.
>>
>> Passing ingress_netdev also does not work cleanly, as packets in a
>> batch may have different md.orig_in_port values and therefore require
>> different netdev lookups per packet. So I think the cleanest solution
>> is to pass the dpif to odp_execute_actions() and resolve the netdev
>> per packet from md.orig_in_port inside the action loop. Any thoughts,
>> or do you have another approach in mind?
>
> This would introduce something that feels like an abstraction break.
> What I mean is, the current odp implementation has a clear separation
> from the dpif layer and treats all of the dp call-in arguments as just
> some opaque blob to pass to the eventual dpif handler. Introducing a
> dpif object here can invite some layer violations by baking more of the
> dpif into the odp layer, and sortof making odp_execute *less* generic,
> since we may need to put some kinds of offload dependencies in place.
>
> Callbacks are currently the way ODP separates itself from the dpif
> layer. If we introduce something in dpif we then do a bidirectional
> dependency (ie: dpif -> odp -> dpif). That seems less desirable since
> it makes the abstractions a bit fuzzy.
The more I think about this, the more I feel like the original
version of this patch is what we need, i.e. where the code is
executed as part of the datapath assistance path. I guess the
main issue raised was that it would become a "might" require
datapath assistance path rather than it always need datapath
assistance. But I think it would be fine for this specific use
case, and maybe for use cases in the future.
I'll try to come up with a v3 that uses this approach, and be
clear about the "might" part. We can then continue discussion
on this version.
Cheers,
Eelco
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
