On 14 Jul 2021, at 16:14, kumar Amber wrote:
From: Kumar Amber <[email protected]>
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]>
---
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
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
