Hi Sunil,
Thanks for the reviews will add the necessary documentation changes .
Regards
Amber
> -----Original Message-----
> From: Pai G, Sunil <sunil.pa...@intel.com>
> Sent: Tuesday, October 4, 2022 9:00 PM
> To: Amber, Kumar <kumar.am...@intel.com>; ovs-dev@openvswitch.org
> Cc: i.maxim...@ovn.org; f...@sysclose.org; Amber, Kumar
> <kumar.am...@intel.com>
> Subject: RE: [ovs-dev] [PATCH v5 6/9] dpif-mfex: Modify set/get mfex
> commands to include inner.
>
> 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
