Hi Eelco, Flavio , Pls find my comments Inline.
<SNIP> > > + /* For the first call, this will be choosen based on the > > + * compile time flag and if nor flag is set it is set to > > + * default scalar. > > + */ > > + if (OVS_UNLIKELY(!default_mfex_func_set)) { > > + VLOG_INFO("Default MFEX implementation is %s.\n", > > + mfex_impls[mfex_idx].name); > > + atomic_store_relaxed(mfex_func, (uintptr_t) mfex_impls > > + [mfex_idx].extract_func); > > + default_mfex_func_set = true; > > If this only needs to be done once, why not move it to > dpif_miniflow_extract_init() as suggested during the v6 review (in a later > patch)? > This will remove this check every time dp_mfex_impl_get_default() gets called. > Yes it does make . > > + } > > + > > + return default_mfex_func; > > +} > > + > > +int > > +dp_mfex_impl_set_default_by_name(const char *name) { > > + miniflow_extract_func new_default; > > + atomic_uintptr_t *mfex_func = (void *)&default_mfex_func; > > + > > + int err = dp_mfex_impl_get_by_name(name, &new_default); > > + > > + if (!err) { > > + atomic_store_relaxed(mfex_func, (uintptr_t) new_default); > > + } > > + > > + return err; > > + > > +} > > + > > +void > > +dp_mfex_impl_get(struct ds *reply, struct dp_netdev_pmd_thread > **pmd_list, > > + size_t pmd_list_size) { > > + /* Add all MFEX functions to reply string. */ > > + ds_put_cstr(reply, "Available MFEX implementations:\n"); > > + > > + for (int i = 0; i < MFEX_IMPL_MAX; i++) { > > + ds_put_format(reply, " %s (available: %s pmds: ", > > + mfex_impls[i].name, mfex_impls[i].available ? > > + "True" : "False"); > > Flavio mentioned that True/False did not make sense to an end-user, not sure > if > he has the same feeling here? > Maybe yes/no make more sense here? Flavio? > Changes to available same as previous comments. > > + > > + for (size_t j = 0; j < pmd_list_size; j++) { > > + struct dp_netdev_pmd_thread *pmd = pmd_list[j]; > > + if (pmd->core_id == NON_PMD_CORE_ID) { > > + continue; > > + } > > + > > + if (pmd->miniflow_extract_opt == mfex_impls[i].extract_func) { > > + ds_put_format(reply, "%u,", pmd->core_id); > > + } > > + } > > + > > + ds_chomp(reply, ','); > > + > > + if (ds_last(reply) == ' ') { > > + ds_put_cstr(reply, "none"); > > + } > > + > > + ds_put_cstr(reply, ")\n"); > > + } > > + > > +} > > + > > +/* This function checks all available MFEX implementations, and > > +selects and > > + * returns the function pointer to the one requested by "name". If > > +nothing > > + * is found it reutrns error. > > reutrns -> returns > Fixed. > > + */ > > +int > > +dp_mfex_impl_get_by_name(const char *name, miniflow_extract_func > > +*out_func) { > > + if ((name == NULL) || (out_func == NULL)) { > > + return -EINVAL; > > + } > > + > > + for (int i = 0; i < MFEX_IMPL_MAX; i++) { > > + if (strcmp(mfex_impls[i].name, name) == 0) { > > + /* Check available is set before exec. */ > > + if (!mfex_impls[i].available) { > > + *out_func = NULL; > > + return -ENODEV; > > + } > > + > > + *out_func = mfex_impls[i].extract_func; > > + return 0; > > + } > > + } > > + > > + return -ENOENT; > > +} > > diff --git a/lib/dpif-netdev-private-extract.h > > b/lib/dpif-netdev-private-extract.h > > new file mode 100644 > > index 000000000..ddf2e2845 > > --- /dev/null > > +++ b/lib/dpif-netdev-private-extract.h > > @@ -0,0 +1,111 @@ > > +/* > > + * Copyright (c) 2021 Intel. > > + * > > + * Licensed under the Apache License, Version 2.0 (the "License"); > > + * you may not use this file except in compliance with the License. > > + * You may obtain a copy of the License at: > > + * > > + * http://www.apache.org/licenses/LICENSE-2.0 > > + * > > + * Unless required by applicable law or agreed to in writing, > > +software > > + * distributed under the License is distributed on an "AS IS" BASIS, > > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or > implied. > > + * See the License for the specific language governing permissions > > +and > > + * limitations under the License. > > + */ > > + > > +#ifndef MFEX_AVX512_EXTRACT > > +#define MFEX_AVX512_EXTRACT 1 > > + > > +#include <sys/types.h> > > + > > +/* Forward declarations. */ > > +struct dp_packet; > > +struct miniflow; > > +struct dp_netdev_pmd_thread; > > +struct dp_packet_batch; > > +struct netdev_flow_key; > > + > > +/* Function pointer prototype to be implemented in the optimized > > +miniflow > > + * extract code. > > + * returns the hitmask of the processed packets on success. > > + * returns zero on failure. > > + */ > > +typedef uint32_t (*miniflow_extract_func)(struct dp_packet_batch *batch, > > + struct netdev_flow_key *keys, > > + uint32_t keys_size, > > + odp_port_t in_port, > > + struct dp_netdev_pmd_thread > > + *pmd_handle); > > + > > + > > +/* The function pointer miniflow_extract_func depends on batch size. > > +*/ BUILD_ASSERT_DECL(NETDEV_MAX_BURST == 32); > > + > > +/* Probe function is used to detect if this CPU has the ISA required > > + * to run the optimized miniflow implementation. > > + * returns one on successful probe. > > + * returns negative errno on failure. > > + */ > > +typedef int (*miniflow_extract_probe)(void); > > + > > +/* Structure representing the attributes of an optimized > > +implementation. */ struct dpif_miniflow_extract_impl { > > + /* When it is true, this impl has passed the probe() checks. */ > > + bool available; > > + > > + /* Probe function is used to detect if this CPU has the ISA required > > + * to run the optimized miniflow implementation. It is optional and > > + * if it is not used, then it must be null. > > + */ > > + miniflow_extract_probe probe; > > + > > + /* Optional function to call to extract miniflows for a burst of > > packets. > > + * If it is not used must be set to NULL; > > + */ > > + miniflow_extract_func extract_func; > > + > > + /* Name of the optimized implementation. */ > > + char *name; > > +}; > > + > > + > > +/* Enum to hold implementation indexes. The list is traversed > > + * linearly as from the ISA perspective, the VBMI version > > + * should always come before the generic AVX512-F version. > > + */ > > +enum dpif_miniflow_extract_impl_idx { > > + MFEX_IMPL_SCALAR, > > + MFEX_IMPL_MAX > > +}; > > + > > +/* This function returns all available implementations to the caller. > > +The > > + * quantity of implementations is returned by the int return value. > > + */ > > +void > > +dp_mfex_impl_get(struct ds *reply, struct dp_netdev_pmd_thread > **pmd_list, > > + size_t pmd_list_size) OVS_REQUIRES(dp_netdev_mutex); > > + > > +/* This function checks all available MFEX implementations, and > > +selects the > > + * returns the function pointer to the one requested by "name". > > + */ > > +int > > +dp_mfex_impl_get_by_name(const char *name, miniflow_extract_func > > +*out_func); > > + > > +/* Returns the default MFEX which is first ./configure selected, but > > +can be > > + * overridden at runtime. */ > > +miniflow_extract_func dp_mfex_impl_get_default(void); > > + > > +/* Overrides the default MFEX with the user set MFEX. */ int > > +dp_mfex_impl_set_default_by_name(const char *name); > > + > > + > > +/* Initializes the available miniflow extract implementations by > > +probing for > > + * the CPU ISA requirements. As the runtime available CPU ISA does > > +not change > > + * and the required ISA of the implementation also does not change, > > +it is safe > > + * to cache the probe() results, and not call probe() at runtime. > > + */ > > +void > > +dpif_miniflow_extract_init(void); > > + > > +#endif /* MFEX_AVX512_EXTRACT */ > > diff --git a/lib/dpif-netdev-private-thread.h > > b/lib/dpif-netdev-private-thread.h > > index ba79c4a0a..a4c092b69 100644 > > --- a/lib/dpif-netdev-private-thread.h > > +++ b/lib/dpif-netdev-private-thread.h > > @@ -27,6 +27,11 @@ > > #include <stdint.h> > > > > #include "cmap.h" > > + > > +#include "dpif-netdev-private-dfc.h" > > +#include "dpif-netdev-private-dpif.h" > > +#include "dpif-netdev-perf.h" > > +#include "dpif-netdev-private-extract.h" > > #include "openvswitch/thread.h" > > > > #ifdef __cplusplus > > @@ -110,6 +115,9 @@ struct dp_netdev_pmd_thread { > > /* Pointer for per-DPIF implementation scratch space. */ > > void *netdev_input_func_userdata; > > > > + /* Function pointer to call for miniflow_extract() functionality. */ > > + ATOMIC(miniflow_extract_func) miniflow_extract_opt; > > + > > struct seq *reload_seq; > > uint64_t last_reload_seq; > > > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index > > 610949f36..b0eb8b186 100644 > > --- a/lib/dpif-netdev.c > > +++ b/lib/dpif-netdev.c > > @@ -45,6 +45,7 @@ > > #include "dpif.h" > > #include "dpif-netdev-lookup.h" > > #include "dpif-netdev-perf.h" > > +#include "dpif-netdev-private-extract.h" > > #include "dpif-provider.h" > > #include "dummy.h" > > #include "fat-rwlock.h" > > @@ -1050,6 +1051,95 @@ dpif_netdev_impl_set(struct unixctl_conn *conn, > int argc OVS_UNUSED, > > ds_destroy(&reply); > > } > > > > +static void > > +dpif_miniflow_extract_impl_get(struct unixctl_conn *conn, int argc > OVS_UNUSED, > > + const char *argv[] OVS_UNUSED, > > + void *aux OVS_UNUSED) { > > + struct ds reply = DS_EMPTY_INITIALIZER; > > + struct shash_node *node; > > + > > + ovs_mutex_lock(&dp_netdev_mutex); > > + SHASH_FOR_EACH (node, &dp_netdevs) { > > + struct dp_netdev_pmd_thread **pmd_list; > > + struct dp_netdev *dp = node->data; > > + size_t n; > > + > > + /* Get PMD threads list, required to get the DPIF impl used by > > each PMD > > + * thread. */ > > + sorted_poll_thread_list(dp, &pmd_list, &n); > > + dp_mfex_impl_get(&reply, pmd_list, n); > > + } > > + ovs_mutex_unlock(&dp_netdev_mutex); > > + unixctl_command_reply(conn, ds_cstr(&reply)); > > + ds_destroy(&reply); > > +} > > + > > +static void > > +dpif_miniflow_extract_impl_set(struct unixctl_conn *conn, int argc, > > This is failing compilation, you need to mark argc as unused: > Fixed. > +dpif_miniflow_extract_impl_set(struct unixctl_conn *conn, int argc > +OVS_UNUSED, > > > > + const char *argv[], void *aux OVS_UNUSED) { > > + /* This function requires just one parameter, the miniflow name. > > + */ > > + const char *mfex_name = argv[1]; > > + struct shash_node *node; > > + > > + ovs_mutex_lock(&dp_netdev_mutex); > > + int err = dp_mfex_impl_set_default_by_name(mfex_name); > > Not sure if we need to lock, as it might not help in the > dp_netdev_configure_pmd() case (see below), and we might need to convert > this to an atomic get/set, and then the lock can be removed here (moved down). > Moved the mutex before shash loop and changes the default get global variable to atomic. > > + > > + if (err) { > > + ovs_mutex_unlock(&dp_netdev_mutex); > > + struct ds reply = DS_EMPTY_INITIALIZER; > > + char *error_desc = NULL; > > + if (err == -EINVAL) { > > + error_desc = "Unknown miniflow extract implementation:"; > > + } else if (err == -ENOENT) { > > + error_desc = "Miniflow extract implementation doesn't exist:"; > > + } else if (err == -ENODEV) { > > + error_desc = "Miniflow extract implementation not available:"; > > + } else { > > + error_desc = "Miniflow extract implementation Error:"; > > + } > > + ds_put_format(&reply, "%s %s.\n", error_desc, mfex_name); > > + const char *reply_str = ds_cstr(&reply); > > + unixctl_command_reply_error(conn, reply_str); > > + VLOG_INFO("%s", reply_str); > > + ds_destroy(&reply); > > + return; > > + } > > + > > + SHASH_FOR_EACH (node, &dp_netdevs) { > > + struct dp_netdev_pmd_thread **pmd_list; > > + struct dp_netdev *dp = node->data; > > + size_t n; > > + > > + sorted_poll_thread_list(dp, &pmd_list, &n); > > + miniflow_extract_func default_func = > > + dp_mfex_impl_get_default(); > > + > > + for (size_t i = 0; i < n; i++) { > > + struct dp_netdev_pmd_thread *pmd = pmd_list[i]; > > + if (pmd->core_id == NON_PMD_CORE_ID) { > > + continue; > > + } > > + > > + /* Initialize MFEX function pointer to the newly configured > > + * default. */ > > + atomic_uintptr_t *pmd_func = (void *) > > &pmd->miniflow_extract_opt; > > + atomic_store_relaxed(pmd_func, (uintptr_t) default_func); > > + }; > > + } > > + > > + ovs_mutex_unlock(&dp_netdev_mutex); > > + > > + /* Reply with success to command. */ > > + struct ds reply = DS_EMPTY_INITIALIZER; > > + ds_put_format(&reply, "Miniflow implementation set to %s.\n", > > + mfex_name); > > You missed my previous comment, which you where supposed to change: > > “”” > > In other places we use "Miniflow Extract implementation", so I think > > we should use the same here, i.e. “Miniflow Extract implementation set > > to %s.”. Even with this we still use MFEX and "Miniflow Extract", > > maybe we should consolidate to use either one? I think "Miniflow > > Extract" might be the best as MFEX might confuse users? > > > > Using Miniflow extract implementation as standard. > “”” > Sorry Eelco Fixed . > > + const char *reply_str = ds_cstr(&reply); > > + unixctl_command_reply(conn, reply_str); > > + VLOG_INFO("%s", reply_str); > > + ds_destroy(&reply); > > +} > > + > > static void > > dpif_netdev_pmd_rebalance(struct unixctl_conn *conn, int argc, > > const char *argv[], void *aux OVS_UNUSED) > > @@ -1279,6 +1369,13 @@ dpif_netdev_init(void) > > unixctl_command_register("dpif-netdev/dpif-impl-get", "", > > 0, 0, dpif_netdev_impl_get, > > NULL); > > + unixctl_command_register("dpif-netdev/miniflow-parser-set", > > + "miniflow_implementation_name", > > + 1, 1, dpif_miniflow_extract_impl_set, > > + NULL); > > + unixctl_command_register("dpif-netdev/miniflow-parser-get", "", > > + 0, 0, dpif_miniflow_extract_impl_get, > > + NULL); > > return 0; > > } > > > > @@ -1480,6 +1577,8 @@ create_dp_netdev(const char *name, const struct > > dpif_class *class, > > > > dp->conntrack = conntrack_init(); > > > > + dpif_miniflow_extract_init(); > > + > > atomic_init(&dp->emc_insert_min, DEFAULT_EM_FLOW_INSERT_MIN); > > atomic_init(&dp->tx_flush_interval, DEFAULT_TX_FLUSH_INTERVAL); > > > > @@ -6222,6 +6321,11 @@ dp_netdev_configure_pmd(struct > dp_netdev_pmd_thread *pmd, struct dp_netdev *dp, > > atomic_uintptr_t *pmd_func = (void *) &pmd->netdev_input_func; > > atomic_init(pmd_func, (uintptr_t) default_func); > > > > + /* Init default miniflow_extract function */ > > + miniflow_extract_func mfex_func = dp_mfex_impl_get_default(); > > Not sure if the dp_netdev_configure_pmd() path holds the dp_netdev_mutex. I > do not think it does, if it does not, we have to make sure the > dp_mfex_impl_get/set APIs become atomic. > Get default uses atomic set and get inside API. > > + atomic_uintptr_t *pmd_func_mfex = (void *)&pmd->miniflow_extract_opt; > > + atomic_store_relaxed(pmd_func_mfex, (uintptr_t) mfex_func); > > + > > /* init the 'flow_cache' since there is no > > * actual thread created for NON_PMD_CORE_ID. */ > > if (core_id == NON_PMD_CORE_ID) { @@ -6872,6 +6976,7 @@ > > dfc_processing(struct dp_netdev_pmd_thread *pmd, > > } > > > > miniflow_extract(packet, &key->mf); > > + > > Guess this newline can be removed. > > > key->len = 0; /* Not computed yet. */ > > key->hash = > > (md_is_valid == false) > > -- > > 2.25.1 _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev