On 12 Oct 2022, at 13:55, Cian Ferriter wrote:
> From: Kumar Amber <[email protected]>
>
> 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
>
> Signed-off-by: Kumar Amber <[email protected]>
> Signed-off-by: Cian Ferriter <[email protected]>
> Co-authored-by: Cian Ferriter <[email protected]>
> Acked-by: Sunil Pai G <[email protected]>
>
> ---
>
> v7:
> * Reword bridge.rst documentation to better explain the use of the
> -recirc option and provide examples. This is all based on feedback
> from Sunil.
> ---
> Documentation/topics/dpdk/bridge.rst | 32 ++++++++++++++++++++++------
> 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, 75 insertions(+), 12 deletions(-)
>
> diff --git a/Documentation/topics/dpdk/bridge.rst
> b/Documentation/topics/dpdk/bridge.rst
> index 354f1ced1..3bb01888a 100644
> --- a/Documentation/topics/dpdk/bridge.rst
> +++ b/Documentation/topics/dpdk/bridge.rst
> @@ -293,13 +293,29 @@ 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 optional ``-recirc`` parameter allows OVS to set the optimized MFEX
> +function for inner packet processing.
I think from a user perspective we should not talk about inner and outer
processing, and to be honest from a code perspective we should also not use
this, as it's confusing.
We should talk about original input packet processing and re-circulation
processing, as these are known concepts to OVS users. So can you please update
the documentation accordingly, including some more background info on why we
need different processing?
Also update the dpif-netdev-unixctl.man documentation.
> +
> +For example:
> +
> +- ``name``: sets the outer implementation to ``name``, inner defaults to
> + scalar.
> +
> +- ``name`` + ``recirc``: sets both outer and inner implementations to
Maybe call is ``-recird``
> + ``name``.
> +
> +- ``study`` + ``recirc``: sets outer and inner implementations independently
Maybe call is ``-recird``
> + based on the traffic pattern. The full command is::
> +
> + $ ovs-appctl dpif-netdev/miniflow-parser-set study -recirc
>
> 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 +338,10 @@ following command::
>
> $ ovs-appctl dpif-netdev/miniflow-parser-set -pmd 3 scalar
As the study command can set the original packet and re-circulation function
independently, the users should also be able to do this.
I would suggest changing the CLI options to something like this, which will
allow for all combinations:
ovs-appctl dpif-netdev/miniflow-parser-set [-pmd core_id] input_impl
[recirc_impl] [study_cnt]
> +``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;
Use default_mfex_input function, see comments on patch 5.
> +/* Variable to hold the default MFEX inner implementation. */
> +static ATOMIC(miniflow_extract_func) default_mfex_inner_func;
Please use recirc here to be the same as the dpif implementation,
default_mfex_recirc_func
In the dpif implementation, a new function was defined, so the functions both
do not need to pass the md_is_valid flag, and the in_port in the new function.
However I think we need clearly explain all the input variables at the
*miniflow_extract_func typedef, and what combination are and are not valid. For
example if md_is_valid is true the in_port value is not valid, etc. etc.
>
> #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)
As this is common code, please use recirc instead of inner.
> +{
> + 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)
Guess we should have two names, one for each implementation, based on feedback
above on CLI command.
> {
> 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);
This will result in duplicate PMDs, and it's also not clear which function is
for what PMD.
We need to report input and recirc PMDs separately.
> + }
> }
>
> 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);
We should probably rename this also to be more inline with what impl we
getting. dp_mfex_input_impl_get_default()
>
> +/* 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);
s/inner/recirc/g
> +
> /* 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..c7109c2fd 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;
Did not review the CLI handling as it might need to change based on the CLI
feedback.
> 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,
> @@ -7482,9 +7493,13 @@ dp_netdev_configure_pmd(struct dp_netdev_pmd_thread
> *pmd, struct dp_netdev *dp,
> atomic_init(&pmd->netdev_input_recirc_func,
> dp_netdev_recirc_impl_get_default());
>
> - /* Init default miniflow_extract function */
> + /* Init default miniflow_extract function. */
> atomic_init(&pmd->miniflow_extract_opt, dp_mfex_impl_get_default());
>
> + /* Init default miniflow_extract inner function. */
> + atomic_store_relaxed(&pmd->miniflow_extract_inner_opt,
Guess we should use atomic_init() like above/
> + dp_mfex_inner_impl_get_default());
> +
> /* init the 'flow_cache' since there is no
> * actual thread created for NON_PMD_CORE_ID. */
> if (core_id == NON_PMD_CORE_ID) {
> --
> 2.25.1
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
