On 14 Jul 2021, at 12:30, Eelco Chaudron wrote:
> On 14 Jul 2021, at 4:02, kumar Amber wrote: > >> From: Kumar Amber <kumar.am...@intel.com> >> >> 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 <kumar.am...@intel.com> One additional comment, please add some (negative) test cases for the command line options, so we know your changes work. Rather than me having to do this manually every revision. >> --- >> 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 | 37 ++++++- >> lib/dpif-netdev-extract-study.c | 28 ++++- >> lib/dpif-netdev-private-extract.h | 9 ++ >> lib/dpif-netdev.c | 156 ++++++++++++++++++++++----- >> 4 files changed, 201 insertions(+), 29 deletions(-) >> >> diff --git a/Documentation/topics/dpdk/bridge.rst >> b/Documentation/topics/dpdk/bridge.rst >> index a47153495..7860d6173 100644 >> --- a/Documentation/topics/dpdk/bridge.rst >> +++ b/Documentation/topics/dpdk/bridge.rst >> @@ -284,12 +284,45 @@ 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 > > Add space after period. > >> +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 >> usser. > > usser -> 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 last parameter is the CORE ID of the PMD > > This needs a re-write as this is no longer the last parameter. > >> +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 can be put as any arbitrary number or left blank :: > > This is also no longer correct, i.e., study count must NOT be provided. > >> + >> + $ 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..0ed31aa46 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; > > This extra space is still present, see previous review. > >> >> /* Struct to hold miniflow study stats. */ >> struct study_stats { >> @@ -48,6 +48,27 @@ 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 set the packet counter >> + * to default number. > > Also see previous review comment! > > “”” > Guess this comment is not correct, it’s just not set. > “”” > >> + */ >> + if ((strcmp(miniflow_funcs[MFEX_IMPL_STUDY].name, name) == 0) && >> + (pkt_cmp_count != 0)) { >> + >> + atomic_uintptr_t *study_pck_cnt = (void *)&mfex_study_pkts_count; >> + atomic_store_relaxed(study_pck_cnt, (uintptr_t) pkt_cmp_count ); >> + return 0; > > This is an atomic_uint32_t value, why store it as a uintptr? > This will do: > > if ((strcmp(miniflow_funcs[MFEX_IMPL_STUDY].name, name) == 0) && > (pkt_cmp_count != 0)) { > > - atomic_uintptr_t *study_pck_cnt = (void *)&mfex_study_pkts_count; > - atomic_store_relaxed(study_pck_cnt, (uintptr_t) pkt_cmp_count ); > + 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 +107,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) { >> + static uint32_t study_cnt_pkts; >> + atomic_read_relaxed(&mfex_study_pkts_count, &study_cnt_pkts); >> + >> + if (stats->pkt_count >= mfex_study_pkts_count) { > > You should use the read value here, 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..12c38a5a4 100644 >> --- a/lib/dpif-netdev.c >> +++ b/lib/dpif-netdev.c >> @@ -1076,34 +1076,87 @@ 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. >> + /* A First optional paramter PMD thread ID can be also provided which >> + * allows users to set miniflow implementation on a particular pmd. >> + * The second paramter is the name of the function and if -pmd core_id >> + * is not provided this is the first parameter and only mandatory one. >> + * The third and last one is the study-cnt which is only provided for >> + * study function to set the packet count. >> */ >> - const char *mfex_name = argv[1]; >> + const char *mfex_name = NULL; >> struct shash_node *node; >> + int err = 0; >> + bool pmd_core_param_set = false; >> + struct ds reply = DS_EMPTY_INITIALIZER; >> + const char *reply_str = NULL; > > As Harry already mentioned, this code is hard to follow and needs a > re-write/cleanup. > > I do not agree that this should be a separate patch, as all changes are > concentrate in this one. So please do this as part of v12 as you need it > anyway. > > I will also reply to his email. > >> - int err = dp_mfex_impl_set_default_by_name(mfex_name); >> + if ((strcmp("-pmd", argv[1]) == 0) && (argc >= 4)) { >> + mfex_name = argv[3]; >> + pmd_core_param_set = true; >> >> - 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:"; >> + } else if ((strcmp("-pmd", argv[1]) == 0) && (argc < 3)) { >> + ds_put_format(&reply, >> + "Error: Invalid PMD core_id or Null input"); >> + goto error; >> + >> + } else if (argc >= 2) { >> + mfex_name = argv[1]; >> + } >> + >> + bool pmd_thread_specified = false; >> + uint32_t pmd_thread_to_change = 0; >> + bool pmd_thread_update_ok = false; >> + >> + if (pmd_core_param_set) { >> + if (str_to_uint(argv[2], 10, &pmd_thread_to_change)) { >> + pmd_thread_specified = true; >> } else { >> - error_desc = "Miniflow extract implementation Error:"; >> + /* argv[2] isn't even a uint. return without changing anything. >> */ >> + ds_put_format(&reply, >> + "Error: Miniflow parser not changed, PMD thread argument" >> + " passed is not valid: '%d'. Pass a valid pmd thread >> ID.\n", >> + pmd_thread_to_change); >> + goto 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; >> + } >> + >> + /* argv[4]/arg[2] is optional packet count, which user can provide along >> + * with study function to set the minimum packet that must be matched in >> + * order to choose the optimal function. >> + */ >> + uint32_t study_ret = 0; >> + uint32_t pkt_cmp_count = MFEX_MAX_PKT_COUNT; >> + >> + const char *study_cnt_param = NULL; >> + if (argc == 5) { >> + study_cnt_param = argv[4]; >> + } else if ((!pmd_core_param_set) && (argc == 3)) { >> + study_cnt_param = argv[2]; >> + } >> + >> + if (study_cnt_param) { >> + >> + if (str_to_uint(study_cnt_param, 10, &pkt_cmp_count)) { >> + study_ret = mfex_set_study_pkt_cnt(pkt_cmp_count, mfex_name); >> + if (study_ret == -EINVAL) { >> + ds_put_format(&reply, "The study_pkt_cnt option is not >> valid" >> + " for the %s implementation.\n", >> + mfex_name); >> + goto error; >> + } >> + } else { >> + ds_put_format(&reply, "Invalid study_pkt_cnt value: %s.\n", >> + study_cnt_param); >> + goto error; >> + } >> + } else { >> + /* Default packet compare count when packets count not provided. >> + * The Api check if the implementation is study and sets it to >> default >> + * else returns -EINVAl. */ >> + study_ret = mfex_set_study_pkt_cnt(MFEX_MAX_PKT_COUNT, mfex_name); >> } >> >> ovs_mutex_lock(&dp_netdev_mutex); >> @@ -1114,7 +1167,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,8 +1174,38 @@ dpif_miniflow_extract_impl_set(struct unixctl_conn >> *conn, int argc OVS_UNUSED, >> continue; >> } >> >> + if (pmd_core_param_set) { >> + if ((pmd_thread_specified) && >> + (pmd->core_id != pmd_thread_to_change)) { >> + continue; >> + } >> + } >> + >> + /* Set default only when available pmd_core_id was given or >> + * set for all the pmd core_id if no particular core_id was >> + * provided else dont set MFEX default. >> + */ > > This is still wrong we should only set the default if NO pmd core was given. > See discussion with Harry. > >> + err = dp_mfex_impl_set_default_by_name(mfex_name); >> + if (err) { >> + 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); >> + } >> + miniflow_extract_func default_func = dp_mfex_impl_get_default(); >> + >> /* Initialize MFEX function pointer to the newly configured >> * default. */ >> + pmd_thread_update_ok = true; >> atomic_uintptr_t *pmd_func = (void *) >> &pmd->miniflow_extract_opt; >> atomic_store_relaxed(pmd_func, (uintptr_t) default_func); >> }; >> @@ -1131,14 +1213,37 @@ dpif_miniflow_extract_impl_set(struct unixctl_conn >> *conn, int argc OVS_UNUSED, >> >> ovs_mutex_unlock(&dp_netdev_mutex); >> >> + /* If PMD thread was specified, but it wasn't found, return error. */ >> + if (pmd_thread_specified && !pmd_thread_update_ok) { >> + ds_put_format(&reply, >> + "Error: Miniflow 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", >> mfex_name); >> - const char *reply_str = ds_cstr(&reply); >> + if (pmd_thread_specified) { >> + ds_put_format(&reply, ", on pmd thread %d", pmd_thread_to_change); >> + } >> + if (study_ret == 0) { >> + ds_put_format(&reply, ", studying %d packets", pkt_cmp_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; > > Add extra newline > >> +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 +1476,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 d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev