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

[Van Haaren, Harry]

Hi Eelco,

Thanks for review. Updates with [hvh] prefix below. In general I've fixed-up the suggestions,
with the exception of 1 that I didn't understand or think actually worked.

I'll ask Amber to integrate the changes below into the patchset, next version to ML soon.

Regards, -Harry

From: Eelco Chaudron <echaudro at redhat.com>
Sent: Thursday, July 15, 2021 9:25 AM
To: Amber, Kumar <kumar.amber at intel.com>
Cc: ovs-dev at openvswitch.org; fbl at sysclose.org; i.maximets at ovn.org; Van Haaren, Harry <harry.van.haaren at intel.com>; Ferriter, Cian <cian.ferriter at intel.com>; Stokes, Ian <ian.stokes at intel.com>
Subject: Re: [v12 06/11] dpif-netdev: Add packet count and core id paramters for study


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

From: Kumar Amber <kumar.amber at intel.com<mailto: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<mailto: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.

[hvh] Yes, will do.

+ 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.

[hvh] Yes, will do.



+ 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.

[hvh] This is a coding style topic, and its subjective. As OVS tends more to overloading values inside a single variable (aka NON_PMD_CORE_ID is "magic value") I'll rework to use your suggestion.

+
+ /* 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)

[hvh] Yes, check added as suggested, fixed & done.

+ 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.

[hvh] Yes, removes a variable without "magic value", this one I like, good improvement, thanks.

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

[hvh] It was previously used inside the .c which declares that array. That array isn't available in dpif-netdev.c (nor should it be), so the above is the pragmatic solution. Study is an exception case in this code snippet anyway as it accepts extra args that the other mfex impls don’t.

+ 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.

[hvh] OK done.

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):

[hvh] I'm not following the suggestion here, sorry. A parameter after mfex_name is only valid if mfex_name == "study". The implementation today checks that there is another argument IFF mfex impl == study, and consumes it as "study count" in that case. That's exactly we want?

[hvh] study_count = 0; as per suggestion above, so the else(… && study_count) will always fail, and never execute? Handling argc is the best way to do what we want, I see no issue with it, so leaving as is.

          } else if ((mfex_name && study_count) {
·

o              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) {

[hvh] Check for zero added.

+ 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) {

[hvh] Done as part of rework to 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) {

[hvh] Done as part of rework to mfex_name.

+ 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?

[hvh] updated to be 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) {

[hvh] done

+ 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".

[hvh] Amber addressing these on patchset respin.

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