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.
>
> 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?
>
> 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.
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.
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?
>> +
>> static void
>> dp_execute_cb(void *aux_, struct dp_packet_batch *packets_,
>> const struct nlattr *a, bool should_steal)
>> @@ -8290,10 +8319,10 @@ dp_netdev_execute_actions(struct
>> dp_netdev_pmd_thread *pmd,
>> bool should_steal, const struct flow *flow,
>> const struct nlattr *actions, size_t actions_len)
>> {
>> - struct dp_netdev_execute_aux aux = { pmd, flow };
>> + struct dp_netdev_execute_aux aux = { pmd, flow, NULL, ODPP_NONE };
>>
>> odp_execute_actions(&aux, packets, should_steal, actions,
>> - actions_len, dp_execute_cb);
>> + actions_len, dp_execute_cb, dp_hash_override);
>> }
>>
>> struct dp_netdev_ct_dump {
>> diff --git a/lib/dpif-offload-dummy.c b/lib/dpif-offload-dummy.c
>> index cc9fc5792..01b20d3b6 100644
>> --- a/lib/dpif-offload-dummy.c
>> +++ b/lib/dpif-offload-dummy.c
>> @@ -640,6 +640,103 @@ dummy_offload_udp_tnl_get_src_port(
>> return true;
>> }
>>
>> +static uint32_t
>> +fnv1a_add(uint32_t hash, uint32_t word)
>> +{
>> + const uint32_t prime = 16777619U;
>> +
>> + for (size_t i = 0; i < sizeof(uint32_t); i++) {
>> + hash ^= (word >> (i * 8)) & 0xff;
>> + hash *= prime;
>> + }
>> + return hash;
>> +}
>> +
>> +static uint32_t
>> +fnv1a_add64(uint32_t hash, uint64_t word)
>> +{
>> + return fnv1a_add(fnv1a_add(hash, word), word >> 32);
>> +}
>> +
>> +static uint32_t
>> +fnv1a_init(uint32_t seed)
>> +{
>> + const uint32_t fnv_offset_basis = 2166136261U;
>> +
>> + return seed ? fnv1a_add(fnv_offset_basis, seed) : fnv_offset_basis;
>> +}
>> +
>> +static bool
>> +get_l4_sym_hash(struct flow *flow, uint32_t seed, uint32_t *hash_)
>> +{
>> + struct ds ds = DS_EMPTY_INITIALIZER;
>> + uint32_t hash = fnv1a_init(seed);
>> +
>> + if (flow->dl_type == htons(ETH_TYPE_IP)) {
>> + hash = fnv1a_add(hash,
>> + (OVS_FORCE uint32_t)(flow->nw_src ^ flow->nw_dst));
>> + } else if (flow->dl_type == htons(ETH_TYPE_IPV6)) {
>> + /* IPv6 addresses are 64-bit aligned inside struct flow. */
>> + const ovs_be64 *a = ALIGNED_CAST(const ovs_be64 *,
>> + flow->ipv6_src.s6_addr);
>> + const ovs_be64 *b = ALIGNED_CAST(const ovs_be64 *,
>> + flow->ipv6_dst.s6_addr);
>> +
>> + for (int i = 0; i < sizeof flow->ipv6_src / sizeof *a; i++) {
>> + hash = fnv1a_add64(hash, ntohll(a[i]) ^ ntohll(b[i]));
>> + }
>> + } else {
>> + return false;
>> + }
>> +
>> + hash = fnv1a_add(hash, flow->nw_proto);
>> + if (!(flow->nw_frag & FLOW_NW_FRAG_MASK)
>> + && (flow->nw_proto == IPPROTO_TCP
>> + || flow->nw_proto == IPPROTO_SCTP
>> + || flow->nw_proto == IPPROTO_UDP)) {
>> + hash = fnv1a_add(hash,
>> + (OVS_FORCE uint16_t)(flow->tp_src ^ flow->tp_dst));
>> + }
>> +
>> + ds_put_format(&ds, "l4_sym_hash: %8.8x for packet: ", hash);
>> + flow_format(&ds, flow, NULL);
>> + VLOG_DBG("%s", ds_cstr(&ds));
>> + ds_destroy(&ds);
>> +
>> + *hash_ = hash;
>> + return true;
>> +}
>
> I wonder if we need all this machinery for the hash computation? Can't we
> just return a fixed number? Or a simple sum of port numbers? This will be
> enough for most test scenarios, as we can ensure that we're setting the ports
> in those scenarios. It's a dummy implementation, it doesn't have to be good.
Fair point. It was fun to write, but I will simplify it in v2.
[...]
>> +AT_CHECK([ovs-appctl netdev-dummy/receive p1 $packet])
>> +AT_CHECK([ovs-appctl netdev-dummy/receive p1 $packet])
>> +
>> +AT_CHECK([ovs-appctl dpctl/dump-flows -m | strip_hw_offload], [0], [dnl
>
> Is -m needed here?
Yes, the -m flag is needed to include the offloaded:yes and dp:dummy
fields in the output.
[...]
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
