| Message ID | 20220524113837.1689610-3-kumar.amber@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | DPIF AVX512 Recirculation | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
> -----Original Message----- > From: Amber, Kumar <kumar.amber@intel.com> > Sent: Tuesday, May 24, 2022 12:39 PM > To: ovs-dev@openvswitch.org > Cc: fbl@sysclose.org; echaudro@redhat.com; i.maximets@ovn.org; Van Haaren, > Harry <harry.van.haaren@intel.com>; Ferriter, Cian <cian.ferriter@intel.com>; > Stokes, Ian <ian.stokes@intel.com>; Amber, Kumar <kumar.amber@intel.com> > Subject: [PATCH v2 2/5] dpif-netdev: Refactor hash function to own header. > > The refactor allows us to use hash function accross > multiple files which was earlier restricted to > dpif-netdev.c only. > > Signed-off-by: Kumar Amber <kumar.amber@intel.com> > Signed-off-by: Cian Ferriter <cian.ferriter@intel.com> > Co-authored-by: Cian Ferriter <cian.ferriter@intel.com> <snip> > -static inline uint32_t > -dpif_netdev_packet_get_rss_hash(struct dp_packet *packet, > - const struct miniflow *mf) > -{ > - uint32_t hash, recirc_depth; > - > - if (OVS_LIKELY(dp_packet_rss_valid(packet))) { > - hash = dp_packet_get_rss_hash(packet); > - } else { > - hash = miniflow_hash_5tuple(mf, 0); > - dp_packet_set_rss_hash(packet, hash); > - } > - > - /* The RSS hash must account for the recirculation depth to avoid > - * collisions in the exact match cache */ > - recirc_depth = *recirc_depth_get_unsafe(); > - if (OVS_UNLIKELY(recirc_depth)) { > - hash = hash_finish(hash, recirc_depth); > - } > - return hash; > -} Somehow, UBSan builds flag a "NULL pointer load" here, however it is not reproducible in a normal (no -fsanitize enabled) build, nor does OVS segfault due to a NULL ptr load. Code inspection shows that the recirc_depth value is always written to before executing the above code from the dpif-netdev datapath. So I think the Asan/UBSan tools are getting confused around static variables, TLS, and the various tricks done to optimize the access, but test and verify please. > - > struct packet_batch_per_flow { > unsigned int byte_count; > uint16_t tcp_flags; > @@ -8500,11 +8476,12 @@ dp_netdev_input(struct dp_netdev_pmd_thread > *pmd, > return 0; > } > > -static void > +int32_t > dp_netdev_recirculate(struct dp_netdev_pmd_thread *pmd, > struct dp_packet_batch *packets) > { > dp_netdev_input__(pmd, packets, true, 0); > + return 0; > } Perhaps rework this function signature change into the later patch: "dpif-netdev: Add function pointer for dpif re-circulate"? This change is not related to refactoring hash functions, so is not expected here.
diff --git a/lib/dpif-netdev-private-dpcls.h b/lib/dpif-netdev-private-dpcls.h index 0d5da73c7..f13088ce8 100644 --- a/lib/dpif-netdev-private-dpcls.h +++ b/lib/dpif-netdev-private-dpcls.h @@ -25,6 +25,7 @@ #include "cmap.h" #include "openvswitch/thread.h" +#include "dpif-netdev-private-dpif.h" #ifdef __cplusplus extern "C" { @@ -124,6 +125,28 @@ dpif_netdev_packet_get_rss_hash_orig_pkt(struct dp_packet *packet, return hash; } +static inline uint32_t +dpif_netdev_packet_get_rss_hash(struct dp_packet *packet, + const struct miniflow *mf) +{ + uint32_t hash, recirc_depth; + + if (OVS_LIKELY(dp_packet_rss_valid(packet))) { + hash = dp_packet_get_rss_hash(packet); + } else { + hash = miniflow_hash_5tuple(mf, 0); + dp_packet_set_rss_hash(packet, hash); + } + + /* The RSS hash must account for the recirculation depth to avoid + * collisions in the exact match cache */ + recirc_depth = *recirc_depth_get_unsafe(); + if (OVS_UNLIKELY(recirc_depth)) { + hash = hash_finish(hash, recirc_depth); + } + return hash; +} + /* Allow other implementations to call dpcls_lookup() for subtable search. */ bool dpcls_lookup(struct dpcls *cls, const struct netdev_flow_key *keys[], diff --git a/lib/dpif-netdev-private-dpif.h b/lib/dpif-netdev-private-dpif.h index 15f1f36b3..958669b32 100644 --- a/lib/dpif-netdev-private-dpif.h +++ b/lib/dpif-netdev-private-dpif.h @@ -81,4 +81,8 @@ dp_netdev_input_outer_avx512(struct dp_netdev_pmd_thread *pmd, struct dp_packet_batch *packets, odp_port_t in_port); +int32_t +dp_netdev_recirculate(struct dp_netdev_pmd_thread *, + struct dp_packet_batch *); + #endif /* netdev-private.h */ diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index b8fd926ad..d095291cd 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -544,8 +544,6 @@ static void dp_netdev_execute_actions(struct dp_netdev_pmd_thread *pmd, const struct flow *flow, const struct nlattr *actions, size_t actions_len); -static void dp_netdev_recirculate(struct dp_netdev_pmd_thread *, - struct dp_packet_batch *); static void dp_netdev_disable_upcall(struct dp_netdev *); static void dp_netdev_pmd_reload_done(struct dp_netdev_pmd_thread *pmd); @@ -7792,28 +7790,6 @@ dp_netdev_upcall(struct dp_netdev_pmd_thread *pmd, struct dp_packet *packet_, actions, wc, put_actions, dp->upcall_aux); } -static inline uint32_t -dpif_netdev_packet_get_rss_hash(struct dp_packet *packet, - const struct miniflow *mf) -{ - uint32_t hash, recirc_depth; - - if (OVS_LIKELY(dp_packet_rss_valid(packet))) { - hash = dp_packet_get_rss_hash(packet); - } else { - hash = miniflow_hash_5tuple(mf, 0); - dp_packet_set_rss_hash(packet, hash); - } - - /* The RSS hash must account for the recirculation depth to avoid - * collisions in the exact match cache */ - recirc_depth = *recirc_depth_get_unsafe(); - if (OVS_UNLIKELY(recirc_depth)) { - hash = hash_finish(hash, recirc_depth); - } - return hash; -} - struct packet_batch_per_flow { unsigned int byte_count; uint16_t tcp_flags; @@ -8500,11 +8476,12 @@ dp_netdev_input(struct dp_netdev_pmd_thread *pmd, return 0; } -static void +int32_t dp_netdev_recirculate(struct dp_netdev_pmd_thread *pmd, struct dp_packet_batch *packets) { dp_netdev_input__(pmd, packets, true, 0); + return 0; } struct dp_netdev_execute_aux {