[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