Hi Flavio,
> -----Original Message-----
> From: Flavio Leitner <f...@sysclose.org>
> Sent: Sunday, July 11, 2021 6:14 AM
> To: Amber, Kumar <kumar.am...@intel.com>
> Cc: ovs-dev@openvswitch.org; i.maxim...@ovn.org
> Subject: Re: [ovs-dev] [v8 06/12] dpif-netdev: Add packet count and core id
> paramters for study
>
>
> Hi,
>
> On Fri, Jul 09, 2021 at 05:35:56PM +0530, kumar Amber wrote:
> > From: Kumar Amber <kumar.am...@intel.com>
> >
> > This commit introduces additional command line paramter
>
> Parameter.
>
Fixed.
> > 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.am...@intel.com>
> >
> > ---
> > 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 | 39 +++++++-
> > lib/dpif-netdev-extract-study.c | 26 ++++-
> > lib/dpif-netdev-private-extract.c | 2 +-
> > lib/dpif-netdev-private-extract.h | 9 ++
> > lib/dpif-netdev.c | 138 +++++++++++++++++++++++++--
> > 5 files changed, 200 insertions(+), 14 deletions(-)
> >
> > diff --git a/Documentation/topics/dpdk/bridge.rst
> > b/Documentation/topics/dpdk/bridge.rst
> > index 4db416ddd..8ed810f34 100644
> > --- a/Documentation/topics/dpdk/bridge.rst
> > +++ b/Documentation/topics/dpdk/bridge.rst
> > @@ -284,12 +284,47 @@ 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
> > +which can be set optionally. The first parameter core_id to set a
> > +particular miniflow
>
> The above command has two optional parameters: study_cnt and core_id. The
> core_id set a particular miniflow...
>
> > +extract function to a specific pmd thread on the core. Third
> > +parameter study_cnt is specific to study where how many packets
> > +needed to choose best implementation can be provided.. In case of any
> > +other implementation other than study third parameter [study_cnt] can
> > +pe provided with any arbitatry number and is ignored.
>
> 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
>
> The user can select...
>
> > a specific number of packets by applying all available implementaions
> > of miniflow extract and than chooses the one with most optimal result
> > for that -traffic pattern.
> > +traffic pattern. A user can also provide an additional packet count
> > +parameter
>
> The user can optionally provide an packet count [study_cnt] parameter...
>
Fixed all the comments .
> > +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 usser.
> > +
> > +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 last 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 can be put as any arbitary number or left blank::
> > +
> > + $ 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 f14464b2b..d29523db0 100644
> > --- a/lib/dpif-netdev-extract-study.c
> > +++ b/lib/dpif-netdev-extract-study.c
> > @@ -28,7 +28,7 @@
> VLOG_DEFINE_THIS_MODULE(dpif_mfex_extract_study);
> > /* Max count of packets to be compared. */ #define MFEX_MAX_COUNT
> > (128)
> >
> > -static uint32_t mfex_study_pkts_count = 0;
> > +static uint32_t mfex_study_pkts_count = MFEX_MAX_COUNT;
> >
> > /* Struct to hold miniflow study stats. */ struct study_stats { @@
> > -51,6 +51,28 @@ mfex_study_get_study_stats_ptr(void)
> > return stats;
> > }
> >
> > +uint32_t mfex_set_study_pkt_cnt(uint32_t pkt_cmp_count,
> > + const char *name) {
> > + struct dpif_miniflow_extract_impl *miniflow_funcs;
> > + dpif_mfex_impl_info_get(&miniflow_funcs);
> > +
> > + /* If the packet count is set and implementation called is study then
> > + * set packet counter to requested number else set the packet counter
> > + * to default number.
> > + */
> > + if ((strcmp(miniflow_funcs[MFEX_IMPL_STUDY].name, name) == 0) &&
> > + (pkt_cmp_count != 0)) {
> > +
> > + atomic_uintptr_t *study_pck_cnt = (void *)&mfex_study_pkts_count;
> > + atomic_store_relaxed(study_pck_cnt, (uintptr_t) pkt_cmp_count
> > + );
> > +
> > + return 0;
> > + }
> > +
> > + return -EINVAL;
> > +}
> > +
> > uint32_t
> > mfex_study_traffic(struct dp_packet_batch *packets,
> > struct netdev_flow_key *keys, @@ -93,7 +115,7 @@
> > 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_COUNT) {
> > + if (stats->pkt_count >= mfex_study_pkts_count) {
> > uint32_t best_func_index = MFEX_IMPL_START_IDX;
> > uint32_t max_hits = 0;
> > for (int i = MFEX_IMPL_START_IDX; i < impl_count; i++) { diff
> > --git a/lib/dpif-netdev-private-extract.c
> > b/lib/dpif-netdev-private-extract.c
> > index be8c69408..f3d593d39 100644
> > --- a/lib/dpif-netdev-private-extract.c
> > +++ b/lib/dpif-netdev-private-extract.c
> > @@ -122,7 +122,7 @@ dp_mfex_impl_get(struct ds *reply, struct
> > dp_netdev_pmd_thread **pmd_list,
> >
> > for (int i = 0; i < ARRAY_SIZE(mfex_impls); i++) {
> >
> > - ds_put_format(reply, " %s (available: %s)(pmds: ",
> > + ds_put_format(reply, " %s (available: %s, pmds: ",
>
> This should be in the patch introducing that line to avoid the issue in the
> first
> place.
>
>
> > mfex_impls[i].name, mfex_impls[i].available ?
> > "True" : "False");
> >
Fixed at right place.
> > diff --git a/lib/dpif-netdev-private-extract.h
> > b/lib/dpif-netdev-private-extract.h
> > index 802496cb9..bea532115 100644
> > --- a/lib/dpif-netdev-private-extract.h
> > +++ b/lib/dpif-netdev-private-extract.h
> > @@ -146,4 +146,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.
> > + */
> > +uint32_t 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
> > b416dc80c..87156fc69 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -1079,17 +1079,49 @@ static void
> > 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.
> > + /* A First optional paramter PMD thread ID can be also provided which
> > + * allows users to set miniflow implementation on a particular pmd.
> > + * The second paramter is the name of the function and if -pmd
> > + corr_id
>
> parameter, core_id, name of the implementation.
>
> > + * is not provided this is the first paramter and only mandatory one.
>
> parameter
>
Fixed.
> > + * The third and last one is the study-cnt which is only provided for
> > + * study function to set the packet count.
> > */
> > - const char *mfex_name = argv[1];
> > + const char *mfex_name = NULL;
> > struct shash_node *node;
> > + bool pmd_core_param_set = false;
> > + struct ds reply = DS_EMPTY_INITIALIZER;
> > +
> > + if (strcmp("-pmd", argv[1]) == 0) {
> > + mfex_name = argv[3];
> > + pmd_core_param_set = true;
> > + } else {
> > + mfex_name = argv[1];
> > + }
> > +
> > + uint32_t pmd_thread_specified = 0;
>
> That is boolean
>
> > + uint32_t pmd_thread_to_change = 0;
> > + uint32_t pmd_thread_update_ok = 0;
>
> Also boolean
>
Fixed both.
> > +
> > + if (pmd_core_param_set) {
> > + if (str_to_uint(argv[2], 10, &pmd_thread_to_change)) {
> > + pmd_thread_specified = 1;
> > + } else {
> > + /* argv[2] isn't even a uint. return without changing
> > anything. */
> > + 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]);
> > + const char *reply_str = ds_cstr(&reply);
> > + VLOG_INFO("%s", reply_str);
> > + unixctl_command_reply_error(conn, reply_str);
> > + ds_destroy(&reply);
> > + return;
> > + }
> > + }
> >
> > - ovs_mutex_lock(&dp_netdev_mutex);
> > int err = dp_mfex_impl_set_default_by_name(mfex_name);
> >
> > if (err) {
> > - ovs_mutex_unlock(&dp_netdev_mutex);
> > - struct ds reply = DS_EMPTY_INITIALIZER;
> > char *error_desc = NULL;
> > if (err == -EINVAL) {
> > error_desc = "Unknown miniflow extract implementation:";
> > @@ -1106,6 +1138,66 @@ dpif_miniflow_extract_impl_set(struct
> unixctl_conn *conn, int argc,
> > return;
> > }
> >
> > + /* argv[4] is optional packet count, which user can provide along with
> > + * study function to set the minimum packet that must be matched in
> > order
> > + * to choose the optimal function. */
> > + uint32_t pkt_cmp_count = 0;
> > + uint32_t study_ret = 0;
> > +
> > + if (argc == 5) {
> > + if (str_to_uint(argv[4], 10, &pkt_cmp_count)) {
> > + study_ret = mfex_set_study_pkt_cnt(pkt_cmp_count,
> > + mfex_name);
> > +
> > + if (study_ret == -EINVAL) {
> > + ds_put_format(&reply, "The study_pkt_cnt option is not
> > valid"
>
> The parameter is called study_cnt.
>
Fixed.
> > + " for the %s implementation.\n",
> > + 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;
> > + }
> > + } else {
> > + ds_put_format(&reply, "Invalid study_pkt_cnt value: : %s.\n",
> > + argv[4]);
> > + const char *reply_str = ds_cstr(&reply);
> > + unixctl_command_reply_error(conn, reply_str);
> > + VLOG_INFO("%s", reply_str);
> > + ds_destroy(&reply);
> > + return;
> > + }
> > + } else if ((!pmd_core_param_set) && (argc == 3)) {
> > + if (str_to_uint(argv[2], 10, &pkt_cmp_count)) {
> > + study_ret = mfex_set_study_pkt_cnt(pkt_cmp_count,
> > + mfex_name);
> > +
> > + if (study_ret == -EINVAL) {
> > + ds_put_format(&reply, "The study_pkt_cnt option is not
> > valid"
> > + " for the %s implementation.\n",
> > + 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;
> > + }
> > + } else {
> > + ds_put_format(&reply, "Invalid study_pkt_cnt value: %s.\n",
> > + argv[2]);
> > + const char *reply_str = ds_cstr(&reply);
> > + unixctl_command_reply_error(conn, reply_str);
> > + VLOG_INFO("%s", reply_str);
> > + ds_destroy(&reply);
> > + return;
> > + }
> > + } else {
> > + /* Default packet compare count when packets count not provided. */
> > + study_ret = mfex_set_study_pkt_cnt(0, mfex_name);
>
> Setting to zero returns an error. Should that be MFEX_MAX_COUNT?
>
> > +
> > + }
>
> Perhaps we could avoid repeating using the approach below:
>
Fixed.
> char *study_cnt_param = NULL;
> if (argc == 5) {
> study_cnt_param = argv[4];
> } else if ((!pmd_core_param_set) && (argc == 3)) {
> study_cnt_param = argv[2];
> }
>
> if (study_cnt_param) {
> uint32_t pkt_cmp_count;
>
> if (str_to_uint(study_cnt_param, 10, &pkt_cmp_count)) {
> study_ret = mfex_set_study_pkt_cnt(pkt_cmp_count, mfex_name);
> [...]
> } else {
> [...]
> }
> } else {
> /* Default packet compare count when packets count not * provided. */
> study_ret = mfex_set_study_pkt_cnt(MFEX_MAX_PKT_COUNT,
> mfex_name);
> }
>
Actually yes thanks 😊
> > +
> > + ovs_mutex_lock(&dp_netdev_mutex);
> > +
> > SHASH_FOR_EACH (node, &dp_netdevs) {
> > struct dp_netdev *dp = node->data;
> >
> > @@ -1122,8 +1214,14 @@ dpif_miniflow_extract_impl_set(struct
> unixctl_conn *conn, int argc,
> > continue;
> > }
> >
> > + if ((pmd_thread_specified) &&
> > + (pmd->core_id != pmd_thread_to_change)) {
> > + continue;
> > + }
> > +
> > /* Initialize MFEX function pointer to the newly configured
> > * default. */
> > + pmd_thread_update_ok = 1;
> > atomic_uintptr_t *pmd_func = (void *)
> > &pmd->miniflow_extract_opt;
> > atomic_store_relaxed(pmd_func, (uintptr_t) default_func);
> > };
> > @@ -1131,9 +1229,30 @@ dpif_miniflow_extract_impl_set(struct
> > unixctl_conn *conn, int argc,
> >
> > 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_ok) {
> > + 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);
> > + const char *reply_str = ds_cstr(&reply);
> > + VLOG_INFO("%s", reply_str);
> > + unixctl_command_reply_error(conn, reply_str);
> > + ds_destroy(&reply);
> > + return;
> > + }
> > +
> > /* Reply with success to command. */
> > - struct ds reply = DS_EMPTY_INITIALIZER;
> > - ds_put_format(&reply, "Miniflow implementation set to %s.\n",
> mfex_name);
> > + ds_put_format(&reply, "Miniflow implementation set to %s", mfex_name);
> > + if (pmd_thread_specified) {
> > + ds_put_format(&reply, ", on pmd thread %d", pmd_thread_to_change);
> > + }
> > + if (study_ret == 0) {
> > + ds_put_format(&reply, ", studying %d packets", pkt_cmp_count);
> > + }
> > +
> > + ds_put_format(&reply, ".\n");
> > +
> > const char *reply_str = ds_cstr(&reply);
> > VLOG_INFO("%s", reply_str);
> > unixctl_command_reply(conn, reply_str); @@ -1370,8 +1489,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
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
> --
> fbl
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
