[ovs-dev] [v12 06/11] dpif-netdev: Add packet count and core id paramters for study

Eelco Chaudron echaudro at redhat.com
Thu Jul 15 08:25:10 UTC 2021



On 14 Jul 2021, at 16:14, kumar Amber wrote:

> From: Kumar Amber <kumar.amber at 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.amber at intel.com>
>
> ---
> 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      |  30 ++++-
>  lib/dpif-netdev-private-extract.h    |   9 ++
>  lib/dpif-netdev.c                    | 178 
> ++++++++++++++++++++++-----
>  4 files changed, 222 insertions(+), 33 deletions(-)
>

<SNIP>

> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 707bf85c0..a03a98fd7 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -1076,36 +1076,130 @@ 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];
> +    bool pmd_thread_specified = false;
> +    bool pmd_thread_update_done = false;
> +    uint32_t pmd_thread_to_change = 0;

Change this to unsigned int, as this is the pointer type str_to_uint() 
expects.

> +    bool mfex_name_parsed = false;
> +    bool mfex_name_is_study = false;
> +    const char *mfex_name = NULL;
> +    const char *reply_str = NULL;
> +    struct ds reply = DS_EMPTY_INITIALIZER;
> +    uint32_t study_count = MFEX_MAX_PKT_COUNT;

Change this to unsigned int, as this is the pointer type str_to_uint() 
expects.

> +    int err;
>      struct shash_node *node;
>
> -    int err = dp_mfex_impl_set_default_by_name(mfex_name);
> +    while (argc > 1) {
> +        /* Optional argument "-pmd" limits the commands actions to 
> just this
> +         * PMD thread.
> +         */
> +        if (!strcmp(argv[1], "-pmd")) {
> +            if (argc < 3) {
> +                ds_put_format(&reply,
> +                   "Error: -pmd option requires a thread id 
> argument.\n");
> +                goto error;
> +            }
> +            pmd_thread_specified = true;

We could get rid of the pmd_thread_specified bool by assigning 
pmd_thread_to_change the value NON_PMD_CORE_ID and use that instead.

> +
> +            /* Ensure argument can be parsed to an integer. */
> +            if (!str_to_uint(argv[2], 10, &pmd_thread_to_change)) {

As NON_PMD_CORE_ID is an invalid value, we should check for it.

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 parser not changed, PMD thread 
> argument"
> +                  " passed is not valid: '%s'. Pass a valid pmd 
> thread ID.\n",
> +                  argv[2]);
> +                goto error;
> +            }
> +            argc -= 2;
> +            argv += 2;
> +
> +        } else if (!mfex_name_parsed) {
> +            /* Name of MFEX impl requested by user. */
> +            mfex_name = argv[1];
> +            mfex_name_parsed = true;

As mfex_name is initialized to NULL, you can use that to check if the 
name is set. So you could get rid of the mfex_name_parsed variable.

> +            argc -= 1;
> +            argv += 1;
> +
> +            /* If name is study and more args, parse study_count 
> value. */
> +            if (strncmp("study", mfex_name, 5) == 0) {

Don’t think you need strncmp here, also previously you use the below, 
which is much nicer:

if ((strcmp(miniflow_funcs[MFEX_IMPL_STUDY].name, mfex_name) == 0)

> +                mfex_name_is_study = true;

You might not meed this variable, you could just initialize study_count 
to 0, and here set it to MFEX_MAX_PKT_COUNT.


Below you break again with the idea of doing every parameter in a 
separate case?
I think it should be something like (not tested, and assuming you 
removed mfex_name_is_study and mfex_name_parsed):

               } else if ((mfex_name && study_count) {


- > +                if (argc >= 2) {

> +                    if (!str_to_uint(argv[1], 10, &study_count) ) {

You need to also check for 0, as that is invalid, so:

  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]);
> +                        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_parsed) {

if (!mfex_name) {

> +        ds_put_format(&reply, "Error: no miniflow extract name 
> provided. "
> +                "Output of miniflow-parser-get shows implementation 
> list.\n");
> +        goto error;
> +    }
> +
> +    /* If the MFEX name is "study", set the study packet count. */
> +    if (mfex_name_is_study) {

if (study_count) {

> +        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_specified) {
> +        char *err_str;
> +        err = dp_mfex_impl_set_default_by_name(mfex_name);
> +        if (err == -ENODEV) {
> +            err_str =
> +              "Miniflow extract not available due to CPU ISA 
> requirements:";
> +        } else if (err) {
> +            err_str = "Unknown miniflow extract implementation:";
> +        }
> +        if (err) {
> +            ds_put_format(&reply, "%s %s.\n", err_str, 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,
> +              "Miniflow extract %s not available due to CPU ISA 
> requirements.",
> +              mfex_name);
>          } else {
> -            error_desc = "Miniflow extract implementation Error:";
> +            ds_put_format(&reply,
> +               "Unknown miniflow extract implementation %s.", 
> mfex_name);
>          }

The error message format here is different than above for the same 
errors. Can we make them the same?


> -        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 +1208,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 +1215,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_specified) &&
> +                    (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_specified && !pmd_thread_update_done) {
> +        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",
> +    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 (mfex_name_is_study) {

if (study_count) {

> +        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);
>  }


This concludes the review. When you sent out a v13, can you update the 
commit subject to be according to the guidelines, i.e., add the PATCH 
keyword?

In addition, can you make sure all of them start with a capital letter, 
i.e. patches 4, 8, 9, and 11 do not start with "Add" but with "add".

>  static void
> @@ -1371,8 +1492,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


More information about the dev mailing list