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. > > 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. > 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. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
