On 13 Jul 2021, at 7:32, Kumar Amber wrote:
> 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]> > > --- > 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 | 25 +++++- > lib/dpif-netdev-private-extract.h | 9 ++ > lib/dpif-netdev.c | 128 +++++++++++++++++++++++++-- > 4 files changed, 187 insertions(+), 12 deletions(-) > > diff --git a/Documentation/topics/dpdk/bridge.rst > b/Documentation/topics/dpdk/bridge.rst > index 0fa9341ac..662446401 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 > +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 Guess you forgot about this one: “”” > Should study() just be study? > Changed “” > +differing packet counts will use the most recent count value provided by > usser. > + > +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 > +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:: > + > + $ 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 eddb35682..61260cb70 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; Why is there an extra space added before mfex_study_pkts_count? > /* 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. 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)) { > + > + mfex_study_pkts_count = pkt_cmp_count; Where did the atomic store go? > + > + return 0; > + } > + > + return -EINVAL; > +} > + > uint32_t > mfex_study_traffic(struct dp_packet_batch *packets, > struct netdev_flow_key *keys, > @@ -86,7 +107,7 @@ 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) { > + if (stats->pkt_count >= mfex_study_pkts_count) { Your previous command: “”” Changed to atomic variable type for mfex_study_pkts_count and hence can be kept like it is . “”” This need to be read atomically, atomic_read_relaxed(), even when defined as atomic. > 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 968952625..1132a0ad5 100644 > --- a/lib/dpif-netdev.c > +++ b/lib/dpif-netdev.c > @@ -1076,19 +1076,103 @@ 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; > + bool pmd_core_param_set = false; > + struct ds reply = DS_EMPTY_INITIALIZER; > + > + if (strcmp("-pmd", argv[1]) == 0) { > + mfex_name = argv[3]; So here only set mfex_name if argc >= 4, else set to NULL > + pmd_core_param_set = true; > + Don’t use argv[2] below / [3] above if you have not checked the argc count. > + if (argv[2] == NULL) { This access garbage data, instead you should user argc here. > + ds_put_format(&reply, > + "Error: Pls provide Pmd core number"); Use PMD all uppercase > + const char *reply_str = ds_cstr(&reply); > + VLOG_INFO("%s", reply_str); > + unixctl_command_reply_error(conn, reply_str); > + ds_destroy(&reply); > + return; > + } > + } else { > + 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 { > + /* 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: '%s'. Pass a valid pmd thread ID.\n", > + argv[2]); I think we really need a goto error; here, as all the below code is copied. Just define the reply globally. > + const char *reply_str = ds_cstr(&reply); > + VLOG_INFO("%s", reply_str); > + unixctl_command_reply_error(conn, reply_str); > + ds_destroy(&reply); > + return; > + } > + } > + > + /* argv[4] is optional packet count, which user can provide along with This is not always argv[4] can also be arg[2] > + * 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 ((!pms_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); > + const char *reply_str = ds_cstr(&reply); > + unixctl_command_reply_error(conn, reply_str); > + VLOG_INFO("%s", reply_str); > + ds_destroy(&reply); > + return; > + } > + } else { > + ds_put_format(&reply, "Invalid study_pkt_cnt value: %s.\n", > + study_cnt_param); > + const char *reply_str = ds_cstr(&reply); > + unixctl_command_reply_error(conn, reply_str); > + VLOG_INFO("%s", reply_str); > + ds_destroy(&reply); > + return; > + } > + } else { > + /* Default packet compare count when packets count not * provided. */ Remove the * > + study_ret = mfex_set_study_pkt_cnt(MFEX_MAX_PKT_COUNT, mfex_name); This is wrong, we should only set it if the actual implementation is study! > + } > > int err = dp_mfex_impl_set_default_by_name(mfex_name); The previous comment was not act upon/replied to: “”” > Should we call dp_mfex_impl_set_default_by_name() if we only set a single > PMD? I do not think, as we change the default for all threads not just this specific one. “”” > > 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:"; > @@ -1123,8 +1207,14 @@ dpif_miniflow_extract_impl_set(struct unixctl_conn > *conn, int argc OVS_UNUSED, > continue; > } > > + if ((pmd_thread_specified) && > + (pmd->core_id != pmd_thread_to_change)) { > + continue; > + } > + > /* 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); > }; > @@ -1132,10 +1222,31 @@ 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) { As mentioned the previous review, this error needs to move up before we change the default value, i.e. before dp_mfex_impl_set_default_by_name() > + 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); > + const char *reply_str = ds_cstr(&reply); > + VLOG_INFO("%s", reply_str); > + unixctl_command_reply_error(conn, reply_str); > + ds_destroy(&reply); > + return; > + } > + > /* Reply with success to command. */ > - struct ds reply = DS_EMPTY_INITIALIZER; > ds_put_format(&reply, "Miniflow Extract implementation set to %s", > mfex_name); > + 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"); > + > const char *reply_str = ds_cstr(&reply); > VLOG_INFO("%s", reply_str); > unixctl_command_reply(conn, reply_str); > @@ -1372,8 +1483,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
