Hi Amber,
Few comments inline.
> -----Original Message-----
> From: dev <ovs-dev-boun...@openvswitch.org> On Behalf Of Kumar Amber
> Sent: Friday, August 26, 2022 5:01 AM
> To: ovs-dev@openvswitch.org
> Cc: i.maxim...@ovn.org; f...@sysclose.org; Amber, Kumar
> <kumar.am...@intel.com>
> Subject: [ovs-dev] [PATCH v5 6/9] dpif-mfex: Modify set/get mfex commands
> to include inner.
Capitalize MFEX.
>
> The set command in MFEX is changed as to allow the user to select
> different optimized mfex ISA for processing Inner packets in case of
> tunneling.
Above could be reworded to:
The set command is modified to allow the user to select different
implementations for processing inner packets.
Also, the get command is modified to indicate both inner and outer MFEX
implementation in use.
>
> $ ovs-appctl dpif-netdev/miniflow-parser-set -pmd 3 study 1024 -recirc
>
> The get command is modified to indcitate both inner and Outer MFEXs in
> use.
>
> Signed-off-by: Kumar Amber <kumar.am...@intel.com>
> Signed-off-by: Cian Ferriter <cian.ferri...@intel.com>
> Co-authored-by: Cian Ferriter <cian.ferri...@intel.com>
> ---
> Documentation/topics/dpdk/bridge.rst | 18 ++++++++++++------
> lib/dpif-netdev-private-extract.c | 23 ++++++++++++++++++++++-
> lib/dpif-netdev-private-extract.h | 6 +++++-
> lib/dpif-netdev-private-thread.h | 3 +++
> lib/dpif-netdev.c | 23 ++++++++++++++++++++---
> 5 files changed, 62 insertions(+), 11 deletions(-)
>
> diff --git a/Documentation/topics/dpdk/bridge.rst
> b/Documentation/topics/dpdk/bridge.rst
> index 354f1ced1..d306417ec 100644
> --- a/Documentation/topics/dpdk/bridge.rst
> +++ b/Documentation/topics/dpdk/bridge.rst
> @@ -293,13 +293,15 @@ 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 [-pmd core_id] name \
> - [study_cnt]
> + [study_cnt] [-recirc]
>
> -The above command has two optional parameters: ``study_cnt`` and
> ``core_id``.
> -The ``core_id`` sets a particular packet parsing 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.
> +The above command has three optional parameters: ``study_cnt``,
> +``core_id`` and ``-recirc``. The ``core_id`` sets a particular packet
> +parsing 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. The fourth parameter ``-recirc`` acts like flag
> +which indicates to MFEX to use optimized MFEX inner for processing
> tunneled inner packets.
I would remove "acts like a flag which" from the last line.
I think its also required to document that if the user explicitly chooses a
specific implementation like avx512_ipv4_udp, both inner and outer mfex will be
set to that.
If study is given on the other hand, it gives OVS the flexibility to choose the
different optimized paths for outer and inner mfex.
>
> Also user can select the ``study`` implementation which studies the
> traffic for a specific number of packets by applying all available
> implementations of @@ -322,6 +324,10 @@ following command::
>
> $ ovs-appctl dpif-netdev/miniflow-parser-set -pmd 3 scalar
>
> +``study`` can be selected with packet count and explicit PMD selection
> +along with the ``recirc`` by following command::
> +
> + $ ovs-appctl dpif-netdev/miniflow-parser-set -pmd 3 study 1024
> + -recirc
>
> Actions Implementations (Experimental)
> --------------------------------------
> diff --git a/lib/dpif-netdev-private-extract.c b/lib/dpif-netdev-private-
> extract.c
> index 1a9b35420..fe0a53c2c 100644
> --- a/lib/dpif-netdev-private-extract.c
> +++ b/lib/dpif-netdev-private-extract.c
> @@ -33,6 +33,8 @@ VLOG_DEFINE_THIS_MODULE(dpif_netdev_extract);
>
> /* Variable to hold the default MFEX implementation. */ static
> ATOMIC(miniflow_extract_func) default_mfex_func;
> +/* Variable to hold the default MFEX inner implementation. */ static
> +ATOMIC(miniflow_extract_func) default_mfex_inner_func;
>
> #if MFEX_IMPL_AVX512_CHECK
> static int32_t
> @@ -231,16 +233,31 @@ dp_mfex_impl_get_default(void)
> return return_func;
> }
>
> +miniflow_extract_func
> +dp_mfex_inner_impl_get_default(void)
> +{
> + miniflow_extract_func return_func;
> + atomic_uintptr_t *mfex_func = (void *)&default_mfex_inner_func;
> +
> + atomic_read_relaxed(mfex_func, (uintptr_t *) &return_func);
> +
> + return return_func;
> +}
> +
> int
> -dp_mfex_impl_set_default_by_name(const char *name)
> +dp_mfex_impl_set_default_by_name(const char *name, bool mfex_inner)
> {
> miniflow_extract_func new_default;
> atomic_uintptr_t *mfex_func = (void *)&default_mfex_func;
> + atomic_uintptr_t *mfex_inner_func = (void
> + *)&default_mfex_inner_func;
>
> int err = dp_mfex_impl_get_by_name(name, &new_default);
>
> if (!err) {
> atomic_store_relaxed(mfex_func, (uintptr_t) new_default);
> + if (mfex_inner) {
> + atomic_store_relaxed(mfex_inner_func, (uintptr_t)
> new_default);
> + }
> }
>
> return err;
> @@ -268,6 +285,10 @@ dp_mfex_impl_get(struct ds *reply, struct
> dp_netdev_pmd_thread **pmd_list,
> if (pmd->miniflow_extract_opt == mfex_impls[i].extract_func)
> {
> ds_put_format(reply, "%u,", pmd->core_id);
> }
> + if (pmd->miniflow_extract_inner_opt ==
> + mfex_impls[i].extract_func) {
> + ds_put_format(reply, "%u,", pmd->core_id);
> + }
> }
>
> ds_chomp(reply, ',');
> diff --git a/lib/dpif-netdev-private-extract.h b/lib/dpif-netdev-private-
> extract.h
> index 8a7f9b01a..f5e6d33c1 100644
> --- a/lib/dpif-netdev-private-extract.h
> +++ b/lib/dpif-netdev-private-extract.h
> @@ -159,8 +159,12 @@ dp_mfex_impl_get_by_name(const char *name,
> miniflow_extract_func *out_func);
> * overridden at runtime. */
> miniflow_extract_func dp_mfex_impl_get_default(void);
>
> +/* Returns the default MFEX which is first ./configure selected, but
> +can be
> + * overridden at runtime. */
> +miniflow_extract_func dp_mfex_inner_impl_get_default(void);
> +
> /* Overrides the default MFEX with the user set MFEX. */ -int
> dp_mfex_impl_set_default_by_name(const char *name);
> +int dp_mfex_impl_set_default_by_name(const char *name, bool
> +mfex_inner);
>
> /* Retrieve the array of miniflow implementations for iteration. */
> struct dpif_miniflow_extract_impl * diff --git a/lib/dpif-netdev-private-
> thread.h b/lib/dpif-netdev-private-thread.h
> index bce91358b..3c998aa9a 100644
> --- a/lib/dpif-netdev-private-thread.h
> +++ b/lib/dpif-netdev-private-thread.h
> @@ -133,6 +133,9 @@ struct dp_netdev_pmd_thread {
> /* Function pointer to call for miniflow_extract() functionality. */
> ATOMIC(miniflow_extract_func) miniflow_extract_opt;
>
> + /* Function pointer to call for miniflow_extract() inner
> functionality. */
> + ATOMIC(miniflow_extract_func) miniflow_extract_inner_opt;
> +
> struct seq *reload_seq;
> uint64_t last_reload_seq;
>
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index
> 99102c5a0..93cbc029a 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -1205,6 +1205,7 @@ dpif_miniflow_extract_impl_set(struct unixctl_conn
> *conn, int argc,
> struct ds reply = DS_EMPTY_INITIALIZER;
> bool pmd_thread_update_done = false;
> bool mfex_name_is_study = false;
> + bool mfex_inner_is_set = false;
> const char *mfex_name = NULL;
> const char *reply_str = NULL;
> struct shash_node *node;
> @@ -1243,6 +1244,10 @@ dpif_miniflow_extract_impl_set(struct unixctl_conn
> *conn, int argc,
> argc -= 1;
> argv += 1;
>
> + } else if (strcmp("-recirc", argv[1]) == 0) {
> + mfex_inner_is_set = true;
> + 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) || @@ -1284,7
> +1289,7 @@ dpif_miniflow_extract_impl_set(struct unixctl_conn *conn, int
> argc,
> * 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);
> + err = dp_mfex_impl_set_default_by_name(mfex_name,
> + mfex_inner_is_set);
> if (err == -ENODEV) {
> ds_put_format(&reply,
> "Error: miniflow extract not available due to
> CPU"
> @@ -1301,6 +1306,7 @@ dpif_miniflow_extract_impl_set(struct unixctl_conn
> *conn, int argc,
>
> /* Get the desired MFEX function pointer and error check its usage.
> */
> miniflow_extract_func mfex_func = NULL;
> + miniflow_extract_func mfex_inner_func =
> + dp_mfex_inner_impl_get_default();
> err = dp_mfex_impl_get_by_name(mfex_name, &mfex_func);
> if (err) {
> if (err == -ENODEV) {
> @@ -1315,6 +1321,9 @@ dpif_miniflow_extract_impl_set(struct unixctl_conn
> *conn, int argc,
> goto error;
> }
>
> + if (mfex_inner_is_set) {
> + mfex_inner_func = mfex_func;
> + }
> /* Apply the MFEX pointer to each pmd thread in each netdev,
> filtering
> * by the users "-pmd" argument if required.
> */
> @@ -1341,6 +1350,8 @@ dpif_miniflow_extract_impl_set(struct unixctl_conn
> *conn, int argc,
>
> pmd_thread_update_done = true;
> atomic_store_relaxed(&pmd->miniflow_extract_opt, mfex_func);
> + atomic_store_relaxed(&pmd->miniflow_extract_inner_opt,
> + mfex_inner_func);
> };
>
> free(pmd_list);
> @@ -1615,8 +1626,8 @@ dpif_netdev_init(void)
> NULL);
> unixctl_command_register("dpif-netdev/miniflow-parser-set",
> "[-pmd core] miniflow_implementation_name"
> - " [study_pkt_cnt]",
> - 1, 5, dpif_miniflow_extract_impl_set,
> + " [study_pkt_cnt] [-recirc]",
> + 1, 6, dpif_miniflow_extract_impl_set,
> NULL);
> unixctl_command_register("dpif-netdev/miniflow-parser-get", "",
> 0, 0, dpif_miniflow_extract_impl_get, @@ -
> 7485,6 +7496,12 @@ dp_netdev_configure_pmd(struct dp_netdev_pmd_thread
> *pmd, struct dp_netdev *dp,
> /* Init default miniflow_extract function */
> atomic_init(&pmd->miniflow_extract_opt, dp_mfex_impl_get_default());
>
> + /* Init default miniflow_extract function */
Nit: End comment with a fullstop.
<snipped>
Thanks and regards
Sunil
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
