| Message ID | ee44f9739c6bcca15b7d0f4ef1a407c9b9ecc6d5.1782242362.git.echaudro@redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Ilya Maximets |
| Headers | show |
| Series | [ovs-dev,v2] dpif-offload: Add support for offload provider specific dp-hash. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_FreeBSD_Build_and_Test | success | github build: passed |
Eelco Chaudron <echaudro@redhat.com> writes: > 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. n> > 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 <echaudro@redhat.com> > --- Acked-by: Aaron Conole <aconole@redhat.com>
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 <echaudro@redhat.com> > --- > lib/dpif-netdev.c | 33 ++++++++++++- > lib/dpif-offload-dummy.c | 98 +++++++++++++++++++++++++++++++++++++ > lib/dpif-offload-provider.h | 12 +++++ > lib/dpif-offload.c | 19 +++++++ > lib/dpif-offload.h | 4 ++ > lib/dpif.c | 2 +- > lib/odp-execute.c | 42 +++++++++++----- > lib/odp-execute.h | 13 ++++- > tests/dpif-netdev.at | 47 ++++++++++++++++++ > 9 files changed, 255 insertions(+), 15 deletions(-) > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > index a9bd27573..3cb34ba1a 100644 > --- a/lib/dpif-netdev.c > +++ b/lib/dpif-netdev.c > @@ -7708,6 +7708,8 @@ dp_netdev_recirculate(struct dp_netdev_pmd_thread *pmd, > struct dp_netdev_execute_aux { > struct dp_netdev_pmd_thread *pmd; > const struct flow *flow; > + struct netdev *cached_in_netdev; > + odp_port_t cached_in_odpp; > }; > > static void > @@ -7954,6 +7956,33 @@ dp_execute_lb_output_action(struct dp_netdev_pmd_thread *pmd, > } > } > > +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. > + > 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. > + > +static bool > +dummy_offload_get_dp_hash(const struct dpif_offload *offload OVS_UNUSED, > + const struct netdev *ingress_netdev OVS_UNUSED, > + struct dp_packet *packet, > + const struct ovs_action_hash *hash_act, > + uint32_t *hash) > +{ > + switch ((enum ovs_hash_alg) hash_act->hash_alg) { > + case OVS_HASH_ALG_L4: > + case OVS_HASH_ALG_SYM_L4: { > + /* For our implementation we will just use a symmetric L4 hash. > + * Note that we will use our own hash that will give consistent results > + * across all platforms/compilers. */ > + struct flow flow; > + > + flow_extract(packet, &flow); > + if (!get_l4_sym_hash(&flow, hash_act->hash_basis, hash)) { > + return false; > + } > + break; > + } > + > + case __OVS_HASH_MAX: > + default: > + OVS_NOT_REACHED(); > + } > + > + return true; > +} > + > static bool > dummy_offload_are_all_actions_supported(const struct dpif_offload *offload_, > odp_port_t in_odp, > @@ -1162,6 +1259,7 @@ dummy_netdev_hw_offload_run(struct netdev *netdev) > .get_netdev = dummy_offload_get_netdev, \ > .netdev_hw_post_process = dummy_offload_hw_post_process, \ > .netdev_udp_tnl_get_src_port = dummy_offload_udp_tnl_get_src_port, \ > + .netdev_get_dp_hash = dummy_offload_get_dp_hash, \ > .netdev_flow_put = dummy_flow_put, \ > .netdev_flow_del = dummy_flow_del, \ > .netdev_flow_stats = dummy_flow_stats, \ > diff --git a/lib/dpif-offload-provider.h b/lib/dpif-offload-provider.h > index 444b13138..5dec51173 100644 > --- a/lib/dpif-offload-provider.h > +++ b/lib/dpif-offload-provider.h > @@ -288,6 +288,18 @@ struct dpif_offload_class { > struct dp_packet *packet, > ovs_be16 *src_port); > > + /* Allows the offload provider to override the default dp-hash calculation. > + * Called during packet processing to determine the hash value for datapath > + * operations (e.g., load balancing, packet distribution). > + * > + * If implemented, should return true and set 'hash' to the desired hash > + * value. If not implemented or if default behavior is desired, should > + * return false to use the standard hash calculation. */ > + 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); > + > /* Add or modify the specified flow directly in the offload datapath. > * The actual implementation may choose to handle the offload > * asynchronously by returning EINPROGRESS and invoking the supplied > diff --git a/lib/dpif-offload.c b/lib/dpif-offload.c > index 587360c51..21215d874 100644 > --- a/lib/dpif-offload.c > +++ b/lib/dpif-offload.c > @@ -1482,6 +1482,25 @@ dpif_offload_netdev_udp_tnl_get_src_port(const struct netdev *ingress_netdev, > packet, src_port); > } > > +bool > +dpif_offload_netdev_get_dp_hash(const struct netdev *ingress_netdev, > + struct dp_packet *packet, > + const struct ovs_action_hash *hash_action, > + uint32_t *hash) > +{ > + const struct dpif_offload *offload; > + > + offload = ovsrcu_get(const struct dpif_offload *, > + &ingress_netdev->dpif_offload); > + > + if (OVS_UNLIKELY(!offload) || !offload->class->netdev_get_dp_hash) { > + return false; > + } > + > + return offload->class->netdev_get_dp_hash(offload, ingress_netdev, packet, > + hash_action, hash); > +} > + > void > dpif_offload_datapath_register_flow_unreference_cb( > struct dpif *dpif, dpif_offload_flow_unreference_cb *cb) > diff --git a/lib/dpif-offload.h b/lib/dpif-offload.h > index bf7643320..5b377de28 100644 > --- a/lib/dpif-offload.h > +++ b/lib/dpif-offload.h > @@ -116,6 +116,10 @@ int dpif_offload_netdev_hw_post_process(struct netdev *, unsigned pmd_id, > bool dpif_offload_netdev_udp_tnl_get_src_port(const struct netdev *, > struct dp_packet *, > ovs_be16 *src_port); > +bool dpif_offload_netdev_get_dp_hash(const struct netdev *, > + struct dp_packet *, > + const struct ovs_action_hash *, > + uint32_t *hash); > > > /* Callback invoked when a hardware flow offload operation (put/del) completes. > diff --git a/lib/dpif.c b/lib/dpif.c > index 1afc7c662..dac5df7b3 100644 > --- a/lib/dpif.c > +++ b/lib/dpif.c > @@ -1308,7 +1308,7 @@ dpif_execute_with_help(struct dpif *dpif, struct dpif_execute *execute) > ofpbuf_init(&aux.meter_actions, 0); > dp_packet_batch_init_packet(&pb, execute->packet); > odp_execute_actions(&aux, &pb, false, execute->actions, > - execute->actions_len, dpif_execute_helper_cb); > + execute->actions_len, dpif_execute_helper_cb, NULL); > ofpbuf_uninit(&aux.meter_actions); > return aux.error; > } > diff --git a/lib/odp-execute.c b/lib/odp-execute.c > index 4642f3375..afde4f8e5 100644 > --- a/lib/odp-execute.c > +++ b/lib/odp-execute.c > @@ -706,7 +706,8 @@ odp_execute_masked_set_action(struct dp_packet *packet, > static void > odp_execute_sample(void *dp, struct dp_packet *packet, bool steal, > const struct nlattr *action, > - odp_execute_cb dp_execute_action) > + odp_execute_cb dp_execute_action, > + odp_hash_cb hash_override) > { > const struct nlattr *subactions = NULL; > const struct nlattr *a; > @@ -756,13 +757,15 @@ odp_execute_sample(void *dp, struct dp_packet *packet, bool steal, > } > dp_packet_batch_init_packet(&pb, packet); > odp_execute_actions(dp, &pb, true, nl_attr_get(subactions), > - nl_attr_get_size(subactions), dp_execute_action); > + nl_attr_get_size(subactions), dp_execute_action, > + hash_override); > } > > static void > odp_execute_clone(void *dp, struct dp_packet_batch *batch, bool steal, > const struct nlattr *actions, > - odp_execute_cb dp_execute_action) > + odp_execute_cb dp_execute_action, > + odp_hash_cb hash_override) > { > if (!steal) { > /* The 'actions' may modify the packet, but the modification > @@ -774,18 +777,21 @@ odp_execute_clone(void *dp, struct dp_packet_batch *batch, bool steal, > dp_packet_batch_clone(&clone_pkt_batch, batch); > dp_packet_batch_reset_cutlen(batch); > odp_execute_actions(dp, &clone_pkt_batch, true, nl_attr_get(actions), > - nl_attr_get_size(actions), dp_execute_action); > + nl_attr_get_size(actions), dp_execute_action, > + hash_override); > } > else { > odp_execute_actions(dp, batch, true, nl_attr_get(actions), > - nl_attr_get_size(actions), dp_execute_action); > + nl_attr_get_size(actions), dp_execute_action, > + hash_override); > } > } > > static void > odp_execute_check_pkt_len(void *dp, struct dp_packet *packet, bool steal, > const struct nlattr *action, > - odp_execute_cb dp_execute_action) > + odp_execute_cb dp_execute_action, > + odp_hash_cb hash_override) > { > static const struct nl_policy ovs_cpl_policy[] = { > [OVS_CHECK_PKT_LEN_ATTR_PKT_LEN] = { .type = NL_A_U16 }, > @@ -842,7 +848,7 @@ odp_execute_check_pkt_len(void *dp, struct dp_packet *packet, bool steal, > * odp_execute_actions. */ > dp_packet_batch_init_packet(&pb, packet); > odp_execute_actions(dp, &pb, true, nl_attr_get(a), nl_attr_get_size(a), > - dp_execute_action); > + dp_execute_action, hash_override); > } > > static bool > @@ -929,7 +935,8 @@ requires_datapath_assistance(const struct nlattr *a) > void > odp_execute_actions(void *dp, struct dp_packet_batch *batch, bool steal, > const struct nlattr *actions, size_t actions_len, > - odp_execute_cb dp_execute_action) > + odp_execute_cb dp_execute_action, > + odp_hash_cb hash_override) > { > struct dp_packet *packet; > const struct nlattr *a; > @@ -989,6 +996,12 @@ odp_execute_actions(void *dp, struct dp_packet_batch *batch, bool steal, > uint32_t hash; > > DP_PACKET_BATCH_FOR_EACH (i, packet, batch) { > + /* Allow the datapath to override the hash. */ > + if (hash_override > + && hash_override(dp, packet, hash_act, &hash)) { > + packet->md.dp_hash = hash; > + continue; > + } > /* RSS hash can be used here instead of 5tuple for > * performance reasons. */ > if (dp_packet_rss_valid(packet)) { > @@ -1007,6 +1020,12 @@ odp_execute_actions(void *dp, struct dp_packet_batch *batch, bool steal, > uint32_t hash; > > DP_PACKET_BATCH_FOR_EACH (i, packet, batch) { > + /* Allow the datapath to override the hash. */ > + if (hash_override > + && hash_override(dp, packet, hash_act, &hash)) { > + packet->md.dp_hash = hash; > + continue; > + } > flow_extract(packet, &flow); > hash = flow_hash_symmetric_l3l4(&flow, > hash_act->hash_basis, > @@ -1052,7 +1071,7 @@ odp_execute_actions(void *dp, struct dp_packet_batch *batch, bool steal, > case OVS_ACTION_ATTR_SAMPLE: > DP_PACKET_BATCH_FOR_EACH (i, packet, batch) { > odp_execute_sample(dp, packet, steal && last_action, a, > - dp_execute_action); > + dp_execute_action, hash_override); > } > > if (last_action) { > @@ -1075,7 +1094,8 @@ odp_execute_actions(void *dp, struct dp_packet_batch *batch, bool steal, > > case OVS_ACTION_ATTR_CLONE: > odp_execute_clone(dp, batch, steal && last_action, a, > - dp_execute_action); > + dp_execute_action, > + hash_override); > if (last_action) { > /* We do not need to free the packets. odp_execute_clone() has > * stolen them. */ > @@ -1134,7 +1154,7 @@ odp_execute_actions(void *dp, struct dp_packet_batch *batch, bool steal, > case OVS_ACTION_ATTR_CHECK_PKT_LEN: > DP_PACKET_BATCH_FOR_EACH (i, packet, batch) { > odp_execute_check_pkt_len(dp, packet, steal && last_action, a, > - dp_execute_action); > + dp_execute_action, hash_override); > } > > if (last_action) { > diff --git a/lib/odp-execute.h b/lib/odp-execute.h > index 7a54fa6ec..8e6395190 100644 > --- a/lib/odp-execute.h > +++ b/lib/odp-execute.h > @@ -24,11 +24,21 @@ > #include "openvswitch/types.h" > > struct nlattr; > +struct dp_packet; > struct dp_packet_batch; > +struct ovs_action_hash; > > typedef void (*odp_execute_cb)(void *dp, struct dp_packet_batch *batch, > const struct nlattr *action, bool should_steal); > > +/* Optional callback that allows a datapath to override the default dp-hash > + * calculation for OVS_ACTION_ATTR_HASH. Called per-packet during hash action > + * processing. Should return true and set '*hash' if the override is applied, > + * or false to fall back to the default software hash. */ > +typedef bool (*odp_hash_cb)(void *dp, struct dp_packet *packet, > + const struct ovs_action_hash *hash_act, > + uint32_t *hash); > + > /* Actions that need to be executed in the context of a datapath are handed > * to 'dp_execute_action', if non-NULL. Currently this is called only for > * actions OVS_ACTION_ATTR_OUTPUT and OVS_ACTION_ATTR_USERSPACE so > @@ -36,7 +46,8 @@ typedef void (*odp_execute_cb)(void *dp, struct dp_packet_batch *batch, > void odp_execute_actions(void *dp, struct dp_packet_batch *batch, > bool steal, > const struct nlattr *actions, size_t actions_len, > - odp_execute_cb dp_execute_action); > + odp_execute_cb dp_execute_action, > + odp_hash_cb hash_override); > > #define odp_get_key_mask(a, type) ((const type *)(const void *)(a + 1) + 1) > > diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at > index 048b826cb..199c85a71 100644 > --- a/tests/dpif-netdev.at > +++ b/tests/dpif-netdev.at > @@ -662,6 +662,53 @@ arp,in_port=ANY,dl_vlan=11,dl_vlan_pcp=7,vlan_tci1=0x0000,dl_src=00:06:07:08:09: > DPIF_NETDEV_FLOW_HW_OFFLOAD_OFFSETS_VID_ARP([dummy]) > DPIF_NETDEV_FLOW_HW_OFFLOAD_OFFSETS_VID_ARP([dummy-pmd]) > > +AT_SETUP([dpif-netdev - partial hw offload - dp_hash - dummy-pmd]) > +OVS_VSWITCHD_START( > + [add-port br0 p1 -- \ > + add-port br0 p2 -- \ > + set interface p1 type=dummy-pmd ofport_request=1 options:ifindex=1100 -- \ > + set interface p2 type=dummy-pmd ofport_request=2 options:ifindex=1200 \ > + options:tx_pcap=p2.pcap -- \ > + set bridge br0 datapath-type=dummy other-config:datapath-id=1234 \ > + fail-mode=secure], [], [], [--dummy-numa="0"]) > +AT_CHECK([ovs-appctl vlog/set dpif_offload_dummy:file:dbg]) > + > +AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:hw-offload=true]) > +OVS_WAIT_UNTIL([grep "Flow HW offload is enabled" ovs-vswitchd.log]) > + > +AT_CHECK([ovs-ofctl -O OpenFlow12 add-group br0 \ > + 'group_id=123,type=select,bucket=output:p2,output:p2']) > +AT_CHECK([ovs-ofctl -O OpenFlow12 add-flow br0 'ip actions=group:123']) > + > +packet="in_port(1),"\ > +"eth(src=50:54:00:00:00:07,dst=50:54:00:00:00:1),eth_type(0x0800),"\ > +"ipv4(src=192.168.1.1,dst=192.168.1.100,proto=6,tos=0,ttl=128,frag=no),"\ > +"tcp(src=1000,dst=1000)" > + > +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? > +recirc_id(0),in_port(p1),packet_type(ns=0,id=0),eth(),eth_type(0x0800),ipv4(),tcp(src=1000/0,dst=1000/0), packets:1, bytes:118, used:0.0s, offloaded:partial, dp:ovs, actions:hash(sym_l4(0)),recirc(0x1) > +recirc_id(0x1),dp_hash(0x34a08190/0xf),in_port(p1),packet_type(ns=0,id=0),eth(),eth_type(0x0800),ipv4(),tcp(src=1000/0,dst=1000/0), packets:1, bytes:118, used:0.0s, offloaded:yes, dp:dummy, actions:p2 > +]) Best regards, Ilya Maximets.
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 <echaudro@redhat.com> [...] >> +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. [...]
Eelco Chaudron <echaudro@redhat.com> 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 <echaudro@redhat.com> > > [...] > > >> +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 i
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 <echaudro@redhat.com> --- lib/dpif-netdev.c | 33 ++++++++++++- lib/dpif-offload-dummy.c | 98 +++++++++++++++++++++++++++++++++++++ lib/dpif-offload-provider.h | 12 +++++ lib/dpif-offload.c | 19 +++++++ lib/dpif-offload.h | 4 ++ lib/dpif.c | 2 +- lib/odp-execute.c | 42 +++++++++++----- lib/odp-execute.h | 13 ++++- tests/dpif-netdev.at | 47 ++++++++++++++++++ 9 files changed, 255 insertions(+), 15 deletions(-)