> Some small things you forgot (also added the one Ian found) guess he can fix > them all up on commit. Also, a lot of indentation issues, guess no one noticed > them earlier :( > > Ian if you are going to fix all of this including indentation issues, you can > add my > ack when committing. If it’s too much change and you request a v15 I’ll review > that and add my ack then. >
Thanks Eelco, I have a few fixes of my own I wanted to add on commit so I can roll these in with them. I'll add your ack with to the patch then (as well as patch 3). Thanks for all the hard work and input reviewing the series. Thanks Ian > //Eelco > > On 15 Jul 2021, at 18:06, kumar Amber wrote: > > > From: Kumar Amber <[email protected]> > > > > This commit introduces additional command line paramter > > for mfex study function. If user provides additional packet out > > it is used in study to compare minimum packets which must be processed > > else a default value is choosen. > > Also introduces a third paramter for choosing a particular pmd core. > > > > $ ovs-appctl dpif-netdev/miniflow-parser-set study 500 3 > > > > Signed-off-by: Kumar Amber <[email protected]> > > > > --- > > v14: > > - fix logging format and xmas ordering > > v13: > > - reowrked the set command as per discussion > > - fixed the atomic set in study > > - added bool for handling study mfex to simplify logic and command output > > - fixed double space in variable declaration and removed static > > v12: > > - re-work the set command to sweep > > - inlcude fixes to study.c and doc changes > > v11: > > - include comments from Eelco > > - reworked set command as per discussion > > v10: > > - fix review comments Eelco > > v9: > > - fix review comments Flavio > > v7: > > - change the command paramters for core_id and study_pkt_cnt > > v5: > > - fix review comments(Ian, Flavio, Eelco) > > - introucde pmd core id parameter > > --- > > --- > > Documentation/topics/dpdk/bridge.rst | 38 +++++- > > lib/dpif-netdev-extract-study.c | 26 +++- > > lib/dpif-netdev-private-extract.h | 9 ++ > > lib/dpif-netdev.c | 175 ++++++++++++++++++++++----- > > 4 files changed, 216 insertions(+), 32 deletions(-) > > > > diff --git a/Documentation/topics/dpdk/bridge.rst > b/Documentation/topics/dpdk/bridge.rst > > index a47153495..8c500c504 100644 > > --- a/Documentation/topics/dpdk/bridge.rst > > +++ b/Documentation/topics/dpdk/bridge.rst > > @@ -284,12 +284,46 @@ command also shows whether the CPU supports > each implementation :: > > > > An implementation can be selected manually by the following command :: > > > > - $ ovs-appctl dpif-netdev/miniflow-parser-set study > > + $ ovs-appctl dpif-netdev/miniflow-parser-set [-pmd core_id] [name] > > + [study_cnt] > > + > > +The above command has two optional parameters: study_cnt and core_id. > > +The core_id sets a particular miniflow extract function to a specific > > +pmd thread on the core. The third parameter study_cnt, which is specific > > +to study and ignored by other implementations, means how many packets > > +are needed to choose the best implementation. > > > > Also user can select the study implementation which studies the traffic for > > a specific number of packets by applying all available implementations of > > miniflow extract and then chooses the one with the most optimal result for > > -that traffic pattern. > > +that traffic pattern. The user can optionally provide an packet count > > +[study_cnt] parameter which is the minimum number of packets that OVS > must > > +study before choosing an optimal implementation. If no packet count is > > +provided, then the default value, 128 is chosen. Also, as there is no > > +synchronization point between threads, one PMD thread might still be > running > > +a previous round, and can now decide on earlier data. > > + > > +The per packet count is a global value, and parallel study executions with > > +differing packet counts will use the most recent count value provided by > > user. > > + > > +Study can be selected with packet count by the following command :: > > + > > + $ ovs-appctl dpif-netdev/miniflow-parser-set study 1024 > > + > > +Study can be selected with packet count and explicit PMD selection > > +by the following command :: > > + > > + $ ovs-appctl dpif-netdev/miniflow-parser-set -pmd 3 study 1024 > > + > > +In the above command the first parameter is the CORE ID of the PMD > > +thread and this can also be used to explicitly set the miniflow > > +extraction function pointer on different PMD threads. > > + > > +Scalar can be selected on core 3 by the following command where > > +study count should not be provided for any implementation other > > +than study :: > > + > > + $ ovs-appctl dpif-netdev/miniflow-parser-set -pmd 3 scalar > > > > Miniflow Extract Validation > > ~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > diff --git a/lib/dpif-netdev-extract-study.c > > b/lib/dpif-netdev-extract-study.c > > index 02b709f8b..4340c9eee 100644 > > --- a/lib/dpif-netdev-extract-study.c > > +++ b/lib/dpif-netdev-extract-study.c > > @@ -25,7 +25,7 @@ > > > > VLOG_DEFINE_THIS_MODULE(dpif_mfex_extract_study); > > > > -static atomic_uint32_t mfex_study_pkts_count = 0; > > +static atomic_uint32_t mfex_study_pkts_count = MFEX_MAX_PKT_COUNT; > > > > /* Struct to hold miniflow study stats. */ > > struct study_stats { > > @@ -48,6 +48,25 @@ mfex_study_get_study_stats_ptr(void) > > return stats; > > } > > > > +int > > +mfex_set_study_pkt_cnt(uint32_t pkt_cmp_count, const char *name) > > +{ > > + struct dpif_miniflow_extract_impl *miniflow_funcs; > > + miniflow_funcs = dpif_mfex_impl_info_get(); > > + > > + /* If the packet count is set and implementation called is study then > > + * set packet counter to requested number else return -EINVAL. > > + */ > > + if ((strcmp(miniflow_funcs[MFEX_IMPL_STUDY].name, name) == 0) && > > + (pkt_cmp_count != 0)) { > > + > > + atomic_store_relaxed(&mfex_study_pkts_count, pkt_cmp_count); > > + return 0; > > + } > > + > > + return -EINVAL; > > +} > > + > > uint32_t > > mfex_study_traffic(struct dp_packet_batch *packets, > > struct netdev_flow_key *keys, > > @@ -86,7 +105,10 @@ mfex_study_traffic(struct dp_packet_batch *packets, > > /* Choose the best implementation after a minimum packets have been > > * processed. > > */ > > - if (stats->pkt_count >= MFEX_MAX_PKT_COUNT) { > > + uint32_t study_cnt_pkts; > > + atomic_read_relaxed(&mfex_study_pkts_count, &study_cnt_pkts); > > + > > + if (stats->pkt_count >= study_cnt_pkts) { > > uint32_t best_func_index = MFEX_IMPL_START_IDX; > > uint32_t max_hits = 0; > > for (int i = MFEX_IMPL_START_IDX; i < MFEX_IMPL_MAX; i++) { > > diff --git a/lib/dpif-netdev-private-extract.h > > b/lib/dpif-netdev-private-extract.h > > index 9c03a1aa0..b93fecbbc 100644 > > --- a/lib/dpif-netdev-private-extract.h > > +++ b/lib/dpif-netdev-private-extract.h > > @@ -151,4 +151,13 @@ mfex_study_traffic(struct dp_packet_batch > *packets, > > uint32_t keys_size, odp_port_t in_port, > > struct dp_netdev_pmd_thread *pmd_handle); > > > > +/* Sets the packet count from user to the stats for use in > > + * study function to match against the classified packets to choose > > + * the optimal implementation. > > + * On error, returns -EINVAL. > > + * On success, returns 0. > > + */ > > +int > > +mfex_set_study_pkt_cnt(uint32_t pkt_cmp_count, const char *name); > > + > > #endif /* MFEX_AVX512_EXTRACT */ > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > > index 707bf85c0..585b9500c 100644 > > --- a/lib/dpif-netdev.c > > +++ b/lib/dpif-netdev.c > > @@ -1076,36 +1076,127 @@ dpif_miniflow_extract_impl_get(struct > unixctl_conn *conn, int argc OVS_UNUSED, > > } > > > > static void > > -dpif_miniflow_extract_impl_set(struct unixctl_conn *conn, int argc > OVS_UNUSED, > > +dpif_miniflow_extract_impl_set(struct unixctl_conn *conn, int argc, > > const char *argv[], void *aux OVS_UNUSED) > > { > > - /* This function requires just one parameter, the miniflow name. > > + /* This command takes some optional and mandatory arguments. The > function > > + * here first parses all of the options, saving results in local > > variables. > > + * Then the parsed values are acted on. > > */ > > - const char *mfex_name = argv[1]; > > + unsigned int pmd_thread_to_change = NON_PMD_CORE_ID; > > + unsigned int study_count = MFEX_MAX_PKT_COUNT; > > + struct ds reply = DS_EMPTY_INITIALIZER; > > + bool pmd_thread_update_done = false; > > + bool mfex_name_is_study = false; > > + const char *mfex_name = NULL; > > + const char *reply_str = NULL; > > struct shash_node *node; > > + int err; > > + > > + while (argc > 1) { > > + /* Optional argument "-pmd" limits the commands actions to just > > this > > + * PMD thread. > > + */ > > + if ((!strcmp(argv[1], "-pmd") && !mfex_name)) { > > + if (argc < 3) { > > + ds_put_format(&reply, > > + "Error: -pmd option requires a thread id argument.\n"); > > Here also the indentation looks wrong. > > > + goto error; > > + } > > + > > + /* Ensure argument can be parsed to an integer. */ > > + if (!str_to_uint(argv[2], 10, &pmd_thread_to_change) || > > + (pmd_thread_to_change == NON_PMD_CORE_ID)) { > Here also the indentation looks wrong. > > > + ds_put_format(&reply, > > + "Error: miniflow extract parser not changed, PMD thread" > > + " passed is not valid: '%s'. Pass a valid pmd thread > > ID.\n", > > + argv[2]); > Here also the indentation looks wrong. > > > + goto error; > > + } > > + > > + argc -= 2; > > + argv += 2; > > + > > + } else if (!mfex_name) { > > + /* Name of MFEX impl requested by user. */ > > + mfex_name = argv[1]; > > + mfex_name_is_study = strcmp("study", mfex_name) == 0; > > + argc -= 1; > > + argv += 1; > > + > > + /* If name is study and more args exist, parse study_count value. > > */ > > + } else if (mfex_name && mfex_name_is_study) { > > + if (!str_to_uint(argv[1], 10, &study_count) || > > + (study_count == 0)) { > > Here also the indentation looks wrong. > > > + ds_put_format(&reply, > > + "Error: Invalid study_pkt_cnt value: %s.\n", argv[1]); > > Lower case “invalid” + indentation > > > + goto error; > > + } > > + > > + argc -= 1; > > + argv += 1; > > + } else { > > + ds_put_format(&reply, "Error: unknown argument %s.\n", > > argv[1]); > > + goto error; > > Indentation > > > + break; > > + } > > + } > > + > > + /* Ensure user passed an MFEX name. */ > > + if (!mfex_name) { > > + ds_put_format(&reply, "Error: no miniflow extract name provided. " > > + "Output of miniflow-parser-get shows implementation > > list.\n"); > > Here also the indentation looks wrong. > > > + goto error; > > + } > > I > > - int err = dp_mfex_impl_set_default_by_name(mfex_name); > > + /* If the MFEX name is "study", set the study packet count. */ > > + if (mfex_name_is_study) { > > + err = mfex_set_study_pkt_cnt(study_count, mfex_name); > > + if (err) { > > + ds_put_format(&reply, "Error: failed to set study count %d for" > > + " miniflow extract implementation %s.\n", > > + study_count, mfex_name); > > + goto error; > > + } > > + } > > > > + /* Set the default MFEX impl only if the command was applied to all PMD > > + * threads. If a PMD thread was selected, do NOT update the default. > > + */ > > + if (pmd_thread_to_change == NON_PMD_CORE_ID) { > > + err = dp_mfex_impl_set_default_by_name(mfex_name); > > + if (err == -ENODEV) { > > + ds_put_format(&reply, > > + "Miniflow extract not available due to CPU ISA requirements: > > %s", > > No Error:, as requested, so “Error: miniflow extract not available due to CPU > ISA > requirements” > > Not sure if the change is within 80 chars, so might need a line break. Also > indentation looks odd. > > > + mfex_name); > > + goto error; > > + } else if (err) { > > + ds_put_format(&reply, > > + "Error: unknown miniflow extract implementation %s.", > > + mfex_name); > > Here also the indentation looks wrong. > > > + goto error; > > + } > > + } > > + > > + /* Get the desired MFEX function pointer and error check its usage. */ > > + miniflow_extract_func mfex_func = NULL; > > + err = dp_mfex_impl_get_by_name(mfex_name, &mfex_func); > > if (err) { > > - 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:"; > > + if (err == -ENODEV) { > > + ds_put_format(&reply, > > + "Error: miniflow extract not available due to CPU ISA " > > + "requirements: %s", mfex_name); > > Indentation look off > > > } else { > > - error_desc = "Miniflow extract implementation Error:"; > > + ds_put_format(&reply, > > + "Error: unknown miniflow extract implementation %s.", > > + mfex_name); > > Indentation looks off > > > } > > - 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; > > + goto error; > > } > > > > + /* Apply the MFEX pointer to each pmd thread in each netdev, filtering > > + * by the users "-pmd" argument if required. > > + */ > > ovs_mutex_lock(&dp_netdev_mutex); > > > > SHASH_FOR_EACH (node, &dp_netdevs) { > > @@ -1114,7 +1205,6 @@ dpif_miniflow_extract_impl_set(struct unixctl_conn > *conn, int argc OVS_UNUSED, > > 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]; > > @@ -1122,23 +1212,51 @@ dpif_miniflow_extract_impl_set(struct > unixctl_conn *conn, int argc OVS_UNUSED, > > continue; > > } > > > > - /* Initialize MFEX function pointer to the newly configured > > - * default. */ > > + /* If -pmd specified, skip all other pmd threads. */ > > + if ((pmd_thread_to_change != NON_PMD_CORE_ID) && > > + (pmd->core_id != pmd_thread_to_change)) { > > indentation > > > + continue; > > + } > > + > > + pmd_thread_update_done = true; > > atomic_uintptr_t *pmd_func = (void *) > > &pmd->miniflow_extract_opt; > > - atomic_store_relaxed(pmd_func, (uintptr_t) default_func); > > + atomic_store_relaxed(pmd_func, (uintptr_t) mfex_func); > > }; > > } > > > > ovs_mutex_unlock(&dp_netdev_mutex); > > > > + /* If PMD thread was specified, but it wasn't found, return error. */ > > + if (pmd_thread_to_change != NON_PMD_CORE_ID && > !pmd_thread_update_done) { > > + ds_put_format(&reply, > > + "Error: miniflow extract parser not changed, " > > + "PMD thread %d not in use, pass a valid pmd" > > + " thread ID.\n", pmd_thread_to_change); > > + goto error; > > + } > > + > > /* Reply with success to command. */ > > - struct ds reply = DS_EMPTY_INITIALIZER; > > - ds_put_format(&reply, "Miniflow Extract implementation set to %s", > > + ds_put_format(&reply, "Miniflow extract implementation set to %s", > > mfex_name); > > - const char *reply_str = ds_cstr(&reply); > > + if (pmd_thread_to_change != NON_PMD_CORE_ID) { > > + ds_put_format(&reply, ", on pmd thread %d", pmd_thread_to_change); > > + } > > + if (mfex_name_is_study) { > > + ds_put_format(&reply, ", studying %d packets", study_count); > > + } > > + ds_put_format(&reply, ".\n"); > > + > > + reply_str = ds_cstr(&reply); > > VLOG_INFO("%s", reply_str); > > unixctl_command_reply(conn, reply_str); > > ds_destroy(&reply); > > + return; > > + > > +error: > > + reply_str = ds_cstr(&reply); > > + VLOG_ERR("%s", reply_str); > > + unixctl_command_reply_error(conn, reply_str); > > + ds_destroy(&reply); > > } > > > > static void > > @@ -1371,8 +1489,9 @@ dpif_netdev_init(void) > > 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, > > + "[-pmd core] miniflow_implementation_name" > > + " [study_pkt_cnt]", > > + 1, 5, dpif_miniflow_extract_impl_set, > > NULL); > > unixctl_command_register("dpif-netdev/miniflow-parser-get", "", > > 0, 0, dpif_miniflow_extract_impl_get, > > -- > > 2.25.1 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
