Hi Flavio,
> -----Original Message-----
> From: Flavio Leitner <[email protected]>
> Sent: Thursday, July 15, 2021 8:28 PM
> To: Eelco Chaudron <[email protected]>
> Cc: Amber, Kumar <[email protected]>; [email protected];
> [email protected]; Van Haaren, Harry <[email protected]>;
> Ferriter, Cian <[email protected]>; Stokes, Ian <[email protected]>
> Subject: Re: [PATCH v13 06/11] dpif-netdev: Add packet count and core id
> paramters for study
>
> On Thu, Jul 15, 2021 at 04:15:13PM +0200, Eelco Chaudron wrote:
> >
> > Some minor changes in output, maybe they can be done during the commit?
> >
> > On 15 Jul 2021, at 14:42, 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]>
> > >
> > > ---
> > > 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 | 27 ++++-
> > > lib/dpif-netdev-private-extract.h | 9 ++
> > > lib/dpif-netdev.c | 173 ++++++++++++++++++++++-----
> > > 4 files changed, 215 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..7725c8f6e 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,26 @@ 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 or set the packet counter
> > > + * to default number else return -EINVAL.
> > > + */
>
> The comment above says about setting to the default number, but there is no
> such condition here.
>
Yes removed that part of comment.
>
> > > + 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 +106,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..519f6f21f 100644
> > > --- a/lib/dpif-netdev.c
> > > +++ b/lib/dpif-netdev.c
> > > @@ -1076,36 +1076,125 @@ 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;
> > > + bool pmd_thread_update_done = false;
> > > + const char *mfex_name = NULL;
> > > + const char *reply_str = NULL;
> > > + struct ds reply = DS_EMPTY_INITIALIZER;
> > > + unsigned int study_count = MFEX_MAX_PKT_COUNT;
> > > + bool mfex_name_is_study = false;
> > > + int err;
> > > struct shash_node *node;
> >
> > These are not in Reverse XMAS tree order, I think Flavio requested
> > this earlier. Flavio?
>
> That is what we usually ask for, but on another hand I don't think we have
> that in
> the coding style document. It would be great to have that fixed as well.
>
>
> Otherwise with Eelco's fixes I have no other comments.
>
I have fixed the xmas order in revision.
> fbl
>
>
> > > - 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")) {
> >
> > In the last comment I suggested to also check if we have not already
> > parsed the name to avoid an off error like:
> >
> > $ ovs-appctl dpif-netdev/miniflow-parser-set -pmd 1 hallo -pmd
> > Error: -pmd option requires a thread id argument.
> > ovs-appctl: ovs-vswitchd: server returned an error
> >
> > So
> >
> > 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 parser not changed, PMD thread
> > > argument"
> >
> > We miss the “extract” part here (and do we need capital M as it’s not
> > the case elsewhere), i.e., "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]);
> > > + 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;
> > > + }
> > > +
> > > + /* 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);
> >
> > All other error messages have “Error: miniflow...” this one does no,
> > also the other three below.
> >
> > > + goto error;
> > > + } else if (err) {
> > > + ds_put_format(&reply,
> > > + "Unknown miniflow extract implementation %s.",
> > > mfex_name);
> >
> > “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,
> > > + "Miniflow extract not available due to CPU ISA
> > > requirements: %s",
> >
> > “Error: miniflow extract not available due to CPU ISA
> > requirements: %s",
> >
> >
> > > + mfex_name);
> > > } else {
> > > - error_desc = "Miniflow extract implementation Error:";
> > > + ds_put_format(&reply,
> > > + "Unknown miniflow extract implementation %s.",
> > > mfex_name);
> >
> > “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 +1203,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 +1210,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 parser not changed, PMD
> > > + thread
> > > %d"
> > > <
> >
> > We miss the “extract” part here (and do we need capital M as it’s not
> > the case elsewhere), i.e., "Error: miniflow extract parser not
> > changed, PMD thread”
> >
> > > + " 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 +1487,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
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
