On Thu, Jul 15, 2021 at 09:36:12PM +0530, 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>
>
> ---
> 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");
> + 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)) {
> + 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]);
> + 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)) {
> + ds_put_format(&reply,
> + "Error: Invalid study_pkt_cnt value: %s.\n", argv[1]);
Ian flagged this one, but there is another missing spot below.
> + goto error;
> + }
> +
> + argc -= 1;
> + argv += 1;
> + } else {
> + ds_put_format(&reply, "Error: unknown argument %s.\n", argv[1]);
> + goto error;
> + 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");
> + goto error;
> + }
>
> - 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",
> + mfex_name);
Here. Error: miniflow extract...
AppVeyor is ok, notice no regression, study works and selects the
correct traffic profile for UDP and TCP, testsuite ok, dpdk
testsuite ok.
Acked-by: Flavio Leitner <f...@sysclose.org>
> + goto error;
> + } else if (err) {
> + ds_put_format(&reply,
> + "Error: unknown miniflow extract implementation %s.",
> + mfex_name);
> + 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);
> } else {
> - error_desc = "Miniflow extract implementation Error:";
> + ds_put_format(&reply,
> + "Error: unknown miniflow extract implementation %s.",
> + mfex_name);
> }
> - 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)) {
> + 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
>
--
fbl
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
