Re: [dpdk-dev] [PATCH 2/3] app/testpmd: fix RSS hash type update
+Shahaf,
Hi Maxime,
On Mon, Sep 13, 2021 at 11:41:04AM +0200, Maxime Coquelin wrote:
> Hi Nélio,
>
> On 9/10/21 4:16 PM, Nélio Laranjeiro wrote:
> > On Fri, Sep 10, 2021 at 01:06:53PM +0300, Andrew Rybchenko wrote:
> >> On 9/10/21 12:57 PM, Maxime Coquelin wrote:
> >>>
> >>>
> >>> On 9/10/21 11:51 AM, Andrew Rybchenko wrote:
> >>>> On 9/10/21 12:17 PM, Maxime Coquelin wrote:
> >>>>> port_rss_hash_key_update() initializes rss_conf with the
> >>>>> RSS hash type and key provided by the user, but it calls
> >>>>> rte_eth_dev_rss_hash_conf_get() before calling
> >>>>> rte_eth_dev_rss_hash_update(), which overides the parsed
> >>>>> config with current NIC's config.
> >>>>>
> >>>>> While the RSS key value is set again after, this is not
> >>>>> the case of the key length and the type of hash.
> >>>>>
> >>>>> There is no need to read the RSS config from the NIC, let's
> >>>>> just try to set the user defined one.
> >>>>>
> >>>>> Fixes: 8205e241b2b0 ("app/testpmd: add missing type to RSS hash
> >>>>> commands")
> >>>>> Cc: sta...@dpdk.org
> >>>>> Cc: nelio.laranje...@6wind.com
> >>>>>
> >>>>> Signed-off-by: Maxime Coquelin
> >>>>> ---
> >>>>> app/test-pmd/config.c | 8 ++--
> >>>>> 1 file changed, 2 insertions(+), 6 deletions(-)
> >>>>>
> >>>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> >>>>> index 31d8ba1b91..451bda53b1 100644
> >>>>> --- a/app/test-pmd/config.c
> >>>>> +++ b/app/test-pmd/config.c
> >>>>> @@ -2853,18 +2853,14 @@ port_rss_hash_key_update(portid_t port_id, char
> >>>>> rss_type[], uint8_t *hash_key,
> >>>>> int diag;
> >>>>> unsigned int i;
> >>>>>
> >>>>> - rss_conf.rss_key = NULL;
> >>>>> + rss_conf.rss_key = hash_key;
> >>>>> rss_conf.rss_key_len = hash_key_len;
> >>>>> rss_conf.rss_hf = 0;
> >>>>> for (i = 0; rss_type_table[i].str; i++) {
> >>>>> if (!strcmp(rss_type_table[i].str, rss_type))
> >>>>> rss_conf.rss_hf = rss_type_table[i].rss_type;
> >>>>> }
> >>>>> - diag = rte_eth_dev_rss_hash_conf_get(port_id, &rss_conf);
> >>>>> - if (diag == 0) {
> >>>>> - rss_conf.rss_key = hash_key;
> >>>>> - diag = rte_eth_dev_rss_hash_update(port_id, &rss_conf);
> >>>>> - }
> >>>>> + diag = rte_eth_dev_rss_hash_update(port_id, &rss_conf);
> >>>>
> >>>> I'm not 100% sure, but I'd say the intent above could be
> >>>> to update key only as the function name says. I.e. keep
> >>>> rss_hf as is. That could be the reason to get first.
> >
> > True,
> >
> >>> I think that was the intial purpose of the command, but patch
> >>> 8205e241b2b0 added setting the hash type as mandatory. There are
> >>> no other command to configure the hash type from testpmd AFAICT.
> >
> > Also for the same initial purpose, some NIC have an hash key per
> > protocol, by default it uses the same key for all of them but it can be
> > configured individually making for example key0 for all protocols expect
> > IPv4 which uses key1.
>
> Thanks for the info, I have looked at most drivers but didn't found one
> that support this feature, could you give some pointer?
Well, I have done the modification at that time for MLX5 PMD, since I
left DPDK in 2018 I don't know if they still support such configuration
from this API or if they fully moved to rte_flow.
> Given how the drivers implément the callback, do you agree with the fix,
> or do you have something else in mind?
I cannot answer if this get() is mandatory, this predates my arrival on
DPDK (original commit written in 2014), looking at DPDK state on
commit f79959ea1504 ("app/testpmd: allow to configure RSS hash key").
Maybe someone from Intel can help, eventually you can contact PMD
maintainers to review this patch.
Regards,
Nélio
> Thanks,
> Maxime
>
> >>> Also, even without 8205e241b2b0, the function was broken because the
> >>> key length was overiden.
> >>
> >> I see, many thanks for explanations.
> >
>
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH 2/3] app/testpmd: fix RSS hash type update
On Fri, Sep 10, 2021 at 01:06:53PM +0300, Andrew Rybchenko wrote:
> On 9/10/21 12:57 PM, Maxime Coquelin wrote:
> >
> >
> > On 9/10/21 11:51 AM, Andrew Rybchenko wrote:
> >> On 9/10/21 12:17 PM, Maxime Coquelin wrote:
> >>> port_rss_hash_key_update() initializes rss_conf with the
> >>> RSS hash type and key provided by the user, but it calls
> >>> rte_eth_dev_rss_hash_conf_get() before calling
> >>> rte_eth_dev_rss_hash_update(), which overides the parsed
> >>> config with current NIC's config.
> >>>
> >>> While the RSS key value is set again after, this is not
> >>> the case of the key length and the type of hash.
> >>>
> >>> There is no need to read the RSS config from the NIC, let's
> >>> just try to set the user defined one.
> >>>
> >>> Fixes: 8205e241b2b0 ("app/testpmd: add missing type to RSS hash commands")
> >>> Cc: sta...@dpdk.org
> >>> Cc: nelio.laranje...@6wind.com
> >>>
> >>> Signed-off-by: Maxime Coquelin
> >>> ---
> >>> app/test-pmd/config.c | 8 ++--
> >>> 1 file changed, 2 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> >>> index 31d8ba1b91..451bda53b1 100644
> >>> --- a/app/test-pmd/config.c
> >>> +++ b/app/test-pmd/config.c
> >>> @@ -2853,18 +2853,14 @@ port_rss_hash_key_update(portid_t port_id, char
> >>> rss_type[], uint8_t *hash_key,
> >>> int diag;
> >>> unsigned int i;
> >>>
> >>> - rss_conf.rss_key = NULL;
> >>> + rss_conf.rss_key = hash_key;
> >>> rss_conf.rss_key_len = hash_key_len;
> >>> rss_conf.rss_hf = 0;
> >>> for (i = 0; rss_type_table[i].str; i++) {
> >>> if (!strcmp(rss_type_table[i].str, rss_type))
> >>> rss_conf.rss_hf = rss_type_table[i].rss_type;
> >>> }
> >>> - diag = rte_eth_dev_rss_hash_conf_get(port_id, &rss_conf);
> >>> - if (diag == 0) {
> >>> - rss_conf.rss_key = hash_key;
> >>> - diag = rte_eth_dev_rss_hash_update(port_id, &rss_conf);
> >>> - }
> >>> + diag = rte_eth_dev_rss_hash_update(port_id, &rss_conf);
> >>
> >> I'm not 100% sure, but I'd say the intent above could be
> >> to update key only as the function name says. I.e. keep
> >> rss_hf as is. That could be the reason to get first.
True,
> > I think that was the intial purpose of the command, but patch
> > 8205e241b2b0 added setting the hash type as mandatory. There are
> > no other command to configure the hash type from testpmd AFAICT.
Also for the same initial purpose, some NIC have an hash key per
protocol, by default it uses the same key for all of them but it can be
configured individually making for example key0 for all protocols expect
IPv4 which uses key1.
> > Also, even without 8205e241b2b0, the function was broken because the
> > key length was overiden.
>
> I see, many thanks for explanations.
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH v3 0/7] ethdev: add flow API object converter
On Fri, Aug 31, 2018 at 11:00:57AM +0200, Adrien Mazarguil wrote: > This is a follow up to the "Flow API helpers enhancements" series submitted > almost a year ago [1]. The new title is due to the reduced scope of this > version. > > rte_flow_conv() is a flexible replacement to rte_flow_copy(), itself a > temporary solution pending something better [2]. It replaces a lot of > duplicated code found in testpmd and removes some of the maintenance burden > that developers tend to forget (me included) when modifying pattern > items or actions (updating app/test-pmd/config.c to be clear). > > This series was unearthed in order to complete the implementation of > RTE_FLOW_ACTION_TYPE_ENCAP_(VXLAN|NVGRE) in testpmd [3] without having to > duplicate existing code once again. > > See individual patches for specific changes in this version. > > v3 changes: > > - Marked rte_flow_conv() as experimental, modified net/bonding accordingly. > - Fixed compilation issue on ARM. > - Removed deprecation notice. > > v2 changes: > > - rte_flow_copy() is kept, albeit deprecated, no API/ABI impact. > - Updated bonding PMD. > - No more automatic generation of rte_flow_conv.h. > > [1] https://mails.dpdk.org/archives/dev/2017-October/077551.html > [2] https://mails.dpdk.org/archives/dev/2017-July/070492.html > [3] Currently the command-line parser (cmdline_flow.c) is aware of these > actions, however config.c isn't. Flow rules with such actions cannot > be created and cannot be validated with PMDs that implement them. > > Adrien Mazarguil (7): > ethdev: add flow API object converter > ethdev: add flow API item/action name conversion > app/testpmd: rely on flow API conversion function > net/failsafe: switch to flow API object conversion function > net/bonding: switch to flow API object conversion function > ethdev: add missing items/actions to flow object converter > ethdev: deprecate rte_flow_copy function > > app/test-pmd/config.c | 407 +++ > app/test-pmd/testpmd.h | 7 +- > doc/guides/prog_guide/rte_flow.rst | 20 + > doc/guides/rel_notes/deprecation.rst | 7 - > drivers/net/bonding/Makefile | 1 + > drivers/net/bonding/meson.build| 1 + > drivers/net/bonding/rte_eth_bond_api.c | 6 +- > drivers/net/bonding/rte_eth_bond_flow.c| 31 +- > drivers/net/bonding/rte_eth_bond_private.h | 5 +- > drivers/net/failsafe/failsafe_ether.c | 6 +- > drivers/net/failsafe/failsafe_flow.c | 31 +- > drivers/net/failsafe/failsafe_private.h| 5 +- > lib/librte_ethdev/rte_ethdev_version.map | 1 + > lib/librte_ethdev/rte_flow.c | 666 ++++++-- > lib/librte_ethdev/rte_flow.h | 231 +++- > 15 files changed, 886 insertions(+), 539 deletions(-) > > -- > 2.11.0 Acked-by: Nelio Laranjeiro -- Nélio Laranjeiro 6WIND
Re: [dpdk-dev] [PATCH v3] net/mlx: add meson build support
On Thu, Aug 30, 2018 at 03:46:17PM +0100, Bruce Richardson wrote:
> On Wed, Aug 29, 2018 at 03:48:17PM +0200, Nelio Laranjeiro wrote:
> > Compile Mellanox drivers when their external dependencies are met. A
> > glue version of the driver can still be requested by using the
> > -Denable_driver_mlx_glue=true
> >
> > To avoid modifying the whole sources and keep the compatibility with
> > current build systems (e.g. make), the mlx{4,5}_autoconf.h is still
> > generated by invoking DPDK scripts though meson's run_command() instead
> > of using has_types, has_members, ... commands.
> >
> > Meson will try to find the required external libraries. When they are
> > not installed system wide, they can be provided though CFLAGS, LDFLAGS
> > and LD_LIBRARY_PATH environment variables, example (considering
> > RDMA-Core is installed in /tmp/rdma-core):
> >
> > # CLFAGS=-I/tmp/rdma-core/build/include \
> >LDFLAGS=-L/tmp/rdma-core/build/lib \
> >LD_LIBRARY_PATH=/tmp/rdma-core/build/lib \
> >meson output
> > # LD_LIBRARY_PATH=/tmp/rdma-core/build/lib \
> >ninja -C output install
> >
> > Note: LD_LIBRARY_PATH before ninja is necessary when the meson
> > configuration has changed (e.g. meson configure has been called), in
> > such situation the LD_LIBRARY_PATH is necessary to invoke the
> > autoconfiguration script.
> >
> > Signed-off-by: Nelio Laranjeiro
> >
>
> Couple of minor comments inline below. Otherwise:
>
> Acked-by: Bruce Richardson
>[...]
Hi Bruce,
I'll address those comments in a v4, thanks a lot for your help and
review.
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH v2] net/mlx: add meson build support
On Wed, Aug 29, 2018 at 11:01:15AM +0100, Bruce Richardson wrote:
> On Wed, Aug 29, 2018 at 11:34:10AM +0200, Nélio Laranjeiro wrote:
> > Hi Bruce,
> >
> > Thanks for your comments I have address almost all of them in the v3 by
> > doing what you suggest, I still have some comments, please see below,
> >
>
> Thanks.
>
> > On Tue, Aug 28, 2018 at 04:45:00PM +0100, Bruce Richardson wrote:
> > > Thanks for this, comments inline below.
> > >
> > > /Bruce
> > >
> > > On Mon, Aug 27, 2018 at 02:42:25PM +0200, Nelio Laranjeiro wrote:
> > > > Mellanox drivers remains un-compiled by default due to third party
> > > > libraries dependencies. They can be enabled through:
> > > > - enable_driver_mlx{4,5}=true or
> > > > - enable_driver_mlx{4,5}_glue=true
> > > > depending on the needs.
> > >
> > > The big reason why we wanted a new build system was to move away from this
> > > sort of static configuration. Instead, detect if the requirements as
> > > present and build the driver if you can.
> >
> > Ok, I am letting only the glue option for both drivers as suggested at
> > the end of your answer.
> >
> > > > To avoid modifying the whole sources and keep the compatibility with
> > > > current build systems (e.g. make), the mlx{4,5}_autoconf.h is still
> > > > generated by invoking DPDK scripts though meson's run_command() instead
> > > > of using has_types, has_members, ... commands.
> > > >
> > > > Meson will try to find the required external libraries. When they are
> > > > not installed system wide, they can be provided though CFLAGS, LDFLAGS
> > > > and LD_LIBRARY_PATH environment variables, example (considering
> > > > RDMA-Core is installed in /tmp/rdma-core):
> > > >
> > > > # CLFAGS=-I/tmp/rdma-core/build/include \
> > > >LDFLAGS=-L/tmp/rdma-core/build/lib \
> > > >LD_LIBRARY_PATH=/tmp/rdma-core/build/lib \
> > > >meson -Denable_driver_mlx4=true output
> > > >
> > > > # CLFAGS=-I/tmp/rdma-core/build/include \
> > > >LDFLAGS=-L/tmp/rdma-core/build/lib \
> > > >LD_LIBRARY_PATH=/tmp/rdma-core/build/lib \
> > > >ninja -C output install
> > >
> > > Once the CFLAGS/LDFLAGS are passed to meson, they should not be needed for
> > > ninja. The LD_LIBRARY_PATH might be - I'm not sure about that one! :-)
> >
> > CFLAGS/LDFLAGS are correctly evaluated and inserted in the build.ninja
> > file, for the LD_LIBRARY_PATH, it is necessary for the run_command stuff
> > generating the mlx*_autoconf.h
> >
>
> Just realised there is another issue which you should address. The
> mlx*_autoconf.h files are being written into a source folder rather than
> into the destination folder. This could cause problems if we are enabling
> mlx for cross-compile builds. Perhaps inside the auto-config-h.sh script
> you can check for $MESON_BUILD_ROOT value, and use that (and possibly
> $MESON_SUBDIR) to put the header file in the build directory.
Indeed, I was searching also for a solution, I finally found it without
modifying the shell script by using meson.current_build_dir() which
contains the same path has $MESON_BUILD_ROOT/$MESON_SUBDIR.
> > >[...]
> > > Rather than having your own separate debug option flag, why not set these
> > > based on the "buildtype" option e.g. if buildtype is set to "debug".
> > >
> > > > +# To maintain the compatibility with the make build system
> > > > +# mlx4_autoconf.h file is still generated.
> > > > +r = run_command('sh', '../../../buildtools/auto-config-h.sh',
> > > > +'mlx4_autoconf.h',
> > > > +'HAVE_IBV_MLX4_WQE_LSO_SEG',
> > > > +'infiniband/mlx4dv.h',
> > > > +'type', 'struct mlx4_wqe_lso_seg')
> > > > +if r.returncode() != 0
> > > > +error('autoconfiguration fail')
> > > > +endif
> > >
> > > Just to check that you are ok with this only being run at configure time?
> > > If any changes are made to the inputs, ninja won't pick them up. To have
> > > it
> > > tracked for input changes, "custom_target" should be used instead of
> > > run_command.
> >
Re: [dpdk-dev] [PATCH v2] net/mlx: add meson build support
On Wed, Aug 29, 2018 at 11:00:54AM +0100, Luca Boccassi wrote:
> On Tue, 2018-08-28 at 16:45 +0100, Bruce Richardson wrote:
> > Thanks for this, comments inline below.
> >
> > /Bruce
> >
> > On Mon, Aug 27, 2018 at 02:42:25PM +0200, Nelio Laranjeiro wrote:
> > > Mellanox drivers remains un-compiled by default due to third party
> > > libraries dependencies. They can be enabled through:
> > > - enable_driver_mlx{4,5}=true or
> > > - enable_driver_mlx{4,5}_glue=true
> > > depending on the needs.
> >
> > The big reason why we wanted a new build system was to move away from
> > this
> > sort of static configuration. Instead, detect if the requirements as
> > present and build the driver if you can.
> >
> > >
> > > To avoid modifying the whole sources and keep the compatibility
> > > with
> > > current build systems (e.g. make), the mlx{4,5}_autoconf.h is still
> > > generated by invoking DPDK scripts though meson's run_command()
> > > instead
> > > of using has_types, has_members, ... commands.
> > >
> > > Meson will try to find the required external libraries. When they
> > > are
> > > not installed system wide, they can be provided though CFLAGS,
> > > LDFLAGS
> > > and LD_LIBRARY_PATH environment variables, example (considering
> > > RDMA-Core is installed in /tmp/rdma-core):
> > >
> > > # CLFAGS=-I/tmp/rdma-core/build/include \
> > > LDFLAGS=-L/tmp/rdma-core/build/lib \
> > > LD_LIBRARY_PATH=/tmp/rdma-core/build/lib \
> > > meson -Denable_driver_mlx4=true output
> > >
> > > # CLFAGS=-I/tmp/rdma-core/build/include \
> > > LDFLAGS=-L/tmp/rdma-core/build/lib \
> > > LD_LIBRARY_PATH=/tmp/rdma-core/build/lib \
> > > ninja -C output install
> >
> > Once the CFLAGS/LDFLAGS are passed to meson, they should not be
> > needed for
> > ninja. The LD_LIBRARY_PATH might be - I'm not sure about that one! :-
> > )
> >
> > >
> > > Signed-off-by: Nelio Laranjeiro
> > >
> > > ---
> > >
> > > Changes in v2:
> > >
> > > - dropped patch https://patches.dpdk.org/patch/43897/
> > > - remove extra_{cflags,ldflags} as already honored by meson through
> > > environment variables.
> > > ---
> > > drivers/net/meson.build | 2 +
> > > drivers/net/mlx4/meson.build | 94 ++
> > > drivers/net/mlx5/meson.build | 545
> > > +++
> > > meson_options.txt| 8 +
> > > 4 files changed, 649 insertions(+)
> > > create mode 100644 drivers/net/mlx4/meson.build
> > > create mode 100644 drivers/net/mlx5/meson.build
> > >
> > > diff --git a/drivers/net/meson.build b/drivers/net/meson.build
> > > index 9c28ed4da..c7a2d0e7d 100644
> > > --- a/drivers/net/meson.build
> > > +++ b/drivers/net/meson.build
> > > @@ -18,6 +18,8 @@ drivers = ['af_packet',
> > > 'ixgbe',
> > > 'kni',
> > > 'liquidio',
> > > + 'mlx4',
> > > + 'mlx5',
> > > 'mvpp2',
> > > 'netvsc',
> > > 'nfp',
> > > diff --git a/drivers/net/mlx4/meson.build
> > > b/drivers/net/mlx4/meson.build
> > > new file mode 100644
> > > index 0..debaca5b6
> > > --- /dev/null
> > > +++ b/drivers/net/mlx4/meson.build
> > > @@ -0,0 +1,94 @@
> > > +# SPDX-License-Identifier: BSD-3-Clause
> > > +# Copyright 2018 6WIND S.A.
> > > +# Copyright 2018 Mellanox Technologies, Ltd
> > > +
> > > +# As there is no more configuration file to activate/configure the
> > > PMD it will
> > > +# use some variables here to configure it.
> > > +pmd_dlopen = get_option('enable_driver_mlx4_glue')
> > > +build = get_option('enable_driver_mlx4') or pmd_dlopen
> >
> > As stated above, I believe this should be based upon whether you find
> > the
> > "mnl", "mlx4" and "ibverbs" libraries. If we start adding back in
> > static
> > options for every driver, then we'll be back to having a mass of
> > config
> > options like we had before.
>
> BTW, slightly related to that: ibverbs doesn't ship pkg-config files at
> the moment which makes the detection slightly more awkward that it
> could be, so I've sent a PR upstream to add that:
>
> https://github.com/linux-rdma/rdma-core/pull/373
>
> Hope this can be useful!
Thanks Luca, I was also searching for it, you save me some time, I hope
this can be backported down to RDMA-Core's stable version v15 of
RDMA-Core it would fully help.
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH v2] net/mlx: add meson build support
Hi Bruce,
Thanks for your comments I have address almost all of them in the v3 by
doing what you suggest, I still have some comments, please see below,
On Tue, Aug 28, 2018 at 04:45:00PM +0100, Bruce Richardson wrote:
> Thanks for this, comments inline below.
>
> /Bruce
>
> On Mon, Aug 27, 2018 at 02:42:25PM +0200, Nelio Laranjeiro wrote:
> > Mellanox drivers remains un-compiled by default due to third party
> > libraries dependencies. They can be enabled through:
> > - enable_driver_mlx{4,5}=true or
> > - enable_driver_mlx{4,5}_glue=true
> > depending on the needs.
>
> The big reason why we wanted a new build system was to move away from this
> sort of static configuration. Instead, detect if the requirements as
> present and build the driver if you can.
Ok, I am letting only the glue option for both drivers as suggested at
the end of your answer.
> > To avoid modifying the whole sources and keep the compatibility with
> > current build systems (e.g. make), the mlx{4,5}_autoconf.h is still
> > generated by invoking DPDK scripts though meson's run_command() instead
> > of using has_types, has_members, ... commands.
> >
> > Meson will try to find the required external libraries. When they are
> > not installed system wide, they can be provided though CFLAGS, LDFLAGS
> > and LD_LIBRARY_PATH environment variables, example (considering
> > RDMA-Core is installed in /tmp/rdma-core):
> >
> > # CLFAGS=-I/tmp/rdma-core/build/include \
> >LDFLAGS=-L/tmp/rdma-core/build/lib \
> >LD_LIBRARY_PATH=/tmp/rdma-core/build/lib \
> >meson -Denable_driver_mlx4=true output
> >
> > # CLFAGS=-I/tmp/rdma-core/build/include \
> >LDFLAGS=-L/tmp/rdma-core/build/lib \
> >LD_LIBRARY_PATH=/tmp/rdma-core/build/lib \
> >ninja -C output install
>
> Once the CFLAGS/LDFLAGS are passed to meson, they should not be needed for
> ninja. The LD_LIBRARY_PATH might be - I'm not sure about that one! :-)
CFLAGS/LDFLAGS are correctly evaluated and inserted in the build.ninja
file, for the LD_LIBRARY_PATH, it is necessary for the run_command stuff
generating the mlx*_autoconf.h
>[...]
> Rather than having your own separate debug option flag, why not set these
> based on the "buildtype" option e.g. if buildtype is set to "debug".
>
> > +# To maintain the compatibility with the make build system
> > +# mlx4_autoconf.h file is still generated.
> > +r = run_command('sh', '../../../buildtools/auto-config-h.sh',
> > +'mlx4_autoconf.h',
> > +'HAVE_IBV_MLX4_WQE_LSO_SEG',
> > +'infiniband/mlx4dv.h',
> > +'type', 'struct mlx4_wqe_lso_seg')
> > +if r.returncode() != 0
> > +error('autoconfiguration fail')
> > +endif
>
> Just to check that you are ok with this only being run at configure time?
> If any changes are made to the inputs, ninja won't pick them up. To have it
> tracked for input changes, "custom_target" should be used instead of
> run_command.
It seems to not be possible to have several custom_target on the same
output file has this last is used as the target identifier in ninja.
This limitation is acceptable for now, when meson will be the default
build system, then such autoconf can be removed to use meson built-in
functions.
> > +endif
> > +# Build Glue Library
> > +if pmd_dlopen
> > +dlopen_name = 'mlx4_glue'
> > +dlopen_lib_name = driver_name_fmt.format(dlopen_name)
> > +dlopen_so_version = LIB_GLUE_VERSION
> > +dlopen_sources = files('mlx4_glue.c')
> > +dlopen_install_dir = [ eal_pmd_path + '-glue' ]
> > +shared_lib = shared_library(
> > + dlopen_lib_name,
> > + dlopen_sources,
> > + include_directories: global_inc,
> > + c_args: cflags,
> > + link_args: [
> > + '-Wl,-export-dynamic',
> > + '-Wl,-h,@0@'.format(LIB_GLUE),
> > + '-lmlx4',
> > + '-libverbs',
>
> While this works, the recommended approach is to save the return value from
> cc.find_library() above, and pass that as a dependency directly, rather
> than as a linker flag.
I tried it, but:
drivers/net/mlx5/meson.build:216:8: ERROR: Link_args arguments must be
strings.
find_library returns a compiler object, I did not found anyway to use
directly the output of the find_library which works in places.
Thanks,
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH 1/2] build: add extra cflags ldflags to meson option
On Mon, Aug 27, 2018 at 12:24:11PM +0100, Bruce Richardson wrote:
> On Mon, Aug 27, 2018 at 01:10:52PM +0200, Nelio Laranjeiro wrote:
> > Almost equivalent to the make system build which uses those options
> > through environment variables (EXTRA_{CFLAGS,LDFLAGS}).
> >
> > Signed-off-by: Nelio Laranjeiro
> > ---
> > drivers/meson.build | 2 +-
> > meson_options.txt | 2 ++
> > 2 files changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/meson.build b/drivers/meson.build
> > index f94e2fe67..008aac62c 100644
> > --- a/drivers/meson.build
> > +++ b/drivers/meson.build
> > @@ -11,7 +11,7 @@ driver_classes = ['common',
> >'event', # depends on common, bus, mempool and net.
> >'raw'] # depends on common, bus, mempool, net and event.
> >
> > -default_cflags = machine_args
> > +default_cflags = machine_args + [get_option('extra_cflags'),
> > get_option('extra_ldflags')]
> > if cc.has_argument('-Wno-format-truncation')
> > default_cflags += '-Wno-format-truncation'
> > endif
> > diff --git a/meson_options.txt b/meson_options.txt
> > index c84327858..da6373a2c 100644
> > --- a/meson_options.txt
> > +++ b/meson_options.txt
> > @@ -22,3 +22,5 @@ option('use_hpet', type: 'boolean', value: false,
> > description: 'use HPET timer in EAL')
> > option('tests', type: 'boolean', value: true,
> > description: 'build unit tests')
> > +option('extra_cflags', type: 'string', description: 'Extra compiler flags')
> > +option('extra_ldflags', type: 'string', description: 'Extra linker flags')
>
> This should not be needed. Meson should pick up CFLAGS and LDFLAGS from the
> environment without having to add options for them.
>
> https://mesonbuild.com/howtox.html#set-extra-compiler-and-linker-flags-from-the-outside-when-eg-building-distro-packages
>
> /Bruce
Indeed this works with the CLFAGS/LDFLAGS way, but to find correctly the
library dependencies, it also needs to have the LD_LIBRARY_PATH set with
the correct path.
This patch will be discarded in the next version.
Thanks,
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH] net/mlx5: fix possible endless loop when clearing flow flags
On Tue, Jul 24, 2018 at 09:47:19PM +, Yongseok Koh wrote:
>
> > On Jul 23, 2018, at 11:57 PM, Nélio Laranjeiro
> > wrote:
> >
> > On Mon, Jul 23, 2018 at 11:27:44AM -0700, Yongseok Koh wrote:
> >> If one of (*priv->rxqs)[] is null, the for loop can iterate infinitely as
> >> idx can't be increased.
> >>
> >> Fixes: cd24d526395e ("net/mlx5: add mark/flag flow action")
> >> Cc: Nelio Laranjeiro
> >>
> >> Signed-off-by: Yongseok Koh
> >> ---
> >> drivers/net/mlx5/mlx5_flow.c | 8 +++-
> >> 1 file changed, 3 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
> >> index 32854198b..c156f01eb 100644
> >> --- a/drivers/net/mlx5/mlx5_flow.c
> >> +++ b/drivers/net/mlx5/mlx5_flow.c
> >> @@ -2762,22 +2762,20 @@ mlx5_flow_rxq_flags_clear(struct rte_eth_dev *dev)
> >> {
> >>struct priv *priv = dev->data->dev_private;
> >>unsigned int i;
> >> - unsigned int idx;
> >>
> >> - for (idx = 0, i = 0; idx != priv->rxqs_n; ++i) {
> >> + for (i = 0; i != priv->rxqs_n; ++i) {
> >>struct mlx5_rxq_ctrl *rxq_ctrl;
> >>unsigned int j;
> >>
> >> - if (!(*priv->rxqs)[idx])
> >> + if (!(*priv->rxqs)[i])
> >>continue;
> >> - rxq_ctrl = container_of((*priv->rxqs)[idx],
> >> + rxq_ctrl = container_of((*priv->rxqs)[i],
> >>struct mlx5_rxq_ctrl, rxq);
> >>rxq_ctrl->flow_mark_n = 0;
> >>rxq_ctrl->rxq.mark = 0;
> >>for (j = 0; j != MLX5_FLOW_TUNNEL; ++j)
> >>rxq_ctrl->flow_tunnels_n[j] = 0;
> >>rxq_ctrl->rxq.tunnel = 0;
> >> - ++idx;
> >>}
> >> }
> >>
> >> --
> >> 2.11.0
> >
> > This patch is wrong, (*priv->rxqs)[i] may un-initialised by the
> > application, the number of queues says how are in used, it does not mean
> > they are contiguous in the rxqs arrays and this due to the DPDK API
> > which configure the number of queues with rte_eth_dev_configure()
> > whereas queues are instantiated with rte_eth_rx_queue_setup() which
> > takes an position in the array as parameter.
> >
> > Indeed this code is wrong, idx should always increase whereas i should
> > only increase if the (*priv->rxqs)[idx] is non null.
>
> I don't understand what you mean. In rte_eth_rx_queue_setup(), rx_queue_id is
> checked against dev->data->nb_rx_queues.
>
> if (rx_queue_id >= dev->data->nb_rx_queues) {
> RTE_ETHDEV_LOG(ERR, "Invalid RX queue_id=%u\n", rx_queue_id);
> return -EINVAL;
> }
>
> This means the index should be [0, priv->rxqs_n) anyway. There is the same
> check
> in mlx5_rx_queue_setup(). If user mistakenly doesn't configure some of queues,
> then the corresponding slots could be null but indexes are still within the
> range.
>
> Then, what's your point of having both i and idx?
I remember I've face some issue while I've re-write the PMD to work on
top of flow API. That's why I've introduce such logic, but it seems not
necessary as it comply with the documentation of the function and the
code itself.
Acked-by: Nelio Laranjeiro
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH] mk: fix application compilation with lmnl and mlx5
On Tue, Jul 24, 2018 at 11:21:52AM +, Shahaf Shuler wrote:
> Tuesday, July 24, 2018 12:29 PM, Nelio Laranjeiro:
> > Subject: [PATCH] mk: fix application compilation with lmnl and mlx5
> >
> > When Mellanox MLX5 PMD is compiled with
> > CONFIG_RTE_LIBRTE_MLX5_DLOPEN_DEPS=y, the external dependency on
> > libmln is missing.
> >
> > Fixes: 4d5cce06231a ("net/mlx5: lay groundwork for switch offloads")
> > Cc: adrien.mazarg...@6wind.com
> >
> > Signed-off-by: Nelio Laranjeiro
> > ---
> > mk/rte.app.mk | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/mk/rte.app.mk b/mk/rte.app.mk index f4d28c2da..ff39d37aa
> > 100644
> > --- a/mk/rte.app.mk
> > +++ b/mk/rte.app.mk
> > @@ -149,7 +149,7 @@ else
> > _LDLIBS-$(CONFIG_RTE_LIBRTE_MLX4_PMD) += -lrte_pmd_mlx4 -
> > libverbs -lmlx4
> > endif
> > ifeq ($(CONFIG_RTE_LIBRTE_MLX5_DLOPEN_DEPS),y)
> > -_LDLIBS-$(CONFIG_RTE_LIBRTE_MLX5_PMD) += -lrte_pmd_mlx5 -ldl
> > +_LDLIBS-$(CONFIG_RTE_LIBRTE_MLX5_PMD) += -lrte_pmd_mlx5 -ldl -
> > lmnl
>
> This issue raise some more basic question.
> The DLOPEN mode was introduced to run in systems which don't have
> verbs/mlx5 libs installed, because those were the only dependencies
> for the PMD back then.
> Now we have the libmnl, which is external dependency just like
> rdma-core, and following your fix, hard linked also in case of DLOPEN
> option.
> It means the whole DPDK binary/lib will be depended on libmnl and this
> is not what we want with DLOPEN.
>
> Can we consider different options:
> 1. always statically link libmnl
This will force users to re-compile their application to have the missing
features disabled because some calls are not available, see the list of
HAVE_RDMA_NLDEV_* elements in the MLX5 makefile due to the issues this
Netlink stuff brings.
> 2. dlopen libmnl just like we do for verbs/mlx5
You want another glue library. It won't be for this release in this
case, I don't have time to write such glue.
> > else
> > _LDLIBS-$(CONFIG_RTE_LIBRTE_MLX5_PMD) += -lrte_pmd_mlx5 -
> > libverbs -lmlx5 -lmnl
> > endif
> > --
> > 2.18.0
>
[1] https://mails.dpdk.org/archives/dev/2018-March/092876.html
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH] net/mlx5: fix possible endless loop when clearing flow flags
On Mon, Jul 23, 2018 at 11:27:44AM -0700, Yongseok Koh wrote:
> If one of (*priv->rxqs)[] is null, the for loop can iterate infinitely as
> idx can't be increased.
>
> Fixes: cd24d526395e ("net/mlx5: add mark/flag flow action")
> Cc: Nelio Laranjeiro
>
> Signed-off-by: Yongseok Koh
> ---
> drivers/net/mlx5/mlx5_flow.c | 8 +++-
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
> index 32854198b..c156f01eb 100644
> --- a/drivers/net/mlx5/mlx5_flow.c
> +++ b/drivers/net/mlx5/mlx5_flow.c
> @@ -2762,22 +2762,20 @@ mlx5_flow_rxq_flags_clear(struct rte_eth_dev *dev)
> {
> struct priv *priv = dev->data->dev_private;
> unsigned int i;
> - unsigned int idx;
>
> - for (idx = 0, i = 0; idx != priv->rxqs_n; ++i) {
> + for (i = 0; i != priv->rxqs_n; ++i) {
> struct mlx5_rxq_ctrl *rxq_ctrl;
> unsigned int j;
>
> - if (!(*priv->rxqs)[idx])
> + if (!(*priv->rxqs)[i])
> continue;
> - rxq_ctrl = container_of((*priv->rxqs)[idx],
> + rxq_ctrl = container_of((*priv->rxqs)[i],
> struct mlx5_rxq_ctrl, rxq);
> rxq_ctrl->flow_mark_n = 0;
> rxq_ctrl->rxq.mark = 0;
> for (j = 0; j != MLX5_FLOW_TUNNEL; ++j)
> rxq_ctrl->flow_tunnels_n[j] = 0;
> rxq_ctrl->rxq.tunnel = 0;
> - ++idx;
> }
> }
>
> --
> 2.11.0
This patch is wrong, (*priv->rxqs)[i] may un-initialised by the
application, the number of queues says how are in used, it does not mean
they are contiguous in the rxqs arrays and this due to the DPDK API
which configure the number of queues with rte_eth_dev_configure()
whereas queues are instantiated with rte_eth_rx_queue_setup() which
takes an position in the array as parameter.
Indeed this code is wrong, idx should always increase whereas i should
only increase if the (*priv->rxqs)[idx] is non null.
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH] net/mlx5: fix representors detection
/%s/phys_switch_id",
> > + ifname);
> > +
> > + file = fopen(phys_port_name, "rb");
> > + if (file == NULL)
> > + goto error;
>
> This goto is not correct.
> Representors will indeed be able to read, but PFs will not be able to
> open the phys_port_name (as there is none).
>From my testing, this is wrong, the PF can open the file, but it cannot
read (verified with strace).
> The phys_switch_id is accessible form both.
>
> I pasted a suggestion below[1], let me know what you think.
>
> > + port_name_set = fscanf(file, "%d%c", &data.port_name, &c) == 2
> > &&
> > + c == '\n';
> > + fclose(file);
> > + file = fopen(phys_switch_id, "rb");
> > + if (file == NULL)
> > + goto error;
> > + port_switch_id_set =
> > + fscanf(file, "%" SCNx64 "%c", &data.switch_id, &c) == 2 &&
> > + c == '\n';
> > + fclose(file);
> > + data.master = port_switch_id_set && !port_name_set;
> > + data.representor = port_switch_id_set && port_name_set;
> > + *info = data;
> > + return 0;
> > +error:
> > + rte_errno = errno;
> > + return -rte_errno;
> > +}
> > --
> > 2.18.0
>
> [1]
> diff --git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c
> index 6cfe02d..b64eb20 100644
> --- a/drivers/net/mlx5/mlx5_ethdev.c
> +++ b/drivers/net/mlx5/mlx5_ethdev.c
> @@ -1328,23 +1328,20 @@ struct ethtool_link_settings {
> ifname);
>
> file = fopen(phys_port_name, "rb");
> - if (file == NULL)
> - goto error;
> - port_name_set = fscanf(file, "%d%c", &data.port_name, &c) == 2 &&
> - c == '\n';
> - fclose(file);
> + if (file) {
> + port_name_set = fscanf(file, "%d%c", &data.port_name, &c) ==
> 2 &&
> + c == '\n';
> + fclose(file);
> + }
> file = fopen(phys_switch_id, "rb");
> - if (file == NULL)
> - goto error;
> - port_switch_id_set =
> - fscanf(file, "%" SCNx64 "%c", &data.switch_id, &c) == 2 &&
> - c == '\n';
> - fclose(file);
> + if (file) {
> + port_switch_id_set =
> + fscanf(file, "%" SCNx64 "%c", &data.switch_id, &c) ==
> 2 &&
> + c == '\n';
> + fclose(file);
> + }
> data.master = port_switch_id_set && !port_name_set;
> data.representor = port_switch_id_set && port_name_set;
Regards,
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH] ethdev: fix missing function in map file
Hi Pablo,
On Fri, Jul 13, 2018 at 08:52:07AM +, De Lara Guarch, Pablo wrote:
> Hi Nelio,
>
> > -Original Message-
> > From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Nelio Laranjeiro
> > Sent: Friday, July 13, 2018 9:25 AM
> > To: dev@dpdk.org; Thomas Monjalon
> > Subject: [dpdk-dev] [PATCH] ethdev: fix missing function in map file
> >
> > Fixes: 4ed05fcd441b ("ethdev: add flow API to expand RSS flows")
> >
> > Signed-off-by: Nelio Laranjeiro
> > Acked-by: Adrien Mazarguil
> > ---
> > lib/librte_ethdev/rte_ethdev_version.map | 1 +
> > lib/librte_ethdev/rte_flow_driver.h | 2 +-
> > 2 files changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/librte_ethdev/rte_ethdev_version.map
> > b/lib/librte_ethdev/rte_ethdev_version.map
> > index 9a0d12d41..0af8731b6 100644
> > --- a/lib/librte_ethdev/rte_ethdev_version.map
> > +++ b/lib/librte_ethdev/rte_ethdev_version.map
> > @@ -251,4 +251,5 @@ EXPERIMENTAL {
> > rte_mtr_policer_actions_update;
> > rte_mtr_stats_read;
> > rte_mtr_stats_update;
> > + rte_flow_expand_rss;
>
> This list is in alphabetical order.
> > };
> > diff --git a/lib/librte_ethdev/rte_flow_driver.h
> > b/lib/librte_ethdev/rte_flow_driver.h
> > index ca675f6d3..141d4acac 100644
> > --- a/lib/librte_ethdev/rte_flow_driver.h
> > +++ b/lib/librte_ethdev/rte_flow_driver.h
> > @@ -169,7 +169,7 @@ struct rte_flow_expand_rss {
> > *
> > * -E2BIG: graph-depth @p graph is too deep.
> > */
> > -int
> > +int __rte_experimental
>
> The implementation in the .c file should also have the experimental tag.
>
> > rte_flow_expand_rss(struct rte_flow_expand_rss *buf, size_t size,
> > const struct rte_flow_item *pattern, uint64_t types,
> > const struct rte_flow_expand_node graph[],
> > --
Will fix both point in a v2.
Thanks,
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH v2 19/20] net/mlx5: add flow MPLS item
On Fri, Jul 06, 2018 at 05:11:31PM -0700, Yongseok Koh wrote:
> On Wed, Jun 27, 2018 at 05:07:51PM +0200, Nelio Laranjeiro wrote:
> > Signed-off-by: Nelio Laranjeiro
> > ---
>[...]
> > + if (spec) {
> > + memcpy(&mpls.val.label, spec, sizeof(mpls.val.label));
> > + memcpy(&mpls.mask.label, mask, sizeof(mpls.mask.label));
> > + /* Remove unwanted bits from values. */
> > + mpls.val.label &= mpls.mask.label;
> > + }
> > + if (size <= flow_size)
>
> Is it guaranteed flow->cur_verbs isn't null if size fits? Could be obvious but
> just want to make sure.
Yes it is.
> > + mlx5_flow_spec_verbs_add(flow, &mpls, size);
> > + mlx5_flow_layers_update(flow, MLX5_FLOW_LAYER_MPLS);
> > + if (layers & MLX5_FLOW_LAYER_OUTER_L4_UDP)
> > + flow->ptype = RTE_PTYPE_TUNNEL_MPLS_IN_GRE | RTE_PTYPE_L4_UDP;
> > + else
> > + flow->ptype = RTE_PTYPE_TUNNEL_MPLS_IN_GRE;
> > + return size;
> > +#endif /* !HAVE_IBV_DEVICE_MPLS_SUPPORT */
> > + return rte_flow_error_set(error, ENOTSUP,
> > + RTE_FLOW_ERROR_TYPE_ITEM,
> > + item,
> > + "MPLS is not supported by Verbs, please"
> > + " update.");
> > +}
> > +
> > /**
> > * Validate items provided by the user.
> > *
> > @@ -1650,6 +1722,9 @@ mlx5_flow_items(struct rte_eth_dev *dev,
> > case RTE_FLOW_ITEM_TYPE_GRE:
> > ret = mlx5_flow_item_gre(items, flow, remain, error);
> > break;
>
> #ifdef HAVE_IBV_DEVICE_MPLS_SUPPORT
>
> > + case RTE_FLOW_ITEM_TYPE_MPLS:
> > + ret = mlx5_flow_item_mpls(items, flow, remain, error);
> > + break;
>
> #endif /* !HAVE_IBV_DEVICE_MPLS_SUPPORT */
>
> How about this?
>[...]
It adds another couple of #ifdef #endif and the final output won't help
much the user, having an error "MPLS is not updated by Verbs, please
update" will help more than "item not supported".
Regards,
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH v2 17/20] net/mlx5: add flow VXLAN-GPE item
On Fri, Jul 06, 2018 at 04:23:26PM -0700, Yongseok Koh wrote: > On Wed, Jun 27, 2018 at 05:07:49PM +0200, Nelio Laranjeiro wrote: > > Signed-off-by: Nelio Laranjeiro > > --- > > drivers/net/mlx5/mlx5_flow.c | 123 ++- > > 1 file changed, 120 insertions(+), 3 deletions(-) > > >[...] > > +/** > > + * Validate VXLAN-GPE layer and possibly create the Verbs specification. > > + * > > + * @param dev > > + * Pointer to Ethernet device. > > + * @param item[in] > > + * Item specification. > > + * @param flow[in, out] > > + * Pointer to flow structure. > > + * @param flow_size[in] > > + * Size in bytes of the available space for to store the flow > > information. > > + * @param error > > + * Pointer to error structure. > > + * > > + * @return > > + * size in bytes necessary for the conversion, a negative errno value > > + * otherwise and rte_errno is set. > > + */ > > +static int > > +mlx5_flow_item_vxlan_gpe(struct rte_eth_dev *dev, > > +const struct rte_flow_item *item, > > +struct rte_flow *flow, const size_t flow_size, > > +struct rte_flow_error *error) > > It is almost same as mlx5_flow_item_vxlan() except for checking > priv->config.l3_vxlan_en. One more difference I noticed is that it doesn't > check > flow->exapnd on validation. Why is that? If that's a mistake, isn't it better > to > make the common code shareable? >[...] The GPE version needs: - l3_vxlan_en - set its own tunnel bit (as in this case the following layer may directly be an L3) Indeed there are some common code (as for the TCP/UDP) but sharing it will be more difficult to read and fix in case of bugs. In addition if this RFC [1] is fully dropped it will be easier to remove the dedicated code when the ITEM in the API will also be removed, it may not be from Mellanox PMD team but from anyone proposing the drop. The chances he breaks anything if the code is shared among several items is high. It is better to have a single function per item/action according to the API directly. Thanks, [1] https://datatracker.ietf.org/doc/draft-quinn-vxlan-gpe/ -- Nélio Laranjeiro 6WIND
Re: [dpdk-dev] [PATCH v2 18/20] net/mlx5: add flow GRE item
Hi Yongseok,
Only discussing here question, other comments are address, as I don't
have any objection I'll make the modification for them.
On Fri, Jul 06, 2018 at 04:46:11PM -0700, Yongseok Koh wrote:
>[...]
> > +
> > /** Handles information leading to a drop fate. */
> > struct mlx5_flow_verbs {
> > LIST_ENTRY(mlx5_flow_verbs) next;
> > @@ -1005,12 +1010,23 @@ mlx5_flow_item_ipv6(const struct rte_flow_item
> > *item, struct rte_flow *flow,
> > item,
> > "L3 cannot follow an L4"
> > " layer");
> > + /*
> > +* IPv6 is not recognised by the NIC inside a GRE tunnel.
> > +* Such support has to be disabled as the rule will be
> > +* accepted. Tested with Mellanox OFED 4.3-3.0.2.1
> > +*/
>
> This comment doesn't look appropriate. Do you think it is a bug of OFED/FW,
> which can be fixed? Or, is it a HW erratum? Let's talk offline.
By the time it was as this Mellanox OFED was the latest GA 4.3-3.0.2.1,
this is no more the case today as it cannot be downloaded anymore. A
verification is still necessary. If the issue is not present anymore
I'll remove the comment with the test.
>[...]
> > +{
> > + unsigned int i;
> > + const enum ibv_flow_spec_type search = IBV_FLOW_SPEC_IPV6;
> > + struct ibv_spec_header *hdr = (struct ibv_spec_header *)
> > + ((uint8_t *)attr + sizeof(struct ibv_flow_attr));
> > +
> > + if (!attr)
> > + return;
> > + for (i = 0; i != attr->num_of_specs; ++i) {
> > + if (hdr->type == search) {
> > + struct ibv_flow_spec_ipv6 *ip =
> > + (struct ibv_flow_spec_ipv6 *)hdr;
> > +
> > + if (!ip->val.next_hdr) {
>
> What if protocol in IP header does have wrong value other than 47
> (IPPROTO_GRE)?
> Shouldn't we have a validation check for it in mlx5_flow_item_gre()?
>[...]
Already added, the same issue occurs also with UDP/TCP. If the user
uses some protocol it must match the following layer, otherwise its
request won't be respected which is a bug.
Thanks,
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH v2 13/20] net/mlx5: add RSS flow action
On Fri, Jul 06, 2018 at 05:35:22PM +, Yongseok Koh wrote:
>
> > On Jul 6, 2018, at 8:59 AM, Nélio Laranjeiro
> > wrote:
> >
> > Hi Yongseok,
> >
> > I am only addressing your questions concerns here, almost all other
> > points I also agree with them.
> >
> > On Thu, Jul 05, 2018 at 07:16:35PM -0700, Yongseok Koh wrote:
> >> On Wed, Jun 27, 2018 at 05:07:45PM +0200, Nelio Laranjeiro wrote:
> >>> Signed-off-by: Nelio Laranjeiro
> >>> ---
> >> [...]
> >>
> >>> + */
> >>> +static void
> >>> +mlx5_flow_layers_update(struct rte_flow *flow, uint32_t layers)
> >>> +{
> >>> + if (flow->expand) {
> >>> + if (flow->cur_verbs)
> >>> + flow->cur_verbs->layers |= layers;
> >>
> >> If flow->cur_verbs is null, does that mean it is a testing call? Then, is
> >> it
> >> unnecessary to update layers for the testing call? Confusing..
> >
> > No it may also happen if the buffer was too small, in any case the code
> > continues its validation.
>
> Okay, understand. Thanks.
> But another question was, if it is a testing call (flow->cur_verbs is null)
> with
> flow->expand being set, then no 'layers' isn't updated in this code. Is it
> okay?
yes it was ok, after I've fixed the issue in the layers themselves,
again no layer position was done when the expanded was enabled.
Thanks,
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH v2 13/20] net/mlx5: add RSS flow action
AG/MARK are the only actions having a specification in Verbs and
> > +* not making part of the packet fate. Due to this specificity and to
> > +* avoid extra variable, their bit in the flow->modifier bit-field are
> > +* disabled here to compute the exact necessary memory those action
> > +* needs.
> > +*/
> > + flow->modifier &= ~(MLX5_FLOW_MOD_FLAG | MLX5_FLOW_MOD_MARK);
>
> Can't understand this well. Is this for the case where the flow is expanded?
> If
> so, why don't you reset flow->modifier in the for loop of mlx5_flow_merge()?
Yes it is, I'll move it.
>[...]
> > + assert(ret > 0);
> > + buf = rte_calloc(__func__, 1, ret, 0);
> > + if (!buf) {
> > + rte_flow_error_set(error, ENOMEM,
> > + RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
> > + NULL,
> > + "not enough memory to expand the RSS flow");
> > + goto error;
> > + }
>
> I'm pretty sure you've already fixed this bug. Validation can't return ENOMEM.
You know me well ;)
Thanks,
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH v2 12/20] net/mlx5: add mark/flag flow action
On Thu, Jul 05, 2018 at 12:56:09PM -0700, Yongseok Koh wrote: >[...] > > > > + if (mark->id >= MLX5_FLOW_MARK_MAX) > > > > + return rte_flow_error_set(error, EINVAL, > > > > + > > > > RTE_FLOW_ERROR_TYPE_ACTION_CONF, > > > > + &mark->id, > > > > + "mark must be between 0 and" > > > > + " 16777199"); > > > > > > Use %d and (MLX5_FLOW_MARK_MAX - 1), instead of fixed string. > > > > It needs an snprintf, rte_flow_error_set() does not accept formatting > > strings. > > I think the following would work but never mind. I'm okay with leaving it as > is. > No need to make a change here. > > #define STRINGIFY(x) #x > #define TOSTRING(x) STRINGIFY(x) > "mark must be between 0 and " > TOSTRING(MLX5_FLOW_MARK_MAX - 1)); > It was to avoid adding a macro, but indeed there is the same kind in mlx4, I'll port them for mlx5. > > >[...] > > Addressing both question here, for the flow_stop() and flow_destroy() > > the process is different, for the stop, the flow remains with the mark > > bit set but all queues must me cleared, there is no comparison to make. > > As you can see, it don't even get a flow, it directly unset the mask bit > > in the Rx queues. > > For the destroy the issue is different, several flows may be using the > > same Rx queues, if one of them will remains and has a mark, then the > > associated queues must keep their mark bit set. > > As the process is different, it would end in two distinct functions and > > each one used by a single function. > > > > For the mlx5_flow_rxq_mark(), the situation is different, the same > > process is make when a flow is created and the flow are started. > > I knew the differences but I just wanted to ask if having a separate function > can be a viable option, e.g., > > mlx5_flow_rxq_mark_set() > mlx5_flow_rxq_mark_clear() > mlx5_flow_rxq_mark_trim() Certainly, the point is those functions have a short life as few patches letter they will be removed. I suppose you prefer to have them and I don't think it will take too much time to add such function, it will make part of the next revision ;). Thanks, -- Nélio Laranjeiro 6WIND
Re: [dpdk-dev] [PATCH v8 2/2] app/testpmd: add NVGRE encap/decap support
On Thu, Jul 05, 2018 at 04:07:28PM +0100, Mohammad Abdul Awal wrote:
>Some nits.
>
>Auto-completion suggestion for values should be wrapped between '<' and
>'>', not '(' and ')'. See all the cases.
>[...]
Right, I'll send a v9 to fix this.
Thanks,
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH v7 0/2] app/testpmd implement VXLAN/NVGRE Encap/Decap
On Wed, Jul 04, 2018 at 03:54:32PM +0100, Ferruh Yigit wrote:
> On 7/2/2018 11:40 AM, Mohammad Abdul Awal wrote:
> >
> > On 27/06/2018 12:45, Nelio Laranjeiro wrote:
> >> This series adds an easy and maintainable configuration version support for
> >> those two actions for 18.08 by using global variables in testpmd to store
> >> the
> >> necessary information for the tunnel encapsulation. Those variables are
> >> used
> >> in conjunction of RTE_FLOW_ACTION_{VXLAN,NVGRE}_ENCAP action to create
> >> easily
> >> the action for flows.
> >>
> >> A common way to use it:
> >>
> >> set vxlan ipv4 4 4 4 127.0.0.1 128.0.0.1 11:11:11:11:11:11
> >> 22:22:22:22:22:22
> >> flow create 0 ingress pattern end actions vxlan_encap / queue index 0 /
> >> end
> >>
> >> set vxlan ipv6 4 4 4 ::1 :: 11:11:11:11:11:11 22:22:22:22:22:22
> >> flow create 0 ingress pattern end actions vxlan_encap / queue index 0 /
> >> end
> >>
> >> set nvgre ipv4 4 127.0.0.1 128.0.0.1 11:11:11:11:11:11 22:22:22:22:22:22
> >> flow create 0 ingress pattern end actions nvgre_encap / queue index 0 /
> >> end
> >>
> >> set nvgre ipv6 4 ::1 :: 11:11:11:11:11:11 22:22:22:22:22:22
> >> flow create 0 ingress pattern end actions nvgre_encap / queue index 0 /
> >> end
> >>
> >> This also replace the proposal done by Mohammad Abdul Awal [1] which
> >> handles
> >> in a more complex way for the same work.
> >>
> >> Note this API has already a modification planned for 18.11 [2] thus those
> >> series should have a limited life for a single release.
> >>
> >> [1] https://dpdk.org/ml/archives/dev/2018-May/101403.html
> >> [2] https://dpdk.org/ml/archives/dev/2018-June/103485.html
> >>
> >> Changes in v7:
> >>
> >> - add missing documentation added in v5 and removed in v6 by mistake.
> >>
> >> Changes in v6:
> >>
> >> - fix compilation under redhat 7.5 with gcc 4.8.5 20150623
> >>
> >> Changes in v5:
> >>
> >> - fix documentation generation.
> >> - add more explanation on how to generate several encapsulated flows.
> >>
> >> Changes in v4:
> >>
> >> - fix big endian issue on vni and tni.
> >> - add samples to the documentation.
> >> - set the VXLAN UDP source port to 0 by default to let the driver generate
> >> it
> >>from the inner hash as described in the RFC 7348.
> >> - use default rte flow mask for each item.
> >>
> >> Changes in v3:
> >>
> >> - support VLAN in the outer encapsulation.
> >> - fix the documentation with missing arguments.
> >>
> >> Changes in v2:
> >>
> >> - add default IPv6 values for NVGRE encapsulation.
> >> - replace VXLAN to NVGRE in comments concerning NVGRE layer.
> >>
> >> Nelio Laranjeiro (2):
> >>app/testpmd: add VXLAN encap/decap support
> >>app/testpmd: add NVGRE encap/decap support
> >>
> >> app/test-pmd/cmdline.c | 252 ++
> >> app/test-pmd/cmdline_flow.c | 274
> >> app/test-pmd/testpmd.c | 32 +++
> >> app/test-pmd/testpmd.h | 32 +++
> >> doc/guides/testpmd_app_ug/testpmd_funcs.rst | 82 ++
> >> 5 files changed, 672 insertions(+)
> >
> >
> > Hi,
> >
> > I have one concern in terms of usability though.
> > In testpmd, the rte_flow command line options have auto-completion with
> > " " format which make using the command very
> > much user friendly.
> >
> > For the command "set vxlan ipv4 4 4 4 127.0.0.1 128.0.0.1
> > 11:11:11:11:11:11 22:22:22:22:22:22", it does not look much user
> > friendly to me. A user may easily lose track of sequence of 9 param
> > items. It would be much user friendly if the options would be like below
> > and has auto-completion.
> >
> > set vxlan ip_ver vni udp_src
> > udp-dst ip_src ip_dst
> > eth_src eth_dst
>
> Hi Nelio, Adrien,
>
> I tend to agree with Awal here, this is to forget/confuse and key-value pairs
> makes it easier to use.
>
> Meanwhile this is an usability improvement and I prefer not to block this
> patch
> for this.
>
> What is your comment on this, how should we proceed?
>
> Thanks,
> ferruh
Hi,
I also agree with this proposal, I'll prepare a v8 with those fix
tokens.
> > This way an user may never feel confused. Can maintainers comment on
> > this point please?
> >
> > Regards,
> > Awal.
Thanks
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH v2 12/20] net/mlx5: add mark/flag flow action
6,7 @@ mlx5_flow_list_create(struct rte_eth_dev *dev,
> > if (ret < 0)
> > goto error;
> > }
> > + mlx5_flow_rxq_mark(dev, flow);
> > TAILQ_INSERT_TAIL(list, flow, next);
> > return flow;
> > error:
> > @@ -1323,8 +1499,31 @@ static void
> > mlx5_flow_list_destroy(struct rte_eth_dev *dev, struct mlx5_flows *list,
> >struct rte_flow *flow)
> > {
> > + struct priv *priv = dev->data->dev_private;
> > + struct rte_flow *rflow;
> > + const uint32_t mask = MLX5_FLOW_MOD_FLAG & MLX5_FLOW_MOD_MARK;
> > + int mark = 0;
> > +
> > mlx5_flow_fate_remove(dev, flow);
> > TAILQ_REMOVE(list, flow, next);
> > + if (!(flow->modifier & mask)) {
> > + rte_free(flow);
> > + return;
> > + }
> > + /*
> > +* When a flow is removed and this flow has a flag/mark modifier, all
> > +* flows needs to be parse to verify if the Rx queue use by the flow
> > +* still need to track the flag/mark request.
> > +*/
>
> When a flow is created, mlx5_flow_rxq_mark() is called. Is there a specific
> reason for not writing a separate function in order to drop rxq->mark bit?
>
> > + TAILQ_FOREACH(rflow, &priv->flows, next) {
> > + if (!(rflow->modifier & mask))
> > + continue;
> > + if (flow->queue == rflow->queue) {
> > + mark = 1;
> > + break;
> > + }
> > + }
> > + (*priv->rxqs)[flow->queue]->mark = !!mark;
>
> mark can be either 0 or 1, then !!mark == mark anyway.
>
> > rte_free(flow);
> > }
> >
> > @@ -1358,10 +1557,19 @@ mlx5_flow_list_flush(struct rte_eth_dev *dev,
> > struct mlx5_flows *list)
> > void
> > mlx5_flow_stop(struct rte_eth_dev *dev, struct mlx5_flows *list)
> > {
> > + struct priv *priv = dev->data->dev_private;
> > struct rte_flow *flow;
> > + unsigned int i;
> > + unsigned int idx;
> >
> > TAILQ_FOREACH_REVERSE(flow, list, mlx5_flows, next)
> > mlx5_flow_fate_remove(dev, flow);
> > + for (idx = 0, i = 0; idx != priv->rxqs_n; ++i) {
> > + if (!(*priv->rxqs)[idx])
> > + continue;
> > + (*priv->rxqs)[idx]->mark = 0;
> > + ++idx;
> > + }
>
> Same question here but looks like this part is being moved to
> mlx5_flow_rxqs_clear() in the future.
Addressing both question here, for the flow_stop() and flow_destroy()
the process is different, for the stop, the flow remains with the mark
bit set but all queues must me cleared, there is no comparison to make.
As you can see, it don't even get a flow, it directly unset the mask bit
in the Rx queues.
For the destroy the issue is different, several flows may be using the
same Rx queues, if one of them will remains and has a mark, then the
associated queues must keep their mark bit set.
As the process is different, it would end in two distinct functions and
each one used by a single function.
For the mlx5_flow_rxq_mark(), the situation is different, the same
process is make when a flow is created and the flow are started.
> > }
> >
> > /**
> > @@ -1386,6 +1594,7 @@ mlx5_flow_start(struct rte_eth_dev *dev, struct
> > mlx5_flows *list)
> > ret = mlx5_flow_fate_apply(dev, flow, &error);
> > if (ret < 0)
> > goto error;
> > + mlx5_flow_rxq_mark(dev, flow);
> > }
> > return 0;
> > error:
> > --
> > 2.18.0
Thanks,
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH v2 07/20] net/mlx5: add flow VLAN item
+ eth.mask.vlan_tag = mask->tci;
> > + eth.val.vlan_tag &= eth.mask.vlan_tag;
> > + eth.val.ether_type = spec->inner_type;
> > + eth.mask.ether_type = mask->inner_type;
> > + eth.val.ether_type &= eth.mask.ether_type;
> > + }
> > + /*
> > +* From verbs perspective an empty VLAN is equivalent
> > +* to a packet without VLAN layer.
> > +*/
> > + if (!eth.mask.vlan_tag)
> > + return rte_flow_error_set(error, EINVAL,
> > + RTE_FLOW_ERROR_TYPE_ITEM_SPEC,
> > + item->spec,
> > + "VLAN cannot be empty");
> > + /* Outer TPID cannot be matched. */
> > + if (eth.mask.ether_type)
> > + return rte_flow_error_set(error, ENOTSUP,
> > + RTE_FLOW_ERROR_TYPE_ITEM_SPEC,
> > + item->spec,
> > + "VLAN TPID matching is not"
> > + " supported");
>
> Not sure 100% but I don't think ether_type means TPID but the inner packet
> type
> coming after the VLAN ID. E.g.
>
> /dmac/smac/0x8100/TCI/0x0800/ipv4...
> ^
> |
I remember to have faced such issue several months ago, from what I
understood it is configurable but there is no way in Verbs to make it.
Currently (after testing) it matches the above logic, I will remove this
check.
> > + if (!(flow->layers & l2m)) {
> > + if (size <= flow_size)
> > + mlx5_flow_spec_verbs_add(flow, ð, size);
> > + } else {
> > + if (flow->verbs.attr)
> > + mlx5_flow_item_vlan_update(flow->verbs.attr, ð);
> > + size = 0; /**< Only an update is done in eth specification. */
>
> Any specific reason to use doxygen style comment only here?
No reason, I'll remove it.
Thanks,
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH v2 04/20] net/mlx5: support flow Ethernet item among with drop action
ired size and execute
> translation only if it gets allocated memory. Plus, how to validate a flow,
> e.g.
> setting flow_size=0 and flow can be null in that case.
Agreed,
> > + *
> > + * @param dev
> > + * Pointer to Ethernet device.
> > + * @param flow
> > + * Pointer an flow structure.
>
> s/an/to
> shouldn't it be [out]?
[in, out] in fact.
> And please mention flow can be null for testing call.
It can be anything until the flow_size is accurate. The testing is not
based on the flow nor the size, but in the fact the conversion may use
more size than the one provided.
> > + * @param flow_size
> > + * Size of the allocated space to store the flow information.
>
> Please mention what happens if the size isn't enough or announce it can be
> intentionally zero in order to validate the flow and to know the required size
> for translation.
>
> > * @param[in] attr
> > * Flow rule attributes.
> > * @param[in] pattern
> > @@ -144,21 +589,39 @@ mlx5_flow_priorities(struct rte_eth_dev *dev)
> > * Perform verbose error reporting if not NULL.
> > *
> > * @return
> > - * A flow on success, NULL otherwise and rte_errno is set.
> > + * The amount of bytes necessary to create the flow, a negative errno
> > value
> > + * otherwise and rte_errno is set.
>
> 'amount of bytes' -> 'size of memory'
> And not clear what is 'otherwise' because there's no definition of success.
Reworded.
>[...]
> > +/**
> > + * Remove the flow.
> > + *
> > + * @param dev
> > + * Pointer to Ethernet device.
> > + * @param flow
> > + * Pointer to flow structure.
> > + */
> > +static void
> > +mlx5_flow_fate_remove(struct rte_eth_dev *dev, struct rte_flow *flow)
>
> Sounds weird. Is it removing a fate of a flow?
> Description is just 'removing a flow'.
>[...]
> > +/**
> > + * Apply the flow.
> > + *
> > + * @param dev
> > + * Pointer to Ethernet device structure.
> > + * @param flow
> > + * Pointer to flow structure.
> > + * @param error
> > + * Pointer to error structure.
> > + *
> > + * @return
> > + * 0 on success, a negative errno value otherwise and rte_errno is set.
> > + */
> > +static int
> > +mlx5_flow_fate_apply(struct rte_eth_dev *dev, struct rte_flow *flow,
> > +struct rte_flow_error *error)
>
> Same here. Either change the name or add more description.
Agreed, "fate" removed in both functions.
>[...]
> > +/**
> > + * Create a flow.
>
> And "insert to the provided list".
Added.
> > + *
> > + * @param dev
> > + * Pointer to Ethernet device.
> > + * @param list
> > + * Pointer to a TAILQ flow list.
> > + * @param[in] attr
> > + * Flow rule attributes.
> > + * @param[in] items
> > + * Pattern specification (list terminated by the END pattern item).
> > + * @param[in] actions
> > + * Associated actions (list terminated by the END action).
> > + * @param[out] error
> > + * Perform verbose error reporting if not NULL.
> > + *
> > + * @return
> > + * A flow on success, NULL otherwise and rte_errno is set.
> > + */
> > +static struct rte_flow *
> > +mlx5_flow_list_create(struct rte_eth_dev *dev,
> > + struct mlx5_flows *list,
> > + const struct rte_flow_attr *attr,
> > + const struct rte_flow_item items[],
> > + const struct rte_flow_action actions[],
> > + struct rte_flow_error *error)
> > +{
>[...]
> > /**
> > @@ -422,9 +1011,8 @@ int
> > mlx5_flow_flush(struct rte_eth_dev *dev,
> > struct rte_flow_error *error __rte_unused)
> > {
> > - struct priv *priv = dev->data->dev_private;
> > -
> > - mlx5_flow_list_flush(dev, &priv->flows);
> > + mlx5_flow_list_flush(dev,
> > +&((struct priv *)dev->data->dev_private)->flows);
>
> Can you keep priv and make this line simpler?
>
> > return 0;
> > }
> >
> > @@ -737,9 +1325,8 @@ mlx5_fdir_filter_update(struct rte_eth_dev *dev,
> > static void
> > mlx5_fdir_filter_flush(struct rte_eth_dev *dev)
> > {
> > - struct priv *priv = dev->data->dev_private;
> > -
> > - mlx5_flow_list_flush(dev, &priv->flows);
> > + mlx5_flow_list_flush(dev,
> > +&((struct priv *)dev->data->dev_private)->flows);
>
> Same here.
It was a request from Adrien. (done anyway).
Thanks,
[1]
https://doc.dpdk.org/guides/prog_guide/rte_flow.html?highlight=rte_flow#actions
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH v2 02/20] net/mlx5: handle drop queues are regular queues
On Tue, Jul 03, 2018 at 10:05:05AM -0700, Yongseok Koh wrote:
> On Tue, Jul 03, 2018 at 09:17:56AM +0200, Nélio Laranjeiro wrote:
> > On Mon, Jul 02, 2018 at 06:07:03PM -0700, Yongseok Koh wrote:
> > > On Wed, Jun 27, 2018 at 05:07:34PM +0200, Nelio Laranjeiro wrote:
> [...]
> > flow_drop_queue is also confusing as it is a drop hash rx queue, it can
> > be used without a flow as a regular queue.
> > Renaming it to drop_hrxq.
>
> Not so much critical to me but the entry has a field having repeating name.
> priv->drop_hrxq.hrxq and priv->drop_hrxq.rxq
> Sounds still confusing...
>
> > > > +/**
> > > > + * Release a drop Rx queue Verbs object.
> > > > + *
> > > > + * @param dev
> > > > + * Pointer to Ethernet device.
> > > > + * @param rxq
> > > > + * Pointer to the drop Verbs Rx queue.
> > > > + *
> > > > + * @return
> > > > + * The Verbs object initialised, NULL otherwise and rte_errno is set.
> > > > + */
> > > > +void
> > > > +mlx5_rxq_ibv_drop_release(struct rte_eth_dev *dev, struct mlx5_rxq_ibv
> > > > *rxq)
> > >
> > > If rxq for drop is saved in priv->drop.rxq, then why does it have to get
> > > rxq
> > > pointer as an argument? Looks redundant.
> > >[...]
> >
> > Like for all hrxqs, indirection tables, rxqs, which are stored in priv
> > inside a list or an array.
>
> However, the assumption is there's only one drop queue while the regular ones
> have multiple instances.
>
> > Priv is used as a storage place which is only access through
> > *_{new,get,release} functions.
>
> Yes, that's what I'm telling you. *_{new,get,release}() accesses priv, then
> why
> the pointer (which is saved in priv) is needed as an argument?
>
> > This is also to keep a consistency between regular hrxqs, and drop hrxq
> > also.
> Not sure why that consistency has to be kept.
>
> int mlx5_rxq_release(struct rte_eth_dev *dev, uint16_t idx);
> int mlx5_hrxq_release(struct rte_eth_dev *dev, struct mlx5_hrxq *hxrq);
>
> mlx5_rxq_release() takes index as the instances are stored in an array and
> mlx5_hrxq_release() takes pointer as the instances are stored in a list.
>
> Then, what if there's only one instance and no need to search?
> Not taking such an argument sounds more consistent...
>[...]
Even if I prefer to keep consistency about identical functionality (in
this case creating an hash Rx queue), I'll make the changes to please
you.
Regards,
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH v2 02/20] net/mlx5: handle drop queues are regular queues
On Mon, Jul 02, 2018 at 06:07:03PM -0700, Yongseok Koh wrote:
> On Wed, Jun 27, 2018 at 05:07:34PM +0200, Nelio Laranjeiro wrote:
> > Drop queues are essentially used in flows due to Verbs API, the
> > information if the fate of the flow is a drop or not is already present
> > in the flow. Due to this, drop queues can be fully mapped on regular
> > queues.
>
> The title doesn't look good. Maybe typo?
> net/mlx5: handle drop queues are regular queues
> 'are' -> 'as'?
[...]
Right,
> > struct priv {
> > LIST_ENTRY(priv) mem_event_cb; /* Called by memory event callback. */
> > struct rte_eth_dev_data *dev_data; /* Pointer to device data. */
> > @@ -175,7 +178,7 @@ struct priv {
> > struct rte_intr_handle intr_handle; /* Interrupt handler. */
> > unsigned int (*reta_idx)[]; /* RETA index table. */
> > unsigned int reta_idx_n; /* RETA index size. */
> > - struct mlx5_hrxq_drop *flow_drop_queue; /* Flow drop queue. */
> > + struct mlx5_drop drop; /* Flow drop queues. */
>
> priv->drop sounds strange. Why did you change the name?
> How about priv->flow_drop if you didn't like the full name?
>[...]
flow_drop_queue is also confusing as it is a drop hash rx queue, it can
be used without a flow as a regular queue.
Renaming it to drop_hrxq.
> > +/**
> > + * Release a drop Rx queue Verbs object.
> > + *
> > + * @param dev
> > + * Pointer to Ethernet device.
> > + * @param rxq
> > + * Pointer to the drop Verbs Rx queue.
> > + *
> > + * @return
> > + * The Verbs object initialised, NULL otherwise and rte_errno is set.
> > + */
> > +void
> > +mlx5_rxq_ibv_drop_release(struct rte_eth_dev *dev, struct mlx5_rxq_ibv
> > *rxq)
>
> If rxq for drop is saved in priv->drop.rxq, then why does it have to get rxq
> pointer as an argument? Looks redundant.
>[...]
Like for all hrxqs, indirection tables, rxqs, which are stored in priv
inside a list or an array. Priv is used as a storage place which is
only access through *_{new,get,release} functions.
This is also to keep a consistency between regular hrxqs, and drop hrxq also.
> > +void
> > +mlx5_ind_table_ibv_drop_release(struct rte_eth_dev *dev,
> > + struct mlx5_ind_table_ibv *ind_tbl)
>
> ind_tbl is a redundant argument. Can be referenced by
> priv->drop.hrxq->ind_table.
>[...]
Ditto.
> > +/**
> > + * Release a drop hash Rx queue.
> > + *
> > + * @param dev
> > + * Pointer to Ethernet device.
> > + * @param hrxq
> > + * Pointer to Hash Rx queue to release.
> > + */
> > +void
> > +mlx5_hrxq_drop_release(struct rte_eth_dev *dev, struct mlx5_hrxq *hrxq)
>[...]
>
> hrxq is a redundant argument. Can be referenced by priv->drop.hrxq.
>[...]
Ditto.
Thanks,
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH 0/6] net/mlx5: add support for switch flow rules
On Wed, Jun 27, 2018 at 08:08:08PM +0200, Adrien Mazarguil wrote:
> This series adds support for switch flow rules, that is, rte_flow rules
> applied to mlx5 devices at the switch level.
>
> It allows applications to offload traffic redirection between DPDK ports in
> hardware, while optionally modifying it (e.g. performing encap/decap).
>
> For this to work, involved DPDK ports must be part of the same switch
> domain, as is the case with port representors, and the transfer attribute
> must be requested on flow rules.
>
> Also since the mlx5 switch is controlled through Netlink instead of Verbs,
> and given how tedious formatting Netlink messages is, a new dependency is
> added to mlx5: libmnl. See relevant patch.
>
> This series depends on Nelio's mlx5 flow engine rework ("net/mlx5: flow
> rework" [1][2]) which must be applied first.
>
> [1] https://patches.dpdk.org/project/dpdk/list/?series=268
> [2] https://mails.dpdk.org/archives/dev/2018-June/105499.html
>
> Adrien Mazarguil (6):
> net/mlx5: lay groundwork for switch offloads
> net/mlx5: add framework for switch flow rules
> net/mlx5: add fate actions to switch flow rules
> net/mlx5: add L2-L4 pattern items to switch flow rules
> net/mlx5: add VLAN item and actions to switch flow rules
> net/mlx5: add port ID pattern item to switch flow rules
>
> drivers/net/mlx5/Makefile |2 +
> drivers/net/mlx5/mlx5.c | 32 +
> drivers/net/mlx5/mlx5.h | 28 +
> drivers/net/mlx5/mlx5_flow.c| 113
> drivers/net/mlx5/mlx5_nl_flow.c | 1126 ++
> mk/rte.app.mk |2 +-
> 6 files changed, 1302 insertions(+), 1 deletion(-)
> create mode 100644 drivers/net/mlx5/mlx5_nl_flow.c
>
> --
> 2.11.0
Acked-by: Nelio Laranjeiro
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH] net/mlx5: fix invalid error check
On Wed, Jun 27, 2018 at 11:20:52AM +0200, Adrien Mazarguil wrote:
> Since its return type is unsigned, if_nametoindex() returns 0 in case of
> error, never -1.
>
> Fixes: ccdcba53a3f4 ("net/mlx5: use Netlink to add/remove MAC addresses")
> Cc: Nelio Laranjeiro
> Cc: sta...@dpdk.org
>
> Signed-off-by: Adrien Mazarguil
Acked-by: Nelio Laranjeiro
> ---
> drivers/net/mlx5/mlx5_ethdev.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c
> index 90488af33..ebe5cb6e3 100644
> --- a/drivers/net/mlx5/mlx5_ethdev.c
> +++ b/drivers/net/mlx5/mlx5_ethdev.c
> @@ -192,13 +192,13 @@ int
> mlx5_ifindex(const struct rte_eth_dev *dev)
> {
> char ifname[IF_NAMESIZE];
> - int ret;
> + unsigned int ret;
>
> ret = mlx5_get_ifname(dev, &ifname);
> if (ret)
> return ret;
> ret = if_nametoindex(ifname);
> - if (ret == -1) {
> + if (ret == 0) {
> rte_errno = errno;
> return -rte_errno;
> }
> --
> 2.11.0
Thanks,
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH v6 0/2] app/testpmd implement VXLAN/NVGRE Encap/Decap
Sorry, I've messed up with my local branches. I will send a v7 which only fixes the compilation issues on redhat. Thanks, -- Nélio Laranjeiro 6WIND
Re: [dpdk-dev] [PATCH v4 2/2] app/testpmd: add NVGRE encap/decap support
Hi, On Tue, Jun 26, 2018 at 12:48:42PM +, Iremonger, Bernard wrote: > Hi Nelio, > > > -Original Message- > > From: Nelio Laranjeiro [mailto:nelio.laranje...@6wind.com] > > Sent: Thursday, June 21, 2018 8:14 AM > > To: dev@dpdk.org; Adrien Mazarguil ; Lu, > > Wenzhuo ; Wu, Jingjing ; > > Iremonger, Bernard ; Awal, Mohammad Abdul > > ; Ori Kam ; > > Stephen Hemminger > > Subject: [PATCH v4 2/2] app/testpmd: add NVGRE encap/decap support > > > > Due to the complex NVGRE_ENCAP flow action and based on the fact testpmd > > does not allocate memory, this patch adds a new command in testpmd to > > initialise a global structure containing the necessary information to make > > the > > outer layer of the packet. This same global structure will then be used by > > the > > flow command line in testpmd when the action nvgre_encap will be parsed, at > > this point, the conversion into such action becomes trivial. > > > > This global structure is only used for the encap action. > > > > Signed-off-by: Nelio Laranjeiro > > --- > > app/test-pmd/cmdline.c | 118 ++ > > app/test-pmd/cmdline_flow.c | 129 > > app/test-pmd/testpmd.c | 15 +++ > > app/test-pmd/testpmd.h | 15 +++ > > doc/guides/testpmd_app_ug/testpmd_funcs.rst | 37 ++ > > 5 files changed, 314 insertions(+) > > > > > diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst > > b/doc/guides/testpmd_app_ug/testpmd_funcs.rst > > index 2743043d3..17e0fef63 100644 > > --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst > > +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst > > @@ -1542,6 +1542,14 @@ Configure the outer layer to encapsulate a packet > > inside a VXLAN tunnel:: > > testpmd> set vxlan ipv4|ipv6 (vni) (udp-src) (udp-dst) (ip-src) (ip-dst) > > (mac-src) > > (mac-dst) > > testpmd> set vxlan-with-vlan ipv4|ipv6 (vni) (udp-src) (udp-dst) (ip-src) > > (ip-dst) > > (vlan-tci) (mac-src) (mac-dst) > > > > +Config NVGRE Encap outer layers > > +~~~ > > + > > +Configure the outer layer to encapsulate a packet inside a NVGRE tunnel:: > > + > > + testpmd> set nvgre ipv4|ipv6 (tni) (ip-src) (ip-dst) (mac-src) > > + testpmd> (mac-dst) set nvgre-with-vlan ipv4|ipv6 (tni) (ip-src) > > + testpmd> (ip-dst) (vlan-tci) (mac-src) (mac-dst) > > + > > Port Functions > > -- > > > > @@ -3663,6 +3671,12 @@ This section lists supported actions and their > > attributes, if any. > > - ``vxlan_decap``: Performs a decapsulation action by stripping all > > headers of > >the VXLAN tunnel network overlay from the matched flow. > > > > +- ``nvgre_encap``: Performs a NVGRE encapsulation, outer layer > > +configuration > > + is done through `Config NVGRE Encap outer layers`_. > > + > > +- ``nvgre_decap``: Performs a decapsulation action by stripping all > > +headers of > > + the NVGRE tunnel network overlay from the matched flow. > > + > > Destroying flow rules > > ~ > > > > @@ -3950,6 +3964,29 @@ IPv6 VXLAN outer header:: > >testpmd> set vxlan-with-vlan ipv6 4 4 4 ::1 :: 34 11:11:11:11:11:11 > > 22:22:22:22:22:22 > >testpmd> flow create 0 ingress pattern end actions vxlan_encap / queue > > index > > 0 / end > > > > +Sample NVGRE encapsulation rule > > +~~~ > > + > > +NVGRE encapsulation outer layer has default value pre-configured in > > +testpmd source code, those can be changed by using the following commands:: > > make doc-guides-html > sphinx processing guides-html... > dpdk/doc/guides/testpmd_app_ug/testpmd_funcs.rst:3973: WARNING: Literal block > expected; none found. I will fix it in the v5 among with the VXLAN issue you pointed out in the second patch. > > > + > > +IPv4 NVGRE outer header:: > > + > > + testpmd> set nvgre ipv4 4 127.0.0.1 128.0.0.1 11:11:11:11:11:11 > > + 22:22:22:22:22:22 testpmd> flow create 0 ingress pattern end actions > > + nvgre_encap / queue index 0 / end > > + > > + testpmd> set nvgre-with-vlan 4 127.0.0.1 128.0.0.1 34 > > + 11:11:11:11:11:11 22:22:22:22:22:22 testpmd> flow create 0 ingress > > + pattern end actions vxlan_encap / queue index 0 / end > > + > > +IPv6 NVGRE outer header:: > > + > > + testpmd> set nvgre ipv6 4 ::1 :: 11:11:11:11:11:11 > > + 22:22:22:22:22:22 testpmd> flow create 0 ingress pattern end actions > > + vxlan_encap / queue index 0 / end > > + > > + testpmd> set nvgre-with-vlan ipv6 4 ::1 :: 34 11:11:11:11:11:11 > > + 22:22:22:22:22:22 testpmd> flow create 0 ingress pattern end actions > > + vxlan_encap / queue index 0 / end > > + > > + > > BPF Functions > > -- > > > > -- > > 2.18.0.rc2 > > Regards, > > Bernard. > Thanks, -- Nélio Laranjeiro 6WIND
Re: [dpdk-dev] [PATCH v4 0/2] app/testpmd implement VXLAN/NVGRE Encap/Decap
On Fri, Jun 22, 2018 at 11:19:14AM +0100, Mohammad Abdul Awal wrote:
> On 22/06/2018 10:08, Nélio Laranjeiro wrote:
> > On Fri, Jun 22, 2018 at 09:51:15AM +0100, Mohammad Abdul Awal wrote:
> > >
> > > On 22/06/2018 09:31, Nélio Laranjeiro wrote:
> > > > On Fri, Jun 22, 2018 at 08:42:10AM +0100, Mohammad Abdul Awal wrote:
> > > > > Hi Nelio,
> > > > >
> > > > >
> > > > > On 21/06/2018 08:13, Nelio Laranjeiro wrote:
> > > > > > This series adds an easy and maintainable configuration version
> > > > > > support for
> > > > > > those two actions for 18.08 by using global variables in testpmd to
> > > > > > store the
> > > > > > necessary information for the tunnel encapsulation. Those
> > > > > > variables are used
> > > > > > in conjunction of RTE_FLOW_ACTION_{VXLAN,NVGRE}_ENCAP action to
> > > > > > create easily
> > > > > > the action for flows.
> > > > > >
> > > > > > A common way to use it:
> > > > > >
> > > > > > set vxlan ipv4 4 4 4 127.0.0.1 128.0.0.1 11:11:11:11:11:11
> > > > > > 22:22:22:22:22:22
> > > > > > flow create 0 ingress pattern end actions vxlan_encap / queue
> > > > > > index 0 / end
> > > > > This way we can define only one tunnel for all the flows. This is not
> > > > > a
> > > > > convenient for testing a scenario (e.g. mutiport or switch) with
> > > > > multiple
> > > > > tunnels. Isn't it?
> > > > Hi Awal.
> > > >
> > > > The "set vxlan" command will just configure the outer VXLAN tunnel to be
> > > > used, when the "flow" command is invoked, it will use the VXLAN tunnel
> > > > information and create a valid VXLAN_ENCAP action. For instance:
> > > >
> > > >testpmd> set vxlan ipv4 4 4 4 127.0.0.1 128.0.0.1 11:11:11:11:11:11
> > > > 22:22:22:22:22:22
> > > >testpmd> flow create 0 ingress pattern end actions vxlan_encap /
> > > > queue index 0 / end
> > > >testpmd> set vxlan ipv6 4 34 42 ::1 :: 80:12:13:14:15:16
> > > > 22:22:22:22:22:22
> > > >testpmd> flow create 0 ingress pattern end actions vxlan_encap /
> > > > queue index 0 / end
> > > >
> > > > will create two VLXAN_ENCAP flow one with IPv4 tunnel the second one
> > > > with an IPv6. Whereas:
> > > >
> > > >testpmd> set vxlan ipv4 4 4 4 127.0.0.1 128.0.0.1 11:11:11:11:11:11
> > > > 22:22:22:22:22:22
> > > >testpmd> flow create 0 ingress pattern eth / ipv4 src is 10.2.3.4 /
> > > > end
> > > > actions vxlan_encap / queue index 0 / end
> > > >testpmd> flow create 0 ingress pattern eth / ipv4 src is 20.2.3.4 /
> > > > end
> > > > actions vxlan_encap / queue index 0 / end
> > > >
> > > > will encapsulate the packets having as IPv4 source IP 10.2.3.4 and
> > > > 20.2.3.4 with the same VXLAN tunnel headers.
> > > I understand that the same IPv4 tunnel will be used for both flows in your
> > > example above. I have the following questions.
> > >
> > > 1) How can we create two or more IPv4 (or IPv6) tunnel?
> > > 1) How can we make the flows to use different IPv4 tunnels?
> > > As an example,
> > >
> > > testpmd> set vxlan ipv4 4 4 4 127.0.0.1 128.0.0.1 11:11:11:11:11:11
> > > 22:22:22:22:22:22
> > > testpmd> set vxlan ipv4 4 4 4 127.0.0.1 128.0.0.1 33:33:33:33:33:33
> > > 44:44:44:44:44:44
> > > testpmd> flow create 0 ingress pattern end actions vxlan_encap > > tunnel?> / queue index 0 / end
> > > testpmd> flow create 0 ingress pattern end actions vxlan_encap > > tunnel?> / queue index 0 / end
> > >
> > Doing this, the flows will use the same tunnel, you must do:
> >
> > testpmd> set vxlan ipv4 4 4 4 127.0.0.1 128.0.0.1 11:11:11:11:11:11
> > 22:22:22:22:22:22
> > testpmd> flow create 0 ingress pattern end actions vxlan_encap > tunnel?> / queue index 0 / end
> > testpmd> set vxlan ipv4 4 4 4 127.0.0.1 128.0.0.1 33:33:33:33:33:33
> > 44:44:44:44:44:44
> > testpmd> flow create 0 ingress pattern end actions vxlan_encap > tunnel?> / queue index 0 / end
> >
> > to have what you want.
> OK, thanks for the clarification. So, since there will be only one global
> instance of the tunnel, for any subsequent "set vxlan" operations, the
> tunnel created from the last last operation will be used. May be it should
> be cleared in the description/documentation?
Will add it in the v5.
> > > Is it possible?
> > Regards,
> >
>
Thanks,
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH] net/mlx5: separate generic tunnel TSO from the standard one
On Mon, Jun 25, 2018 at 11:23:22AM +, Shahaf Shuler wrote: > Monday, June 25, 2018 9:41 AM , Nélio Laranjeiro: > > Subject: Re: [PATCH] net/mlx5: separate generic tunnel TSO from the > > standard one > > > > On Sun, Jun 24, 2018 at 09:22:26AM +0300, Shahaf Shuler wrote: > > > The generic tunnel TSO was depended in the regular one capabilities to > > > be enabled. > > > > > > Cc: sta...@dpdk.org > > > > > > Signed-off-by: Shahaf Shuler > > > Acked-by: Yongseok Koh > > > --- > > > drivers/net/mlx5/mlx5_txq.c | 13 + > > > 1 file changed, 9 insertions(+), 4 deletions(-) > > > > > [...] > > > > - txq_ctrl->txq.tunnel_en = config->tunnel_en; > > > + txq_ctrl->txq.tunnel_en = config->tunnel_en | config->swp; > > > txq_ctrl->txq.swp_en = ((DEV_TX_OFFLOAD_IP_TNL_TSO | > > >DEV_TX_OFFLOAD_UDP_TNL_TSO | > > >DEV_TX_OFFLOAD_OUTER_IPV4_CKSUM) & > > > -- > > > 2.12.0 > > > > > > > Is not it a fix? > > Well, more like optimization. To be less strict on when to enable the > generic tunnel TSO. > I can rephrase the title if you insist. I was asking due to the CC'ed stable, which is generally used when the it is a fix. I don't know how the stable maintainers trigger such patch, that why I am asking. I am not insisting in any thing here. By the way: Acked-by: Nelio Laranjeiro -- Nélio Laranjeiro 6WIND
Re: [dpdk-dev] [PATCH] net/mlx5: separate generic tunnel TSO from the standard one
On Sun, Jun 24, 2018 at 09:22:26AM +0300, Shahaf Shuler wrote:
> The generic tunnel TSO was depended in the regular one capabilities to
> be enabled.
>
> Cc: sta...@dpdk.org
>
> Signed-off-by: Shahaf Shuler
> Acked-by: Yongseok Koh
> ---
> drivers/net/mlx5/mlx5_txq.c | 13 +
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/mlx5/mlx5_txq.c b/drivers/net/mlx5/mlx5_txq.c
> index 068f36d99d..669b91319f 100644
> --- a/drivers/net/mlx5/mlx5_txq.c
> +++ b/drivers/net/mlx5/mlx5_txq.c
> @@ -113,15 +113,20 @@ mlx5_get_tx_port_offloads(struct rte_eth_dev *dev)
>DEV_TX_OFFLOAD_TCP_CKSUM);
> if (config->tso)
> offloads |= DEV_TX_OFFLOAD_TCP_TSO;
> + if (config->swp) {
> + if (config->hw_csum)
> + offloads |= DEV_TX_OFFLOAD_OUTER_IPV4_CKSUM;
> + if (config->tso)
> + offloads |= (DEV_TX_OFFLOAD_IP_TNL_TSO |
> + DEV_TX_OFFLOAD_UDP_TNL_TSO);
> + }
> +
> if (config->tunnel_en) {
> if (config->hw_csum)
> offloads |= DEV_TX_OFFLOAD_OUTER_IPV4_CKSUM;
> if (config->tso)
> offloads |= (DEV_TX_OFFLOAD_VXLAN_TNL_TSO |
>DEV_TX_OFFLOAD_GRE_TNL_TSO);
> - if (config->swp)
> - offloads |= (DEV_TX_OFFLOAD_IP_TNL_TSO |
> - DEV_TX_OFFLOAD_UDP_TNL_TSO);
> }
> return offloads;
> }
> @@ -707,7 +712,7 @@ txq_set_params(struct mlx5_txq_ctrl *txq_ctrl)
> max_tso_inline);
> txq_ctrl->txq.tso_en = 1;
> }
> - txq_ctrl->txq.tunnel_en = config->tunnel_en;
> + txq_ctrl->txq.tunnel_en = config->tunnel_en | config->swp;
> txq_ctrl->txq.swp_en = ((DEV_TX_OFFLOAD_IP_TNL_TSO |
> DEV_TX_OFFLOAD_UDP_TNL_TSO |
>DEV_TX_OFFLOAD_OUTER_IPV4_CKSUM) &
> --
> 2.12.0
>
Is not it a fix?
Regards,
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH v4 0/2] app/testpmd implement VXLAN/NVGRE Encap/Decap
On Fri, Jun 22, 2018 at 09:51:15AM +0100, Mohammad Abdul Awal wrote:
>
>
> On 22/06/2018 09:31, Nélio Laranjeiro wrote:
> > On Fri, Jun 22, 2018 at 08:42:10AM +0100, Mohammad Abdul Awal wrote:
> > > Hi Nelio,
> > >
> > >
> > > On 21/06/2018 08:13, Nelio Laranjeiro wrote:
> > > > This series adds an easy and maintainable configuration version support
> > > > for
> > > > those two actions for 18.08 by using global variables in testpmd to
> > > > store the
> > > > necessary information for the tunnel encapsulation. Those variables
> > > > are used
> > > > in conjunction of RTE_FLOW_ACTION_{VXLAN,NVGRE}_ENCAP action to create
> > > > easily
> > > > the action for flows.
> > > >
> > > > A common way to use it:
> > > >
> > > >set vxlan ipv4 4 4 4 127.0.0.1 128.0.0.1 11:11:11:11:11:11
> > > > 22:22:22:22:22:22
> > > >flow create 0 ingress pattern end actions vxlan_encap / queue index
> > > > 0 / end
> > > This way we can define only one tunnel for all the flows. This is not a
> > > convenient for testing a scenario (e.g. mutiport or switch) with multiple
> > > tunnels. Isn't it?
> > Hi Awal.
> >
> > The "set vxlan" command will just configure the outer VXLAN tunnel to be
> > used, when the "flow" command is invoked, it will use the VXLAN tunnel
> > information and create a valid VXLAN_ENCAP action. For instance:
> >
> > testpmd> set vxlan ipv4 4 4 4 127.0.0.1 128.0.0.1 11:11:11:11:11:11
> > 22:22:22:22:22:22
> > testpmd> flow create 0 ingress pattern end actions vxlan_encap / queue
> > index 0 / end
> > testpmd> set vxlan ipv6 4 34 42 ::1 :: 80:12:13:14:15:16
> > 22:22:22:22:22:22
> > testpmd> flow create 0 ingress pattern end actions vxlan_encap / queue
> > index 0 / end
> >
> > will create two VLXAN_ENCAP flow one with IPv4 tunnel the second one
> > with an IPv6. Whereas:
> >
> > testpmd> set vxlan ipv4 4 4 4 127.0.0.1 128.0.0.1 11:11:11:11:11:11
> > 22:22:22:22:22:22
> > testpmd> flow create 0 ingress pattern eth / ipv4 src is 10.2.3.4 / end
> > actions vxlan_encap / queue index 0 / end
> > testpmd> flow create 0 ingress pattern eth / ipv4 src is 20.2.3.4 / end
> > actions vxlan_encap / queue index 0 / end
> >
> > will encapsulate the packets having as IPv4 source IP 10.2.3.4 and
> > 20.2.3.4 with the same VXLAN tunnel headers.
>
> I understand that the same IPv4 tunnel will be used for both flows in your
> example above. I have the following questions.
>
> 1) How can we create two or more IPv4 (or IPv6) tunnel?
> 1) How can we make the flows to use different IPv4 tunnels?
> As an example,
>
> testpmd> set vxlan ipv4 4 4 4 127.0.0.1 128.0.0.1 11:11:11:11:11:11
> 22:22:22:22:22:22
> testpmd> set vxlan ipv4 4 4 4 127.0.0.1 128.0.0.1 33:33:33:33:33:33
> 44:44:44:44:44:44
> testpmd> flow create 0 ingress pattern end actions vxlan_encap tunnel?> / queue index 0 / end
> testpmd> flow create 0 ingress pattern end actions vxlan_encap tunnel?> / queue index 0 / end
>
Doing this, the flows will use the same tunnel, you must do:
testpmd> set vxlan ipv4 4 4 4 127.0.0.1 128.0.0.1 11:11:11:11:11:11
22:22:22:22:22:22
testpmd> flow create 0 ingress pattern end actions vxlan_encap
/ queue index 0 / end
testpmd> set vxlan ipv4 4 4 4 127.0.0.1 128.0.0.1 33:33:33:33:33:33
44:44:44:44:44:44
testpmd> flow create 0 ingress pattern end actions vxlan_encap / queue index 0 / end
to have what you want.
> Is it possible?
Regards,
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH v4 0/2] app/testpmd implement VXLAN/NVGRE Encap/Decap
On Fri, Jun 22, 2018 at 08:42:10AM +0100, Mohammad Abdul Awal wrote:
> Hi Nelio,
>
>
> On 21/06/2018 08:13, Nelio Laranjeiro wrote:
> > This series adds an easy and maintainable configuration version support for
> > those two actions for 18.08 by using global variables in testpmd to store
> > the
> > necessary information for the tunnel encapsulation. Those variables are
> > used
> > in conjunction of RTE_FLOW_ACTION_{VXLAN,NVGRE}_ENCAP action to create
> > easily
> > the action for flows.
> >
> > A common way to use it:
> >
> > set vxlan ipv4 4 4 4 127.0.0.1 128.0.0.1 11:11:11:11:11:11
> > 22:22:22:22:22:22
> > flow create 0 ingress pattern end actions vxlan_encap / queue index 0 /
> > end
>
> This way we can define only one tunnel for all the flows. This is not a
> convenient for testing a scenario (e.g. mutiport or switch) with multiple
> tunnels. Isn't it?
Hi Awal.
The "set vxlan" command will just configure the outer VXLAN tunnel to be
used, when the "flow" command is invoked, it will use the VXLAN tunnel
information and create a valid VXLAN_ENCAP action. For instance:
testpmd> set vxlan ipv4 4 4 4 127.0.0.1 128.0.0.1 11:11:11:11:11:11
22:22:22:22:22:22
testpmd> flow create 0 ingress pattern end actions vxlan_encap / queue index 0
/ end
testpmd> set vxlan ipv6 4 34 42 ::1 :: 80:12:13:14:15:16 22:22:22:22:22:22
testpmd> flow create 0 ingress pattern end actions vxlan_encap / queue index 0
/ end
will create two VLXAN_ENCAP flow one with IPv4 tunnel the second one
with an IPv6. Whereas:
testpmd> set vxlan ipv4 4 4 4 127.0.0.1 128.0.0.1 11:11:11:11:11:11
22:22:22:22:22:22
testpmd> flow create 0 ingress pattern eth / ipv4 src is 10.2.3.4 / end
actions vxlan_encap / queue index 0 / end
testpmd> flow create 0 ingress pattern eth / ipv4 src is 20.2.3.4 / end
actions vxlan_encap / queue index 0 / end
will encapsulate the packets having as IPv4 source IP 10.2.3.4 and
20.2.3.4 with the same VXLAN tunnel headers.
Regards,
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH] net/mlx5: fix Rss level validation
On Wed, Jun 20, 2018 at 05:34:23AM +, Shahaf Shuler wrote:
> Adding the maintainers,
>
> Tuesday, June 19, 2018 4:09 PM, Raslan Darawsheh:
> >Subject: [PATCH] net/mlx5: fix Rss level validation
>
> Rss -> RSS
>
> >
> >When setting the level in rss action it's checking for the value stored in
> >the
> >parser which is set to 0 by default.
> >
> >this change the check to be for the requested action insted.
>
> this -> This
> instead -> instead
>
> >
> >Fixes: d4a40518 ("net/mlx5: support tunnel RSS level")
> >
> >Cc: sta...@dpdk.org
> >Signed-off-by: Raslan Darawsheh
> >---
> > drivers/net/mlx5/mlx5_flow.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> >diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
> >index 994be05..40df633 100644
> >--- a/drivers/net/mlx5/mlx5_flow.c
> >+++ b/drivers/net/mlx5/mlx5_flow.c
> >@@ -738,7 +738,7 @@ mlx5_flow_convert_actions(struct rte_eth_dev *dev,
> > return -rte_errno;
> > }
> > #ifndef HAVE_IBV_DEVICE_TUNNEL_SUPPORT
> >-if (parser->rss_conf.level > 1) {
> >+if (rss->level > 1) {
> > rte_flow_error_set(error, EINVAL,
> >
> >RTE_FLOW_ERROR_TYPE_ACTION,
> >actions,
> >@@ -747,7 +747,7 @@ mlx5_flow_convert_actions(struct rte_eth_dev *dev,
> > return -rte_errno;
> > }
> > #endif
> >-if (parser->rss_conf.level > 2) {
> >+if (rss->level > 2) {
> > rte_flow_error_set(error, EINVAL,
> >
> >RTE_FLOW_ERROR_TYPE_ACTION,
> >actions,
> >--
>
> The above changes (if they are the only one) can be address by me when I
> apply the patch.
> Will wait few days for the maintainers comments.
>
> Acked-by: Shahaf Shuler
We should answer the same as the other patches, there is already a
re-work remodeling the whole engine, this patch remains relevant for
stables branches, but no more for mainline.
Regards,
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH] net/mlx5: fix error number handling
On Tue, Jun 19, 2018 at 11:00:25PM +, Yongseok Koh wrote: >[...] > >>> I did not understood it was only a concern about the success of the > >>> function, even it is better to avoid as most as possible a useless > >>> store, in this specific case, as errno (rte_errno) has a garbage value, > >>> I fully agree with you. > >> > >> Nelio, > >> > >> Do you still want me to make any change for this patch? > >> Let me know if any. > > > > With your modification the function documentation is no more accurate as > > rte_errno is always set. > > I still don't agree with that but will send out v2. It's not a big deal. What I meant is, you could have only changed the function documentation. @return 0 on success with rte_errno always set, negative errno otherwise. letting the function documentation saying rte_errno is only modified in case of error whereas it is not is a bug or in the documentation or in the code, but as a function must respect its documentation, it would have raised a bug in the code itself. Regards, -- Nélio Laranjeiro 6WIND
Re: [dpdk-dev] [PATCH v2] net/mlx5: fix error number handling
On Tue, Jun 19, 2018 at 04:13:13PM -0700, Yongseok Koh wrote:
> rte_errno should be saved only if error has occurred because rte_errno
> could have garbage value.
>
> Fixes: a6d83b6a9209 ("net/mlx5: standardize on negative errno values")
> Cc: sta...@dpdk.org
>
> Signed-off-by: Yongseok Koh
Acked-by: Nelio Laranjeiro
> ---
> drivers/net/mlx5/mlx5_flow.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
> index 994be05be..45207d70e 100644
> --- a/drivers/net/mlx5/mlx5_flow.c
> +++ b/drivers/net/mlx5/mlx5_flow.c
> @@ -3561,15 +3561,17 @@ mlx5_fdir_filter_delete(struct rte_eth_dev *dev,
> /* The flow does not match. */
> continue;
> }
> - ret = rte_errno; /* Save rte_errno before cleanup. */
> if (flow)
> mlx5_flow_list_destroy(dev, &priv->flows, flow);
> exit:
> + if (ret)
> + ret = rte_errno; /* Save rte_errno before cleanup. */
> for (i = 0; i != hash_rxq_init_n; ++i) {
> if (parser.queue[i].ibv_attr)
> rte_free(parser.queue[i].ibv_attr);
> }
> - rte_errno = ret; /* Restore rte_errno. */
> + if (ret)
> + rte_errno = ret; /* Restore rte_errno. */
> return -rte_errno;
> }
>
> --
> 2.11.0
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH] net/mlx5: fix error number handling
On Mon, Jun 18, 2018 at 05:06:41PM +, Yongseok Koh wrote:
>
> > On Jun 7, 2018, at 12:39 AM, Nélio Laranjeiro
> > wrote:
> >
> > On Wed, Jun 06, 2018 at 11:39:27AM -0700, Yongseok Koh wrote:
> >> On Wed, Jun 06, 2018 at 08:55:01AM +0200, Nélio Laranjeiro wrote:
> >>> On Tue, Jun 05, 2018 at 09:36:32PM +, Yongseok Koh wrote:
> >>>>> On Jun 4, 2018, at 11:52 PM, Nélio Laranjeiro
> >>>>> wrote:
> >>>>>
> >>>>> On Mon, Jun 04, 2018 at 10:37:31AM -0700, Yongseok Koh wrote:
> >>>>>> rte_errno should be saved only if error has occurred because rte_errno
> >>>>>> could have garbage value.
> >>>>>>
> >>>>>> Fixes: a6d83b6a9209 ("net/mlx5: standardize on negative errno values")
> >>>>>> Cc: sta...@dpdk.org
> >>>>>>
> >>>>>> Signed-off-by: Yongseok Koh
> >>>>>> ---
> >>>>>> drivers/net/mlx5/mlx5_flow.c | 3 ++-
> >>>>>> 1 file changed, 2 insertions(+), 1 deletion(-)
> >>>>>>
> >>>>>> diff --git a/drivers/net/mlx5/mlx5_flow.c
> >>>>>> b/drivers/net/mlx5/mlx5_flow.c
> >>>>>> index 994be05be..eaffe7495 100644
> >>>>>> --- a/drivers/net/mlx5/mlx5_flow.c
> >>>>>> +++ b/drivers/net/mlx5/mlx5_flow.c
> >>>>>> @@ -3561,7 +3561,8 @@ mlx5_fdir_filter_delete(struct rte_eth_dev *dev,
> >>>>>>/* The flow does not match. */
> >>>>>>continue;
> >>>>>>}
> >>>>>> - ret = rte_errno; /* Save rte_errno before cleanup. */
> >>>>>> + if (ret)
> >>>>>> + ret = rte_errno; /* Save rte_errno before cleanup. */
> >>>>>>if (flow)
> >>>>>>mlx5_flow_list_destroy(dev, &priv->flows, flow);
> >>>>>> exit:
> >>>>>> --
> >>>>>> 2.11.0
> >>>>>
> >>>>> This patch is not enough, the returned value being -rte_errno if no
> >>>>> error is detected by the function it cannot set rte_errno nor return it.
> >>>>
> >>>> We may need to refactor this kind of code (saving and restoring
> >>>> rte_errno). I
> >>>> still don't understand why we should preserve rte_errno like this.
> >>>>
> >>>> Even if this function returns success, there's no obligation to preserve
> >>>> rte_errno in the function. Once it is called, the ownership of rte_errno
> >>>> belongs
> >>>> to this function.
> >>>>
> >>>> I can't find how we define this per-lcore variable but, from
> >>>> the man page of errno,
> >>>>
> >>>> Theheader file defines the integer variable errno,
> >>>> which
> >>>> is set by system calls and some library functions in the event of
> >>>> an
> >>>> error to indicate what went wrong. Its value is significant only
> >>>> when
> >>>> the return value of the call indicated an error (i.e., -1 from most
> >>>> system calls; -1 or NULL from most library functions);
> >>>> a function that succeeds is allowed to change errno.
> >>>>
> >>>> So, I still think an API can change rte_errno even if it succeeds, no
> >>>> need to
> >>>> preserve it. If needed, the caller has to save it.
> >>>
> >>> Functions in this PMD are defined as is:
> >>>
> >>> * @return
> >>> * 0 on success, a negative errno value otherwise and rte_errno is set.
> >>>
> >>> Which means rte_errno is only modified in case of error.
> >>>
> >>> This fix does not respect the documentation of the function or any other
> >>> function of the PMD which can return errors.
> >>
> >> That's logically a wrong interpretation. According to the description, if
> >> returning error, rte_errno is set but the opposite isn't always true. Even
> >> if
> >> rte_errno is set, it doesn't mean there's an error. So the description
> >> coincides
> >> with that of errno. If you want to enforce pre
Re: [dpdk-dev] [PATCH v2 1/2] app/testpmd: add VXLAN encap/decap support
On Mon, Jun 18, 2018 at 02:02:10PM -0700, Stephen Hemminger wrote:
> On Mon, 18 Jun 2018 10:52:54 +0200
> Nelio Laranjeiro wrote:
>
> >
> > +struct vxlan_encap_conf vxlan_encap_conf = {
> > + .select_ipv4 = 1,
> > + .vni = "\x00\x00\x00",
> > + .udp_src = RTE_BE16(1),
>
> Overall looks good. One enhancement I would suggest is to implement generating
> the UDP source port based on a hash of fields from inner packet (as suggested
> in RFC 7348). This would be enabled by default (use udp source port of 0
> as a flag to enable it).
I'll make the modification for the v4,
Thanks,
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH v3 0/2] app/testpmd implement VXLAN/NVGRE Encap/Decap
On Mon, Jun 18, 2018 at 04:28:05PM +, Iremonger, Bernard wrote:
> Hi Nelio,
>
> > -Original Message-
> > From: Nelio Laranjeiro [mailto:nelio.laranje...@6wind.com]
> > Sent: Monday, June 18, 2018 3:37 PM
> > To: dev@dpdk.org; Adrien Mazarguil ; Lu,
> > Wenzhuo ; Wu, Jingjing ;
> > Iremonger, Bernard ; Awal, Mohammad Abdul
> >
> > Subject: [PATCH v3 0/2] app/testpmd implement VXLAN/NVGRE Encap/Decap
> >
> > This series adds an easy and maintainable configuration version support for
> > those two actions for 18.08 by using global variables in testpmd to store
> > the
> > necessary information for the tunnel encapsulation. Those variables are
> > used in
> > conjunction of RTE_FLOW_ACTION_{VXLAN,NVGRE}_ENCAP action to create
> > easily the action for flows.
> >
> > A common way to use it:
> >
> > set vxlan ipv4 4 4 4 127.0.0.1 128.0.0.1 11:11:11:11:11:11
> > 22:22:22:22:22:22
> > flow create 0 ingress pattern end actions vxlan_encap / queue index 0 / end
> >
> > set vxlan ipv6 4 4 4 ::1 :: 11:11:11:11:11:11 22:22:22:22:22:22 flow
> > create
> > 0 ingress pattern end actions vxlan_encap / queue index 0 / end
> >
> > set nvgre ipv4 4 127.0.0.1 128.0.0.1 11:11:11:11:11:11 22:22:22:22:22:22
> > flow
> > create 0 ingress pattern end actions nvgre_encap / queue index 0 / end
> >
> > set nvgre ipv6 4 ::1 :: 11:11:11:11:11:11 22:22:22:22:22:22 flow
> > create 0
> > ingress pattern end actions nvgre_encap / queue index 0 / end
> >
>
> It might be useful to add the above sample testpmd command lines to
> section 4.12 of the doc/guides/testpmd_app_ug/testpmd_funcs.rst file
>[...]
Agreed, I'll add it in the v4.
Thanks,
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH v3 1/2] app/testpmd: add VXLAN encap/decap support
On Tue, Jun 19, 2018 at 07:09:28AM +, Ori Kam wrote:
>
>
> > -Original Message-
> > From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Nelio Laranjeiro
> > Signed-off-by: Nelio Laranjeiro
> > ---
> > app/test-pmd/cmdline.c | 129 ++
> > app/test-pmd/cmdline_flow.c | 139
> > app/test-pmd/testpmd.c | 17 +++
> > app/test-pmd/testpmd.h | 17 +++
> > doc/guides/testpmd_app_ug/testpmd_funcs.rst | 13 ++
> > 5 files changed, 315 insertions(+)
> >
> > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> > index 27e2aa8c8..93573606f 100644
> > --- a/app/test-pmd/cmdline.c
> > +++ b/app/test-pmd/cmdline.c
> > +static void cmd_set_vxlan_parsed(void *parsed_result,
> > + __attribute__((unused)) struct cmdline *cl,
> > + __attribute__((unused)) void *data)
> > +{
> > + struct cmd_set_vxlan_result *res = parsed_result;
> > + uint32_t vni = rte_cpu_to_be_32(res->vni) >> 8;
>
> Is this also correct in case of big endian system?
> I think it will remove part of the vni.
>
> > +
> > + if (strcmp(res->vxlan, "vxlan") == 0)
> > + vxlan_encap_conf.select_vlan = 0;
> > + else if (strcmp(res->vxlan, "vxlan-with-vlan") == 0)
> > + vxlan_encap_conf.select_vlan = 1;
> > + if (strcmp(res->ip_version, "ipv4") == 0)
> > + vxlan_encap_conf.select_ipv4 = 1;
> > + else if (strcmp(res->ip_version, "ipv6") == 0)
> > + vxlan_encap_conf.select_ipv4 = 0;
> > + else
> > + return;
> > + memcpy(vxlan_encap_conf.vni, &vni, 3);
>
> I don't think this line is correct when running on big endian system.
Yes, this is wrong, it will be fixed in v4.
Thanks,
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH v2 0/2] app/testpmd implement VXLAN/NVGRE Encap/Decap
On Mon, Jun 18, 2018 at 03:40:53PM +0100, Ferruh Yigit wrote:
> On 6/18/2018 10:38 AM, Nélio Laranjeiro wrote:
> > On Mon, Jun 18, 2018 at 10:05:03AM +0100, Ferruh Yigit wrote:
> >> On 6/18/2018 9:52 AM, Nelio Laranjeiro wrote:
> >>> This series adds an easy and maintainable configuration version support
> >>> for
> >>> those two actions for 18.08 by using global variables in testpmd to store
> >>> the
> >>> necessary information for the tunnel encapsulation. Those variables are
> >>> used
> >>> in conjunction of RTE_FLOW_ACTION_{VXLAN,NVGRE}_ENCAP action to create
> >>> easily
> >>> the action for flows.
> >>>
> >>> A common way to use it:
> >>>
> >>> set vxlan ipv4 4 4 4 127.0.0.1 128.0.0.1 11:11:11:11:11:11
> >>> 22:22:22:22:22:22
> >>> flow create 0 ingress pattern end actions vxlan_encap / queue index 0 /
> >>> end
> >>>
> >>> set vxlan ipv6 4 4 4 ::1 :: 11:11:11:11:11:11 22:22:22:22:22:22
> >>> flow create 0 ingress pattern end actions vxlan_encap / queue index 0 /
> >>> end
> >>>
> >>> set nvgre ipv4 4 127.0.0.1 128.0.0.1 11:11:11:11:11:11 22:22:22:22:22:22
> >>> flow create 0 ingress pattern end actions nvgre_encap / queue index 0 /
> >>> end
> >>>
> >>> set nvgre ipv6 4 ::1 :: 11:11:11:11:11:11 22:22:22:22:22:22
> >>> flow create 0 ingress pattern end actions nvgre_encap / queue index 0 /
> >>> end
> >>>
> >>> This also replace the proposal done by Mohammad Abdul Awal [1] which
> >>> handles
> >>> in a more complex way for the same work.
> >>
> >> Hi Nelio,
> >>
> >> Is this set on top of mentioned set?
> >
> > Hi Ferruh,
> >
> > No it is another implementation of Declan's API. It can be directly
> > applied on top of the current DPDK code without any other patch.
>
> I mean "based on" more than "on top of". So if this code is based on
> referenced
> patchset, I believe it should keep original sign-off.
>
> If this code is completely new implementation that replaces referenced
> patchset,
> I believe it would be nice to comment on the original patch or communicate
> about
> it instead of just sending another set to replace original one.
Hi Ferruh,
I agree with your point of view but my intention was to show how hard
for an application it will be to implement such actions (as mentioned by
Adrien [1][2]) whereas Mohammad has made the implementation for the
testpmd command line interactive mode. That is also the reason why I've
copied Mohammad at the first place in my series.
Note that such implementation request has been made by Thomas [3] even
with it, it has entered DPDK without it.
I did not comment on his series because I don't have any reason to
block it, if his series enters, mine just won't I also agree with that,
there is no need to have both implementation in DPDK, but it worse to
show how an application may have to deal with such actions.
> >> If so shouldn't the set has the Awal's sign-off too?
> >> Are you replacing someone else patch with dropping his sign-off?
> >>
> >>> Note this API has already a modification planned for 18.11 [2] thus those
> >>> series should have a limited life for a single release.
> >>>
> >>> [1] https://dpdk.org/ml/archives/dev/2018-May/101403.html
> >>> [2] https://dpdk.org/ml/archives/dev/2018-June/103485.html
> >>>
> >>>
> >>> Changes in v2:
> >>>
> >>> - add default IPv6 values for NVGRE encapsulation.
> >>> - replace VXLAN to NVGRE in comments concerning NVGRE layer.
> >>>
> >>> Nelio Laranjeiro (2):
> >>> app/testpmd: add VXLAN encap/decap support
> >>> app/testpmd: add NVGRE encap/decap support
> >>>
> >>> app/test-pmd/cmdline.c | 169 +
> >>> app/test-pmd/cmdline_flow.c | 248
> >>> app/test-pmd/testpmd.c | 28 +++
> >>> app/test-pmd/testpmd.h | 28 +++
> >>> doc/guides/testpmd_app_ug/testpmd_funcs.rst | 25 ++
> >>> 5 files changed, 498 insertions(+)
> >
>
Regards,
[1] https://mails.dpdk.org/archives/dev/2018-April/095945.html
[2] https://mails.dpdk.org/archives/dev/2018-April/098124.html
[3] https://mails.dpdk.org/archives/dev/2018-April/099799.html
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH v2 0/2] app/testpmd implement VXLAN/NVGRE Encap/Decap
On Mon, Jun 18, 2018 at 10:05:03AM +0100, Ferruh Yigit wrote:
> On 6/18/2018 9:52 AM, Nelio Laranjeiro wrote:
> > This series adds an easy and maintainable configuration version support for
> > those two actions for 18.08 by using global variables in testpmd to store
> > the
> > necessary information for the tunnel encapsulation. Those variables are
> > used
> > in conjunction of RTE_FLOW_ACTION_{VXLAN,NVGRE}_ENCAP action to create
> > easily
> > the action for flows.
> >
> > A common way to use it:
> >
> > set vxlan ipv4 4 4 4 127.0.0.1 128.0.0.1 11:11:11:11:11:11
> > 22:22:22:22:22:22
> > flow create 0 ingress pattern end actions vxlan_encap / queue index 0 / end
> >
> > set vxlan ipv6 4 4 4 ::1 :: 11:11:11:11:11:11 22:22:22:22:22:22
> > flow create 0 ingress pattern end actions vxlan_encap / queue index 0 / end
> >
> > set nvgre ipv4 4 127.0.0.1 128.0.0.1 11:11:11:11:11:11 22:22:22:22:22:22
> > flow create 0 ingress pattern end actions nvgre_encap / queue index 0 / end
> >
> > set nvgre ipv6 4 ::1 :: 11:11:11:11:11:11 22:22:22:22:22:22
> > flow create 0 ingress pattern end actions nvgre_encap / queue index 0 / end
> >
> > This also replace the proposal done by Mohammad Abdul Awal [1] which handles
> > in a more complex way for the same work.
>
> Hi Nelio,
>
> Is this set on top of mentioned set?
Hi Ferruh,
No it is another implementation of Declan's API. It can be directly
applied on top of the current DPDK code without any other patch.
> If so shouldn't the set has the Awal's sign-off too?
> Are you replacing someone else patch with dropping his sign-off?
>
> > Note this API has already a modification planned for 18.11 [2] thus those
> > series should have a limited life for a single release.
> >
> > [1] https://dpdk.org/ml/archives/dev/2018-May/101403.html
> > [2] https://dpdk.org/ml/archives/dev/2018-June/103485.html
> >
> >
> > Changes in v2:
> >
> > - add default IPv6 values for NVGRE encapsulation.
> > - replace VXLAN to NVGRE in comments concerning NVGRE layer.
> >
> > Nelio Laranjeiro (2):
> > app/testpmd: add VXLAN encap/decap support
> > app/testpmd: add NVGRE encap/decap support
> >
> > app/test-pmd/cmdline.c | 169 +
> > app/test-pmd/cmdline_flow.c | 248
> > app/test-pmd/testpmd.c | 28 +++
> > app/test-pmd/testpmd.h | 28 +++
> > doc/guides/testpmd_app_ug/testpmd_funcs.rst | 25 ++
> > 5 files changed, 498 insertions(+)
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH 2/2] app/testpmd: add NVGRE encap/decap support
Hi Bernard,
On Fri, Jun 15, 2018 at 09:32:02AM +, Iremonger, Bernard wrote:
> Hi Nelio,
>
>[...]
> > @@ -14927,6 +14930,81 @@ cmdline_parse_inst_t cmd_set_vxlan = {
> > },
> > };
> >
> > +/** Set VXLAN encapsulation details */
>
> VXLAN should be NVGRE.
>[...]
Right,
> > b/app/test-pmd/testpmd.c index 1c68c9d30..f54205949 100644
> > --- a/app/test-pmd/testpmd.c
> > +++ b/app/test-pmd/testpmd.c
> > @@ -408,6 +408,15 @@ struct vxlan_encap_conf vxlan_encap_conf = {
> > .eth_dst = "\xff\xff\xff\xff\xff\xff", };
> >
> > +struct nvgre_encap_conf nvgre_encap_conf = {
> > + .select_ipv4 = 1,
> > + .tni = "\x00\x00\x00",
> > + .ipv4_src = IPv4(127, 0, 0, 1),
> > + .ipv4_dst = IPv4(255, 255, 255, 255),
>
> Should there be .ipv6_src and .ipv6_dst here ?
>[...]
Yes indeed initialisation of IPv6 is missing.
> > +- ``nvgre_decap``: Performs a decapsulation action by stripping all
> > +headers of
> > + the VXLAN tunnel network overlay from the matched flow.
>
> VXLAN should be NVGRE.
>
>[...]
Here also,
I am will update it in a V2.
Thanks for you review,
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH] mlx5: fix log initialization
On Wed, Jun 13, 2018 at 11:46:26AM -0700, Stephen Hemminger wrote:
> The mlx5 driver had two init functions, but this could
> cause log initialization to be done after the
> other initialization. Also, the name of the function does
> not match convention (cut/paste error?).
Indeed an error in the copy/paste, sorry,
> Fix by initializing log type first at start of the pmd_init.
> This also gets rid of having two constructor functions.
>
> Fixes: a170a30d22a8 ("net/mlx5: use dynamic logging")
> Signed-off-by: Stephen Hemminger
Acked-by: Nelio Laranjeiro
> ---
> drivers/net/mlx5/mlx5.c | 13 +
> 1 file changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
> index c933e274fe32..6ec4f3178b6b 100644
> --- a/drivers/net/mlx5/mlx5.c
> +++ b/drivers/net/mlx5/mlx5.c
> @@ -1439,6 +1439,11 @@ RTE_INIT(rte_mlx5_pmd_init);
> static void
> rte_mlx5_pmd_init(void)
> {
> + /* Initialize driver log type. */
> + mlx5_logtype = rte_log_register("pmd.net.mlx5");
> + if (mlx5_logtype >= 0)
> + rte_log_set_level(mlx5_logtype, RTE_LOG_NOTICE);
> +
> /* Build the static tables for Verbs conversion. */
> mlx5_set_ptype_table();
> mlx5_set_cksum_table();
> @@ -1480,11 +1485,3 @@ rte_mlx5_pmd_init(void)
> RTE_PMD_EXPORT_NAME(net_mlx5, __COUNTER__);
> RTE_PMD_REGISTER_PCI_TABLE(net_mlx5, mlx5_pci_id_map);
> RTE_PMD_REGISTER_KMOD_DEP(net_mlx5, "* ib_uverbs & mlx5_core & mlx5_ib");
> -
> -/** Initialize driver log type. */
> -RTE_INIT(vdev_netvsc_init_log)
> -{
> - mlx5_logtype = rte_log_register("pmd.net.mlx5");
> - if (mlx5_logtype >= 0)
> - rte_log_set_level(mlx5_logtype, RTE_LOG_NOTICE);
> -}
> --
> 2.17.1
Thanks,
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [RFC] ethdev: support tunnel encapsulation action
On Mon, Jun 11, 2018 at 07:27:22AM +, Ori Kam wrote:
> Hi
>
> No you shouldn't understand this.
> I still think that the [1] proposal is the correct
> approach, but due to a very short time frame for this
> release I suggest this as intermediate solution.
>
> I want to get comments and open discussion regarding
> the proposal and in worst case add it to next release.
Do you mean this API is proposed for 18.11 and the new one is for 18.08
[1] ?
Please clarify your intention. It is clear both will not make part of
the same release.
> Ori
>
> > -Original Message-
> > From: Nélio Laranjeiro [mailto:nelio.laranje...@6wind.com]
> > Sent: Monday, June 11, 2018 9:53 AM
> > To: Ori Kam
> > Cc: ferruh.yi...@intel.com; declan.dohe...@intel.com; dev@dpdk.org;
> > Adrien Mazarguil
> > Subject: Re: [RFC] ethdev: support tunnel encapsulation action
> >
> > Hi Ori,
> >
> > Should we understand this proposal is nacked by [1] you have also
> > proposed?
> >
> > If yes, answer to this one with a self-nack to make it clear.
> >
> > Thanks,
> >
> > On Tue, Jun 05, 2018 at 06:48:28PM +0300, Ori Kam wrote:
> > > This RFC contain proposal to add generic support for tunnel
> > > encapsulation/decapsulation.
> > >
> > > Due to the fact that there are many possible tunnel types
> > > and each tunnel type has number of header variations, there
> > > is a need for some generic command.
> > >
> > > example for tunnel headers in case of MPLSoGRE:
> > > ETH / VLAN / IPV4 / GRE / MPLS / ETH / IP / L4-L7
> > > ETH / VLAN / IPV6 / GRE / MPLS / ETH / IP / L4-L7
> > > ETH / IPV4 / GRE / MPLS / ETH / IP / L4-L7
> > > ETH / IPV6 / GRE / MPLS / ETH / IP / L4-L7
> > > ETH / VLAN / IPV4 / GRE / MPLS / IP / L4-L7
> > > ETH / VLAN / IPV6 / GRE / MPLS / IP / L4-L7
> > > ETH / IPV4 / GRE / MPLS / IP / L4-L7
> > > ETH / IPV6 / GRE / MPLS / IP / L4-L7
> > >
> > > As can be seen from the examples some of the encapsulation
> > > is done by overwriting the inner L2 packet spec.
> > >
> > > To support all of those configuration it is much
> > > easer if we create 2 encap functions one that
> > > is used to encap L2 packet and one that is used
> > > to encap L3 packet by removing the L2 and applying
> > > the encapsulation header.
> > >
> > > The use of void * buffer will enable the
> > > insertion of any valid encapsulation header.
> > > the use of such a buffer will also simplify the processing
> > > needed to validate and apply vs the use of rte_flow_items.
> > > The use of a buffer will also will be easer
> > > for some applications (for example vrouter)
> > >
> > > For decap we will also have 2 actions
> > > one for decaping a packet with inner L2
> > > and one for decaping a packet with inner L3.
> > > when decaping L3 packet the user should
> > > supplay the L2 data which should be added to the inner packet.
> > >
> > > Signed-off-by: Ori Kam
> > > ---
> > > doc/guides/prog_guide/rte_flow.rst | 141 ++
> > -
> > > lib/librte_ethdev/rte_flow.h | 165
> > > +---
> > ---
> > > 2 files changed, 161 insertions(+), 145 deletions(-)
> > >
> > > diff --git a/doc/guides/prog_guide/rte_flow.rst
> > b/doc/guides/prog_guide/rte_flow.rst
> > > index b305a72..7417833 100644
> > > --- a/doc/guides/prog_guide/rte_flow.rst
> > > +++ b/doc/guides/prog_guide/rte_flow.rst
> > > @@ -1969,112 +1969,95 @@ Implements ``OFPAT_PUSH_MPLS`` ("push a
> > new MPLS tag") as defined by the
> > > | ``ethertype`` | EtherType |
> > > +---+---+
> > >
> > > -Action: ``VXLAN_ENCAP``
> > > -^^^
> > > -
> > > -Performs a VXLAN encapsulation action by encapsulating the matched
> > flow in the
> > > -VXLAN tunnel as defined in the``rte_flow_action_vxlan_encap`` flow
> > items
> > > -definition.
> > > +Action: ``TUNNEL_ENCAP``
> > > +
> > >
> > > -This action modifies the payload of matched flows. The flow definition
> > specified
> > > -in the ``rte_flow_action_tunnel_encap`` action structure must define a
> > valid
> > > -VLXAN
Re: [dpdk-dev] [RFC] ethdev: support tunnel encapsulation action
ignored.
> + * The tunnel definition is provided through the use of buffer that
> + * holds the encapsulating header.
> + * Provided header must be a valid outer tunnel header.
> + */
> +struct rte_flow_action_tunnel_encap {
> + enum rte_flow_tunnel_type type; /**< The tunnel type. */
> + void *buf; /**< The header to be used. */
> + uint32_t len; /**< The buf len. */
> +};
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this structure may change without prior notice
> *
> - * Valid flow definition for RTE_FLOW_ACTION_TYPE_VXLAN_ENCAP include:
> + * RTE_FLOW_ACTION_TYPE_TUNNEL_DECAP
> *
> - * - ETH / IPV4 / UDP / VXLAN / END
> - * - ETH / IPV6 / UDP / VXLAN / END
> - * - ETH / VLAN / IPV4 / UDP / VXLAN / END
> + * Tunnel end-point dencapsulation data definition.
> *
> + * The tunnel type must match the flow rule spec.
> */
> -struct rte_flow_action_vxlan_encap {
> - /**
> - * Encapsulating vxlan tunnel definition
> - * (terminated by the END pattern item).
> - */
> - struct rte_flow_item *definition;
> +struct rte_flow_action_tunnel_decap {
> + enum rte_flow_tunnel_type type; /**< The tunnel type. */
> };
>
> /**
> * @warning
> * @b EXPERIMENTAL: this structure may change without prior notice
> *
> - * RTE_FLOW_ACTION_TYPE_NVGRE_ENCAP
> - *
> - * NVGRE tunnel end-point encapsulation data definition
> - *
> - * The tunnel definition is provided through the flow item pattern the
> - * provided pattern must conform with RFC7637. The flow definition must be
> - * provided in order from the RTE_FLOW_ITEM_TYPE_ETH definition up the end
> item
> - * which is specified by RTE_FLOW_ITEM_TYPE_END.
> + * RTE_FLOW_ACTION_TYPE_TUNNEL_ENCAP_L3
> *
> - * The mask field allows user to specify which fields in the flow item
> - * definitions can be ignored and which have valid data and can be used
> - * verbatim.
> + * Tunnel end-point encapsulation over the inner L2 data definition.
> *
> - * Note: the last field is not used in the definition of a tunnel and can be
> - * ignored.
> + * The tunnel definition is provided through the use of buffer that
> + * holds the encapsulating header.
> + * Provided header must be a valid outer tunnel header.
> + */
> +struct rte_flow_action_tunnel_encap_l3 {
> + enum rte_flow_tunnel_type type; /**< The tunnel type. */
> + void *buf; /**< The header to be used. */
> + uint32_t len; /**< The buf len. */
> +};
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this structure may change without prior notice
> *
> - * Valid flow definition for RTE_FLOW_ACTION_TYPE_NVGRE_ENCAP include:
> + * RTE_FLOW_ACTION_TYPE_TUNNEL_DECAP_L3
> *
> - * - ETH / IPV4 / NVGRE / END
> - * - ETH / VLAN / IPV6 / NVGRE / END
> + * Tunnel end-point dencapsulation data definition.
> + * after the decapsulation, the L2 of the resulted packet
> + * is replaced with the supplied buffer.
> *
> + * The tunnel type must match the flow rule spec.
> */
> -struct rte_flow_action_nvgre_encap {
> - /**
> - * Encapsulating vxlan tunnel definition
> - * (terminated by the END pattern item).
> - */
> - struct rte_flow_item *definition;
> +struct rte_flow_action_tunnel_decap_l3 {
> + enum rte_flow_tunnel_type type; /**< The tunnel type. */
> + void *buf; /**< The L2 header to be used.*/
> + uint32_t len; /**< The len of the buf. */
> };
>
> /*
> --
> 1.7.1
>
[1] http://dpdk.org/dev/patchwork/patch/40965/
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [RFC v1] net/mlx5: support multiple flow priorities
{
> @@ -3739,7 +3751,7 @@ unsigned int
> mlx5_get_max_verbs_prio(struct rte_eth_dev *dev)
> {
> struct priv *priv = dev->data->dev_private;
> - unsigned int verb_priorities = MLX5_VERBS_FLOW_PRIO_8;
> + unsigned int verb_priorities = MLX5_VERBS_FLOW_MIN_PRIOS;
> struct {
> struct ibv_flow_attr attr;
> struct ibv_flow_spec_eth eth;
> @@ -3773,8 +3785,7 @@ mlx5_get_max_verbs_prio(struct rte_eth_dev *dev)
> break;
> }
> } while (1);
> - DRV_LOG(DEBUG, "port %u Verbs flow priorities: %d,"
> - " user flow priorities: %d",
> - dev->data->port_id, verb_priorities, MLX5_CTRL_FLOW_PRIORITY);
> + DRV_LOG(DEBUG, "port %u Verbs flow priorities: %d",
> + dev->data->port_id, verb_priorities);
> return verb_priorities;
> }
> --
> 2.13.3
[1] http://dpdk.org/ml/archives/dev/2018-May/103043.html
Regards,
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH] net/mlx5: fix error number handling
On Wed, Jun 06, 2018 at 11:39:27AM -0700, Yongseok Koh wrote:
> On Wed, Jun 06, 2018 at 08:55:01AM +0200, Nélio Laranjeiro wrote:
> > On Tue, Jun 05, 2018 at 09:36:32PM +, Yongseok Koh wrote:
> > > > On Jun 4, 2018, at 11:52 PM, Nélio Laranjeiro
> > > > wrote:
> > > >
> > > > On Mon, Jun 04, 2018 at 10:37:31AM -0700, Yongseok Koh wrote:
> > > >> rte_errno should be saved only if error has occurred because rte_errno
> > > >> could have garbage value.
> > > >>
> > > >> Fixes: a6d83b6a9209 ("net/mlx5: standardize on negative errno values")
> > > >> Cc: sta...@dpdk.org
> > > >>
> > > >> Signed-off-by: Yongseok Koh
> > > >> ---
> > > >> drivers/net/mlx5/mlx5_flow.c | 3 ++-
> > > >> 1 file changed, 2 insertions(+), 1 deletion(-)
> > > >>
> > > >> diff --git a/drivers/net/mlx5/mlx5_flow.c
> > > >> b/drivers/net/mlx5/mlx5_flow.c
> > > >> index 994be05be..eaffe7495 100644
> > > >> --- a/drivers/net/mlx5/mlx5_flow.c
> > > >> +++ b/drivers/net/mlx5/mlx5_flow.c
> > > >> @@ -3561,7 +3561,8 @@ mlx5_fdir_filter_delete(struct rte_eth_dev *dev,
> > > >>/* The flow does not match. */
> > > >>continue;
> > > >>}
> > > >> - ret = rte_errno; /* Save rte_errno before cleanup. */
> > > >> + if (ret)
> > > >> + ret = rte_errno; /* Save rte_errno before cleanup. */
> > > >>if (flow)
> > > >>mlx5_flow_list_destroy(dev, &priv->flows, flow);
> > > >> exit:
> > > >> --
> > > >> 2.11.0
> > > >
> > > > This patch is not enough, the returned value being -rte_errno if no
> > > > error is detected by the function it cannot set rte_errno nor return it.
> > >
> > > We may need to refactor this kind of code (saving and restoring
> > > rte_errno). I
> > > still don't understand why we should preserve rte_errno like this.
> > >
> > > Even if this function returns success, there's no obligation to preserve
> > > rte_errno in the function. Once it is called, the ownership of rte_errno
> > > belongs
> > > to this function.
> > >
> > > I can't find how we define this per-lcore variable but, from
> > > the man page of errno,
> > >
> > >Theheader file defines the integer variable errno,
> > > which
> > >is set by system calls and some library functions in the event of
> > > an
> > >error to indicate what went wrong. Its value is significant only
> > > when
> > >the return value of the call indicated an error (i.e., -1 from most
> > >system calls; -1 or NULL from most library functions);
> > >a function that succeeds is allowed to change errno.
> > >
> > > So, I still think an API can change rte_errno even if it succeeds, no
> > > need to
> > > preserve it. If needed, the caller has to save it.
> >
> > Functions in this PMD are defined as is:
> >
> > * @return
> > * 0 on success, a negative errno value otherwise and rte_errno is set.
> >
> > Which means rte_errno is only modified in case of error.
> >
> > This fix does not respect the documentation of the function or any other
> > function of the PMD which can return errors.
>
> That's logically a wrong interpretation. According to the description, if
> returning error, rte_errno is set but the opposite isn't always true. Even if
> rte_errno is set, it doesn't mean there's an error. So the description
> coincides
> with that of errno. If you want to enforce preserving rte_errno in case of
> success, you should amend the documentation.
>
> > rte_errno is only set if an error is encountered and contains only the error
> > code of the first error sub-sequent ones are considered consequences of the
> > first one and thus not preserved.
> >
> > Not preserving the rte_errno in roll backs is equivalent to not setting
> > it at all as a function called by the rollback may also set it, example:
> >
> > {
> > void * a;
> >
> > foo_do();
> > a = malloc(10);
> > if (!a) {
> > rte_errno = ENOMEM;
> > foo_undo();
>
> This example is w
Re: [dpdk-dev] [PATCH] net/mlx5: fix error number handling
On Tue, Jun 05, 2018 at 09:36:32PM +, Yongseok Koh wrote:
> > On Jun 4, 2018, at 11:52 PM, Nélio Laranjeiro
> > wrote:
> >
> > On Mon, Jun 04, 2018 at 10:37:31AM -0700, Yongseok Koh wrote:
> >> rte_errno should be saved only if error has occurred because rte_errno
> >> could have garbage value.
> >>
> >> Fixes: a6d83b6a9209 ("net/mlx5: standardize on negative errno values")
> >> Cc: sta...@dpdk.org
> >>
> >> Signed-off-by: Yongseok Koh
> >> ---
> >> drivers/net/mlx5/mlx5_flow.c | 3 ++-
> >> 1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
> >> index 994be05be..eaffe7495 100644
> >> --- a/drivers/net/mlx5/mlx5_flow.c
> >> +++ b/drivers/net/mlx5/mlx5_flow.c
> >> @@ -3561,7 +3561,8 @@ mlx5_fdir_filter_delete(struct rte_eth_dev *dev,
> >>/* The flow does not match. */
> >>continue;
> >>}
> >> - ret = rte_errno; /* Save rte_errno before cleanup. */
> >> + if (ret)
> >> + ret = rte_errno; /* Save rte_errno before cleanup. */
> >>if (flow)
> >>mlx5_flow_list_destroy(dev, &priv->flows, flow);
> >> exit:
> >> --
> >> 2.11.0
> >
> > This patch is not enough, the returned value being -rte_errno if no
> > error is detected by the function it cannot set rte_errno nor return it.
>
> We may need to refactor this kind of code (saving and restoring rte_errno). I
> still don't understand why we should preserve rte_errno like this.
>
> Even if this function returns success, there's no obligation to preserve
> rte_errno in the function. Once it is called, the ownership of rte_errno
> belongs
> to this function.
>
> I can't find how we define this per-lcore variable but, from
> the man page of errno,
>
>Theheader file defines the integer variable errno,
> which
>is set by system calls and some library functions in the event of an
>error to indicate what went wrong. Its value is significant only when
>the return value of the call indicated an error (i.e., -1 from most
>system calls; -1 or NULL from most library functions);
>a function that succeeds is allowed to change errno.
>
> So, I still think an API can change rte_errno even if it succeeds, no need to
> preserve it. If needed, the caller has to save it.
Functions in this PMD are defined as is:
* @return
* 0 on success, a negative errno value otherwise and rte_errno is set.
Which means rte_errno is only modified in case of error.
This fix does not respect the documentation of the function or any other
function of the PMD which can return errors. rte_errno is only set if
an error is encountered and contains only the error code of the first
error sub-sequent ones are considered consequences of the first one and
thus not preserved.
Not preserving the rte_errno in roll backs is equivalent to not setting
it at all as a function called by the rollback may also set it, example:
{
void * a;
foo_do();
a = malloc(10);
if (!a) {
rte_errno = ENOMEM;
foo_undo();
return -rte_errno;
}
}
If foo_undo() also encounter an error it will modify the rte_errno which
may have a value different from ENOMEM, for the callee won't be informed
the error is due to a memory issue and thus cannot make counter parts.
In such situation the rte_errno must be preserved to keep the ENOMEM
error code.
This is also the main reason almost all system function only update
errno when no error is encountered.
Regards,
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH] net/mlx5: fix error number handling
On Mon, Jun 04, 2018 at 10:37:31AM -0700, Yongseok Koh wrote:
> rte_errno should be saved only if error has occurred because rte_errno
> could have garbage value.
>
> Fixes: a6d83b6a9209 ("net/mlx5: standardize on negative errno values")
> Cc: sta...@dpdk.org
>
> Signed-off-by: Yongseok Koh
> ---
> drivers/net/mlx5/mlx5_flow.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
> index 994be05be..eaffe7495 100644
> --- a/drivers/net/mlx5/mlx5_flow.c
> +++ b/drivers/net/mlx5/mlx5_flow.c
> @@ -3561,7 +3561,8 @@ mlx5_fdir_filter_delete(struct rte_eth_dev *dev,
> /* The flow does not match. */
> continue;
> }
> - ret = rte_errno; /* Save rte_errno before cleanup. */
> + if (ret)
> + ret = rte_errno; /* Save rte_errno before cleanup. */
> if (flow)
> mlx5_flow_list_destroy(dev, &priv->flows, flow);
> exit:
> --
> 2.11.0
This patch is not enough, the returned value being -rte_errno if no
error is detected by the function it cannot set rte_errno nor return it.
Thanks,
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [RFC] net/mlx5: add support 32bit build
On Sun, Jun 03, 2018 at 06:29:13PM +0300, Moti Haimovsky wrote: > This RFC and patch aim to add support for building mlx5 PMD on 32bit > archs such as i686. > The attached patch was not tested on 32bit environments and is given > here for explanatory purposes. > > Prerequisites: > * RDMA-core with 32bit support libraries. > * compile and build tools for 32bit systems according to DPDK documentation. > > Assumptions: > * Kernel and drivers are 64bit. > * Only user programs and support libraries are 32Bit. > > Changes proposed in the PMD for adding 32bit support: > The attached patch provides the changes needed in the PMD in order to support > 32bit builds with the major changes being: > * Modifying the UAR access routine to support non-atomic 64bit writes > according to the mlx5 hardware reference manual. > * Handling 64bit address fields that are written to the device WQEs. > * Modify UAR address and size mapping to fit into the 32bit address space. > > Concerns: > * The major concern of this modification is the UAR mapping. > Current PMD assumes 64bit address and therefore feels free to map 4G size > UAR to virtual address just above the huge-page addresses which of course > cannot work for 32bit address space. > As a workaround I reduced the UAR size to half the original size but > this of course is not the solution. > * Will vectorized support work on 32bit builds ? > > I am interested in your inputs regarding the concerns listed above, > especially the UAR mapping issue mentioned there. >[...] Hi Moti, My biggest concern and why such support has been removed [1] is because Mellanox OFED is not supporting 32bits compilation. As Mellanox OFED is still supported, we need such guarantee. On Mellanox website there is no way to download such kind of ISO which means such support becomes only valid for upstream RDMA-Core library with almost newer kernels. If you can find a way to detect the PMD is compiled in front of Mellanox OFED to forbid such support, it would be great, otherwise I am afraid such support cannot be added back without creating a lot of issues for people wanting it on 32bits processors with Mellanox OFED. Thanks, [1] https://dpdk.org/browse/dpdk/commit/?id=ebbb81eb27daca0a89ee8f228fcf141d9eb6ef1c -- Nélio Laranjeiro 6WIND
Re: [dpdk-dev] Compilation of MLX5 driver
On Thu, May 31, 2018 at 09:14:03AM +, Nitin Katiyar wrote: > Yes,I installed it using --dpdk --upstream-libs. What is the way > forward now? In v17.05 MLX5 PMD is still relying on libibverbs and libmlx5, the way Those options you used are necessary to select in their package the installation of libverbs,libmlx5 or rdma-core. Doing this you have selected rdma-core which is not supported in v17.05 DPDK version. You need to install Mellanox OFED without those two options to select libibverbs, libmlx5 to make it work. Regards, > Regards, > Nitin > > -Original Message----- > From: Nélio Laranjeiro [mailto:nelio.laranje...@6wind.com] > Sent: Thursday, May 31, 2018 1:36 PM > To: Nitin Katiyar > Cc: Shahaf Shuler ; dev@dpdk.org > Subject: Re: [dpdk-dev] Compilation of MLX5 driver > > On Thu, May 31, 2018 at 07:01:17AM +, Nitin Katiyar wrote: > > Hi, > > It has following files: > > > > arch.h ib.h kern-abi.h mlx4dv.h mlx5dv.h opcode.h sa.h > > sa-kern-abi.h verbs.h > > > > I tried with both MLNX_OFED_LINUX-4.2-1.0.0.0 and > > MLNX_OFED_LINUX-4.2-1.2.0.0-ubuntu14.04-x86_64 > > Did you installed Mellanox OFED with the --dpdk --upstream-libs arguments for > the installation script? > > If it is the case, you should not add them for this version, those options > are for DPDK v17.11 and higher. > > Regards, > > > Regards, > > Nitin > > > > -Original Message- > > From: Shahaf Shuler [mailto:shah...@mellanox.com] > > Sent: Thursday, May 31, 2018 10:51 AM > > To: Nitin Katiyar ; Nélio Laranjeiro > > > > Cc: dev@dpdk.org > > Subject: RE: [dpdk-dev] Compilation of MLX5 driver > > > > Wednesday, May 30, 2018 7:45 PM, Nitin Katiyar: > > > > > > Hi, > > > I was compiling 17.05.02. > > > Regards, > > > Nitin > > > > > > -Original Message- > > > From: Nélio Laranjeiro [mailto:nelio.laranje...@6wind.com] > > > Sent: Wednesday, May 30, 2018 6:42 PM > > > To: Nitin Katiyar > > > Cc: dev@dpdk.org > > > Subject: Re: [dpdk-dev] Compilation of MLX5 driver > > > > > > Hi, > > > > > > On Wed, May 30, 2018 at 11:54:31AM +, Nitin Katiyar wrote: > > > > Hi, > > > > I am trying to compile MLX5 PMD driver by setting > > > "CONFIG_RTE_LIBRTE_MLX5_PMD=y" and hitting following compilation > > > error. > > > > > > > > fatal error: infiniband/mlx5_hw.h: No such file or directory > > > > Can you list the files you have under /usr/include/infiniband ? > > > > > > > > > > I have installed MLNX_OFED _LINUX-4.2-1.2.0 on my Ubuntu 14.04 > > > > machine > > > but still hitting the same error. Am I missing some other package? > > > > > > Which version of DPDK are you using (it is important to help)? > > > > > > Regards, > > > > > > -- > > > Nélio Laranjeiro > > > 6WIND > > -- > Nélio Laranjeiro > 6WIND -- Nélio Laranjeiro 6WIND
Re: [dpdk-dev] Compilation of MLX5 driver
On Thu, May 31, 2018 at 07:01:17AM +, Nitin Katiyar wrote: > Hi, > It has following files: > > arch.h ib.h kern-abi.h mlx4dv.h mlx5dv.h opcode.h sa.h > sa-kern-abi.h verbs.h > > I tried with both MLNX_OFED_LINUX-4.2-1.0.0.0 and > MLNX_OFED_LINUX-4.2-1.2.0.0-ubuntu14.04-x86_64 Did you installed Mellanox OFED with the --dpdk --upstream-libs arguments for the installation script? If it is the case, you should not add them for this version, those options are for DPDK v17.11 and higher. Regards, > Regards, > Nitin > > -Original Message- > From: Shahaf Shuler [mailto:shah...@mellanox.com] > Sent: Thursday, May 31, 2018 10:51 AM > To: Nitin Katiyar ; Nélio Laranjeiro > > Cc: dev@dpdk.org > Subject: RE: [dpdk-dev] Compilation of MLX5 driver > > Wednesday, May 30, 2018 7:45 PM, Nitin Katiyar: > > > > Hi, > > I was compiling 17.05.02. > > Regards, > > Nitin > > > > -Original Message- > > From: Nélio Laranjeiro [mailto:nelio.laranje...@6wind.com] > > Sent: Wednesday, May 30, 2018 6:42 PM > > To: Nitin Katiyar > > Cc: dev@dpdk.org > > Subject: Re: [dpdk-dev] Compilation of MLX5 driver > > > > Hi, > > > > On Wed, May 30, 2018 at 11:54:31AM +, Nitin Katiyar wrote: > > > Hi, > > > I am trying to compile MLX5 PMD driver by setting > > "CONFIG_RTE_LIBRTE_MLX5_PMD=y" and hitting following compilation > > error. > > > > > > fatal error: infiniband/mlx5_hw.h: No such file or directory > > Can you list the files you have under /usr/include/infiniband ? > > > > > > > I have installed MLNX_OFED _LINUX-4.2-1.2.0 on my Ubuntu 14.04 > > > machine > > but still hitting the same error. Am I missing some other package? > > > > Which version of DPDK are you using (it is important to help)? > > > > Regards, > > > > -- > > Nélio Laranjeiro > > 6WIND -- Nélio Laranjeiro 6WIND
Re: [dpdk-dev] Compilation of MLX5 driver
Hi, On Wed, May 30, 2018 at 11:54:31AM +, Nitin Katiyar wrote: > Hi, > I am trying to compile MLX5 PMD driver by setting > "CONFIG_RTE_LIBRTE_MLX5_PMD=y" and hitting following compilation error. > > fatal error: infiniband/mlx5_hw.h: No such file or directory > > I have installed MLNX_OFED _LINUX-4.2-1.2.0 on my Ubuntu 14.04 machine but > still hitting the same error. Am I missing some other package? Which version of DPDK are you using (it is important to help)? Regards, -- Nélio Laranjeiro 6WIND
Re: [dpdk-dev] [DPDK 18.08 v1 00/12] net/mlx5: flow rework
Hi Keith, On Mon, May 28, 2018 at 01:32:34PM +, Wiles, Keith wrote: > The subject does not have [PATCH ] so it will be missed by patchwork, right? I see them on the patchwork [1], it seems it uses anything else to determine if it is a patch or not. > > On May 28, 2018, at 6:21 AM, Nelio Laranjeiro > > wrote: > > > > First version of for the flow engine rework of MLX5 to prepare the > > introduction for the ENCAP/DECAP and PUSH/POP actions done via TC > > flower/filter. > > > > This first series depends on [1] and is a work in progress, recent work > > added > > on Tunnel RSS are still absent as well as the tunnel support. Those will be > > added in further versions. > > > > Expected for the next 18.08 release: > > > > - same level of features, > > - TC flow support for port redirection, > > - TC filter support for ENCAP/DECAP and PUSH/POP. > > > > [1] https://dpdk.org/dev/patchwork/patch/40462/ > > > > Nelio Laranjeiro (12): > > net/mlx5: remove flow support > > net/mlx5: handle drop queues are regular queues > > net/mlx5: support flow Ethernet item among with drop action > > net/mlx5: add flow queue action > > net/mlx5: add flow stop/start > > net/mlx5: add flow VLAN item > > net/mlx5: add flow IPv4 item > > net/mlx5: add flow IPv6 item > > net/mlx5: add flow UDP item > > net/mlx5: add flow TCP item > > net/mlx5: add mark/flag flow action > > net/mlx5: add RSS flow action > > > > drivers/net/mlx5/mlx5.c |9 - > > drivers/net/mlx5/mlx5.h |3 +- > > drivers/net/mlx5/mlx5_flow.c | 3615 +++--- > > drivers/net/mlx5/mlx5_rxq.c | 221 +++ > > drivers/net/mlx5/mlx5_rxtx.h |6 + > > 5 files changed, 1388 insertions(+), 2466 deletions(-) > > > > -- > > 2.17.0 > > > > Regards, > Keith Thanks, [1] https://dpdk.org/dev/patchwork/project/dpdk/list/?submitter=243 -- Nélio Laranjeiro 6WIND
Re: [dpdk-dev] [PATCH] net/mlx5: fix invalid count in xstats
On Sun, May 20, 2018 at 10:13:58AM +0200, David Marchand wrote:
> With the commit af4f09f28294 ("net/mlx5: prefix all functions with mlx5"),
> mlx5_xstats_get() is not compliant any longer with the api.
> It always returns the caller max entries count while it should return how
> many entries it wrote/wanted to write.
>
> Fixes: af4f09f28294 ("net/mlx5: prefix all functions with mlx5")
>
> Signed-off-by: David Marchand
Acked-by: Nelio Laranjeiro
> ---
> drivers/net/mlx5/mlx5_stats.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/mlx5/mlx5_stats.c b/drivers/net/mlx5/mlx5_stats.c
> index 8e427e7..875dd10 100644
> --- a/drivers/net/mlx5/mlx5_stats.c
> +++ b/drivers/net/mlx5/mlx5_stats.c
> @@ -325,7 +325,7 @@ mlx5_xstats_get(struct rte_eth_dev *dev, struct
> rte_eth_xstat *stats,
> stats[i].value = (counters[i] - xstats_ctrl->base[i]);
> }
> }
> - return n;
> + return xstats_n;
> }
>
> /**
> --
> 2.7.4
Thanks,
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH] net/mlx5: fix default RSS level
On Tue, May 15, 2018 at 04:23:24PM +0300, Shahaf Shuler wrote:
> Using inner RSS by default for GRE leads to memory corruption as the
> extra flow items added for the inner RSS are not counted in the flow
> attributes buffer size.
>
> Fixing by enforcing the default RSS level to be outer. This much
> simplify the flow engine and more robust.
> Future optimization for out of the box RSS can be done on subsequent
> commits.
>
> Fixes: d4a405186b73 ("net/mlx5: support tunnel RSS level")
> Cc: xuemi...@mellanox.com
> Cc: ma...@mellanox.com
>
> Signed-off-by: Shahaf Shuler
Acked-by: Nelio Laranjeiro
> ---
> drivers/net/mlx5/mlx5_flow.c | 22 ++
> 1 file changed, 2 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
> index 0c0d6f99ad..994be05be6 100644
> --- a/drivers/net/mlx5/mlx5_flow.c
> +++ b/drivers/net/mlx5/mlx5_flow.c
> @@ -805,7 +805,7 @@ mlx5_flow_convert_actions(struct rte_eth_dev *dev,
> }
> parser->rss_conf = (struct rte_flow_action_rss){
> .func = RTE_ETH_HASH_FUNCTION_DEFAULT,
> - .level = rss->level,
> + .level = rss->level ? rss->level : 1,
> .types = rss->types,
> .key_len = rss_key_len,
> .queue_num = rss->queue_num,
> @@ -1166,9 +1166,6 @@ mlx5_flow_convert_rss(struct mlx5_flow_parse *parser)
> int outer = parser->tunnel && parser->rss_conf.level < 2;
> uint64_t rss = parser->rss_conf.types;
>
> - /* Default to outer RSS. */
> - if (!parser->rss_conf.level)
> - parser->rss_conf.level = 1;
> layer = outer ? parser->out_layer : parser->layer;
> if (layer == HASH_RXQ_TUNNEL)
> layer = HASH_RXQ_ETH;
> @@ -1801,9 +1798,6 @@ mlx5_flow_create_vxlan(const struct rte_flow_item *item,
> parser->tunnel = ptype_ext[PTYPE_IDX(RTE_PTYPE_TUNNEL_VXLAN)];
> parser->out_layer = parser->layer;
> parser->layer = HASH_RXQ_TUNNEL;
> - /* Default VXLAN to outer RSS. */
> - if (!parser->rss_conf.level)
> - parser->rss_conf.level = 1;
> if (spec) {
> if (!mask)
> mask = default_mask;
> @@ -1876,9 +1870,6 @@ mlx5_flow_create_vxlan_gpe(const struct rte_flow_item
> *item,
> parser->tunnel = ptype_ext[PTYPE_IDX(RTE_PTYPE_TUNNEL_VXLAN_GPE)];
> parser->out_layer = parser->layer;
> parser->layer = HASH_RXQ_TUNNEL;
> - /* Default VXLAN-GPE to outer RSS. */
> - if (!parser->rss_conf.level)
> - parser->rss_conf.level = 1;
> if (spec) {
> if (!mask)
> mask = default_mask;
> @@ -1956,9 +1947,6 @@ mlx5_flow_create_gre(const struct rte_flow_item *item,
> parser->tunnel = ptype_ext[PTYPE_IDX(RTE_PTYPE_TUNNEL_GRE)];
> parser->out_layer = parser->layer;
> parser->layer = HASH_RXQ_TUNNEL;
> - /* Default GRE to inner RSS. */
> - if (!parser->rss_conf.level)
> - parser->rss_conf.level = 2;
> #ifdef HAVE_IBV_DEVICE_MPLS_SUPPORT
> if (spec) {
> if (!mask)
> @@ -2052,12 +2040,6 @@ mlx5_flow_create_mpls(const struct rte_flow_item *item,
> /* parser->out_layer stays as in GRE out_layer. */
> }
> parser->layer = HASH_RXQ_TUNNEL;
> - /*
> - * For MPLS-in-GRE, RSS level should have been set.
> - * For MPLS-in-UDP, use outer RSS.
> - */
> - if (!parser->rss_conf.level)
> - parser->rss_conf.level = 1;
> if (spec) {
> if (!mask)
> mask = default_mask;
> @@ -2501,7 +2483,7 @@ mlx5_flow_list_create(struct rte_eth_dev *dev,
> flow->tunnel = parser.tunnel;
> flow->rss_conf = (struct rte_flow_action_rss){
> .func = RTE_ETH_HASH_FUNCTION_DEFAULT,
> - .level = 0,
> + .level = parser.rss_conf.level,
> .types = parser.rss_conf.types,
> .key_len = parser.rss_conf.key_len,
> .queue_num = parser.rss_conf.queue_num,
> --
> 2.12.0
>
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH v2] net/mlx5: support MPLS-in-GRE and MPLS-in-UDP
n-GRE and MPLS-in-UDP.
> + *
> + * @param item[in]
> + * Item specification.
> + * @param default_mask[in]
> + * Default bit-masks to use when item->mask is not provided.
> + * @param data[in, out]
> + * User structure.
> + *
> + * @return
> + * 0 on success, a negative errno value otherwise and rte_errno is set.
> + */
> +static int
> +mlx5_flow_create_mpls(const struct rte_flow_item *item,
> + const void *default_mask,
> + struct mlx5_flow_data *data)
> +{
> +#ifndef HAVE_IBV_DEVICE_MPLS_SUPPORT
> + (void)default_mask;
> + return rte_flow_error_set(data->error, ENOTSUP,
> + RTE_FLOW_ERROR_TYPE_ITEM,
> + item,
> + "MPLS is not supported by driver");
> +#else
> + const struct rte_flow_item_mpls *spec = item->spec;
> + const struct rte_flow_item_mpls *mask = item->mask;
> + struct mlx5_flow_parse *parser = data->parser;
> + unsigned int size = sizeof(struct ibv_flow_spec_mpls);
> + struct ibv_flow_spec_mpls mpls = {
> + .type = IBV_FLOW_SPEC_MPLS,
> + .size = size,
> + };
> +
> + parser->inner = IBV_FLOW_SPEC_INNER;
> + if (parser->layer == HASH_RXQ_UDPV4 ||
> + parser->layer == HASH_RXQ_UDPV6) {
> + parser->tunnel =
> + ptype_ext[PTYPE_IDX(RTE_PTYPE_TUNNEL_MPLS_IN_UDP)];
> + parser->out_layer = parser->layer;
> + } else {
> + parser->tunnel =
> + ptype_ext[PTYPE_IDX(RTE_PTYPE_TUNNEL_MPLS_IN_GRE)];
> + /* parser->out_layer stays as in GRE out_layer. */
> + }
> + parser->layer = HASH_RXQ_TUNNEL;
> + /*
> + * For MPLS-in-GRE, RSS level should have been set.
> + * For MPLS-in-UDP, use outer RSS.
> + */
> + if (!parser->rss_conf.level)
> + parser->rss_conf.level = 1;
> + if (spec) {
> + if (!mask)
> + mask = default_mask;
> + /*
> + * The verbs label field includes the entire MPLS header:
> + * bits 0:19 - label value field.
> + * bits 20:22 - traffic class field.
> + * bits 23 - bottom of stack bit.
> + * bits 24:31 - ttl field.
> + */
> + mpls.val.label = *(const uint32_t *)spec;
> + mpls.mask.label = *(const uint32_t *)mask;
> + /* Remove unwanted bits from values. */
> + mpls.val.label &= mpls.mask.label;
> + }
> + mlx5_flow_create_copy(parser, &mpls, size);
> + return 0;
> +#endif
> +}
> +
> +/**
> * Convert mark/flag action to Verbs specification.
> *
> * @param parser
> --
> 1.9.5
>
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH] net/mlx5: support MPLS-in-GRE and MPLS-in-UDP
Hi Matan,
I have a single comment, please see bellow,
On Mon, May 14, 2018 at 01:41:20PM +, Matan Azrad wrote:
> Add support for MPLS over GRE and MPLS over UDP tunnel types as
> described in the next RFCs:
> 1. https://tools.ietf.org/html/rfc4023
> 2. https://tools.ietf.org/html/rfc7510
> 3. https://tools.ietf.org/html/rfc4385
>
> Signed-off-by: Matan Azrad
> ---
> doc/guides/nics/mlx5.rst | 4 +-
> drivers/net/mlx5/Makefile| 5 ++
> drivers/net/mlx5/mlx5.c | 13
> drivers/net/mlx5/mlx5.h | 1 +
> drivers/net/mlx5/mlx5_flow.c | 154
> +--
> 5 files changed, 170 insertions(+), 7 deletions(-)
>[...]
> @@ -896,7 +928,9 @@ struct ibv_spec_header {
> if (ret)
> goto exit_item_not_supported;
> if (IS_TUNNEL(items->type)) {
> - if (parser->tunnel) {
> + if (parser->tunnel &&
> +!((items - 1)->type == RTE_FLOW_ITEM_TYPE_GRE &&
> + items->type == RTE_FLOW_ITEM_TYPE_MPLS)) {
> rte_flow_error_set(error, ENOTSUP,
> RTE_FLOW_ERROR_TYPE_ITEM,
> items,
> @@ -904,6 +938,16 @@ struct ibv_spec_header {
> " tunnel encapsulations.");
> return -rte_errno;
> }
> + if (items->type == RTE_FLOW_ITEM_TYPE_MPLS &&
> + !priv->config.mpls_en) {
> + rte_flow_error_set(error, ENOTSUP,
> +RTE_FLOW_ERROR_TYPE_ITEM,
> +items,
> +"MPLS not supported or"
> +" disabled in firmware"
> +" configuration.");
> + return -rte_errno;
> + }
> if (!priv->config.tunnel_en &&
> parser->rss_conf.level > 1) {
> rte_flow_error_set(error, ENOTSUP,
Doing such comparison will not work in all cases like
GRE / VOID / MPLS
which is totally valid from rte_flow perspective.
Regards,
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH v2] net/mlx5: add bluefield device ID
nectX-4 Lx and ConnectX-5
> + It is currently only supported on the ConnectX-4 Lx, ConnectX-5 and
> Bluefield
>families of adapters. Enabled by default.
>
> - ``txq_mpw_hdr_dseg_en`` parameter [int]
> @@ -352,14 +353,14 @@ Run-time configuration
>
> - ``tx_vec_en`` parameter [int]
>
> - A nonzero value enables Tx vector on ConnectX-5 only NIC if the number of
> + A nonzero value enables Tx vector on ConnectX-5 and Bluefield NICs if the
> number of
>global Tx queues on the port is lesser than MLX5_VPMD_MIN_TXQS.
>
>This option cannot be used with certain offloads such as
> ``DEV_TX_OFFLOAD_TCP_TSO,
>DEV_TX_OFFLOAD_VXLAN_TNL_TSO, DEV_TX_OFFLOAD_GRE_TNL_TSO,
> DEV_TX_OFFLOAD_VLAN_INSERT``.
>When those offloads are requested the MPS send function will not be used.
>
> - Enabled by default on ConnectX-5.
> + Enabled by default on ConnectX-5 and Bluefield.
>
> - ``rx_vec_en`` parameter [int]
>
> @@ -422,8 +423,9 @@ DPDK and must be installed separately:
>
> - **libmlx5**
>
> - Low-level user space driver library for Mellanox ConnectX-4/ConnectX-5
> - devices, it is automatically loaded by libibverbs.
> + Low-level user space driver library for Mellanox
> + ConnectX-4/ConnectX-5/Bluefield devices, it is automatically loaded
> + by libibverbs.
>
>This library basically implements send/receive calls to the hardware
>queues.
> @@ -437,15 +439,16 @@ DPDK and must be installed separately:
>Unlike most other PMDs, these modules must remain loaded and bound to
>their devices:
>
> - - mlx5_core: hardware driver managing Mellanox ConnectX-4/ConnectX-5
> -devices and related Ethernet kernel network devices.
> + - mlx5_core: hardware driver managing Mellanox
> +ConnectX-4/ConnectX-5/Bluefield devices and related Ethernet kernel
> +network devices.
>- mlx5_ib: InifiniBand device driver.
>- ib_uverbs: user space driver for Verbs (entry point for libibverbs).
>
> - **Firmware update**
>
> - Mellanox OFED releases include firmware updates for ConnectX-4/ConnectX-5
> - adapters.
> + Mellanox OFED releases include firmware updates for
> + ConnectX-4/ConnectX-5/Bluefield adapters.
>
>Because each release provides new features, these updates must be applied
> to
>match the kernel modules and libraries they come with.
> @@ -482,6 +485,7 @@ Mellanox OFED
>- ConnectX-4 Lx: **14.21.1000** and above.
>- ConnectX-5: **16.21.1000** and above.
>- ConnectX-5 Ex: **16.21.1000** and above.
> + - Bluefield: **18.23.1000** and above.
>
> While these libraries and kernel modules are available on OpenFabrics
> Alliance's `website <https://www.openfabrics.org/>`__ and provided by package
> @@ -699,7 +703,7 @@ Usage example
> -
>
> This section demonstrates how to launch **testpmd** with Mellanox
> -ConnectX-4/ConnectX-5 devices managed by librte_pmd_mlx5.
> +ConnectX-4/ConnectX-5/Bluefield devices managed by librte_pmd_mlx5.
>
> #. Load the kernel modules:
>
> diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
> index 8aa91cc8ed..0ce45eb852 100644
> --- a/drivers/net/mlx5/mlx5.c
> +++ b/drivers/net/mlx5/mlx5.c
> @@ -1248,6 +1248,10 @@ static const struct rte_pci_id mlx5_pci_id_map[] = {
> PCI_DEVICE_ID_MELLANOX_CONNECTX5EXVF)
> },
> {
> + RTE_PCI_DEVICE(PCI_VENDOR_ID_MELLANOX,
> + PCI_DEVICE_ID_MELLANOX_CONNECTX5BF)
> + },
> + {
> .vendor_id = 0
> }
> };
> diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h
> index c4c962b92d..a9c692555e 100644
> --- a/drivers/net/mlx5/mlx5.h
> +++ b/drivers/net/mlx5/mlx5.h
> @@ -50,6 +50,7 @@ enum {
> PCI_DEVICE_ID_MELLANOX_CONNECTX5VF = 0x1018,
> PCI_DEVICE_ID_MELLANOX_CONNECTX5EX = 0x1019,
> PCI_DEVICE_ID_MELLANOX_CONNECTX5EXVF = 0x101a,
> + PCI_DEVICE_ID_MELLANOX_CONNECTX5BF = 0xa2d2,
> };
>
> LIST_HEAD(mlx5_dev_list, priv);
> --
> 2.12.0
>
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH v2 4/4] net/mlx4: add new Memory Region support
On Thu, May 10, 2018 at 08:29:03PM +0100, Ferruh Yigit wrote:
> On 5/10/2018 7:01 AM, Yongseok Koh wrote:
> >
> >> On May 9, 2018, at 8:00 PM, Yongseok Koh wrote:
> >>
> >>
> >>> On May 9, 2018, at 4:12 PM, Ferruh Yigit wrote:
> >>>
> >>> On 5/9/2018 12:09 PM, Yongseok Koh wrote:
> >>> <...>
> >>>
> >>>> +/**
> >>>> + * Insert an entry to B-tree lookup table.
> >>>> + *
> >>>> + * @param bt
> >>>> + * Pointer to B-tree structure.
> >>>> + * @param entry
> >>>> + * Pointer to new entry to insert.
> >>>> + *
> >>>> + * @return
> >>>> + * 0 on success, -1 on failure.
> >>>> + */
> >>>> +static int
> >>>> +mr_btree_insert(struct mlx4_mr_btree *bt, struct mlx4_mr_cache *entry)
> >>>> +{
> >>>> +struct mlx4_mr_cache *lkp_tbl;
> >>>> +uint16_t idx = 0;
> >>>> +size_t shift;
> >>>> +
> >>>> +assert(bt != NULL);
> >>>> +assert(bt->len <= bt->size);
> >>>> +assert(bt->len > 0);
> >>>> +lkp_tbl = *bt->table;
> >>>> +/* Find out the slot for insertion. */
> >>>> +if (mr_btree_lookup(bt, &idx, entry->start) != UINT32_MAX) {
> >>>> +DEBUG("abort insertion to B-tree(%p):"
> >>>> + " already exist at idx=%u [0x%lx, 0x%lx)
> >>>> lkey=0x%x",
> >>>> + (void *)bt, idx, entry->start, entry->end,
> >>>> entry->lkey);
> >>>
> >>> This and various other logs causing 32bits build error because of %lx
> >>> usage. Can
> >>> you please check them?
> >>>
> >>> I am feeling sad to complain a patch like this just because of log format
> >>> issue,
> >>> we should find a solution to this issue as community, either checkpatch
> >>> checks
> >>> or automated 32bit builds, I don't know.
> >>
> >> Bummer. I have to change my bad habit of using %lx. And we will add 32-bit
> >> build
> >> check to our internal system to filter this kind of mistakes beforehand.
> >>
> >> Will work with Shahaf to fix it and rebase next-net-mlx.
> >
> > Ferruh, I've sent out a patch to Shahaf to change printing format
> > specifiers and
> > Shahaf will squash it into the previous patches.
> >
> > However, it seems we had stopped supporting 32-bit compilation since Nelio's
> > commit [1]
> >
> > Not sure I'm doing right but I'm compiling it for
> > T=i686-native-linuxapp-gcc and
> > still having a few more errors even except for my code. And even if I fix
> > all of
> > the errors, linkage fails as explained in the commit message of [1].
> >
> > Are you sure you encountered this 32b compilation issue for the first time?
On mlx5 32 bits has been disabled as Mellanox OFED does not support
32bits compilation whereas RDMA-Core supports it.
I've just taken a look on Mellanox Website, Mellanox OFED 4.3-3.0.2.1 is
still not available for 32bits. We cannot assume the support exists.
> I do just compilation on mlx drivers.
> And building only mlx4 for 32bits, mlx5 doesn't support 32bits as you point
> out
> below patch. mlx4 32bit compiles fine with me as same config you have used.
>
> Also features documentation [2] verifies this, mlx4 supports 32bits but mlx5
> not.
>
> [2]
> https://dpdk.org/browse/next/dpdk-next-net/tree/doc/guides/nics/features/mlx4.ini?h=v18.02#n32
> https://dpdk.org/browse/next/dpdk-next-net/tree/doc/guides/nics/features/mlx5.ini?h=v18.02#n42
>
> >
> >
> > [1]
> > http://dpdk.org/browse/dpdk/commit/?id=ebbb81eb27daca0a89ee8f228fcf141d9eb6ef1c
> >
> >
> > Thanks,
> > Yongseok
> >
> >
>
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH v2 2/2] net/mlx5: fix probe return value polarity
On Thu, May 03, 2018 at 10:59:37AM +0300, Shahaf Shuler wrote:
> mlx5 prefixed function returns a negative errno value.
> the error handler on mlx5_pci_probe is doing the same.
>
> Fixes: a6d83b6a9209 ("net/mlx5: standardize on negative errno values")
> Cc: nelio.laranje...@6wind.com
>
> Signed-off-by: Shahaf Shuler
> ---
>
> On v2:
> - fixed more polarity issues on different mlx5_* functions.
> - aligned verb calls return values.
>
> ---
> drivers/net/mlx5/mlx5.c | 31 ++-
> 1 file changed, 22 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
> index 46cb370a29..7afdae863f 100644
> --- a/drivers/net/mlx5/mlx5.c
> +++ b/drivers/net/mlx5/mlx5.c
> @@ -751,8 +751,9 @@ mlx5_pci_probe(struct rte_pci_driver *pci_drv
> __rte_unused,
> DRV_LOG(WARNING,
> "tunnel offloading disabled due to old OFED/rdma-core version");
> #endif
> - if (mlx5_glue->query_device_ex(attr_ctx, NULL, &device_attr)) {
> - err = errno;
> + err = mlx5_glue->query_device_ex(attr_ctx, NULL, &device_attr);
> + if (err) {
> + DRV_LOG(ERR, "ibv_query_device_ex() failed");
Should be DEBUG() and not DRV_LOG().
> goto error;
> }
> DRV_LOG(INFO, "%u port(s) detected",
> @@ -800,16 +801,22 @@ mlx5_pci_probe(struct rte_pci_driver *pci_drv
> __rte_unused,
> eth_dev->device = &pci_dev->device;
> eth_dev->dev_ops = &mlx5_dev_sec_ops;
> err = mlx5_uar_init_secondary(eth_dev);
> - if (err)
> + if (err) {
> + err = rte_errno;
> goto error;
> + }
> /* Receive command fd from primary process */
> err = mlx5_socket_connect(eth_dev);
> - if (err < 0)
> + if (err < 0) {
> + err = rte_errno;
> goto error;
> + }
> /* Remap UAR for Tx queues. */
> err = mlx5_tx_uar_remap(eth_dev, err);
> - if (err)
> + if (err) {
> + err = rte_errno;
> goto error;
> + }
> /*
>* Ethdev pointer is still required as input since
>* the primary device is not accessible from the
> @@ -873,11 +880,12 @@ mlx5_pci_probe(struct rte_pci_driver *pci_drv
> __rte_unused,
> if (err) {
> DRV_LOG(ERR, "failed to process device arguments: %s",
> strerror(err));
> + err = rte_errno;
> goto port_error;
> }
> - if (mlx5_glue->query_device_ex(ctx, NULL, &device_attr_ex)) {
> + err = mlx5_glue->query_device_ex(ctx, NULL, &device_attr_ex);
> + if (err) {
> DRV_LOG(ERR, "ibv_query_device_ex() failed");
> - err = errno;
> goto port_error;
> }
> config.hw_csum = !!(device_attr_ex.device_cap_flags_ex &
> @@ -952,8 +960,10 @@ mlx5_pci_probe(struct rte_pci_driver *pci_drv
> __rte_unused,
> rte_eth_copy_pci_info(eth_dev, pci_dev);
> eth_dev->device->driver = &mlx5_driver.driver;
> err = mlx5_uar_init_primary(eth_dev);
> - if (err)
> + if (err) {
> + err = rte_errno;
> goto port_error;
> + }
> /* Configure the first MAC address by default. */
> if (mlx5_get_mac(eth_dev, &mac.addr_bytes)) {
> DRV_LOG(ERR,
> @@ -983,8 +993,10 @@ mlx5_pci_probe(struct rte_pci_driver *pci_drv
> __rte_unused,
> #endif
> /* Get actual MTU if possible. */
> err = mlx5_get_mtu(eth_dev, &priv->mtu);
> - if (err)
> + if (err) {
> + err = rte_errno;
> goto port_error;
> + }
> DRV_LOG(DEBUG, "port %u MTU is %u", eth_dev->data->port_id,
> priv->mtu);
> /*
> @@ -1031,6 +1043,7 @@ mlx5_pci_probe(struct rte_pci_driver *pci_drv
> __rte_unused,
> if (err) {
> DRV_LOG(ERR, "port %u drop queue allocation failed: %s",
> eth_dev->data->port_id, strerror(rte_errno));
> + err = rte_errno;
> goto port_error;
> }
> /* Supported Verbs flow priority number detection. */
> --
> 2.12.0
Unless the small comment above,
Acked-by: Nelio Laranjeiro
Thanks,
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [dpdk-stable] [PATCH] net/mlx5: fix: flow validation
On Thu, May 03, 2018 at 07:07:54AM +, Shahaf Shuler wrote:
> Hi Nelio,
>
> Wednesday, May 2, 2018 5:43 PM, Nelio Laranjeiro:
> > Subject: [dpdk-stable] [PATCH] net/mlx5: fix: flow validation
>
> The title is wrong the : after the fix should be removed.
Right,
> > Item spec and last are wrongly compared to the NIC capability causing a
> > validation failure when the mask is null.
> > This validation function should only verify the user is not configuring
> > unsupported matching fields.
> >
> > Fixes: 2097d0d1e2cc ("net/mlx5: support basic flow items and actions")
> > Cc: sta...@dpdk.org
> >
> > Signed-off-by: Nelio Laranjeiro
> > ---
>[...]
> > - rte_errno = EINVAL;
> > - return -rte_errno;
> > - }
> > + if (!spec && (item->mask || last))
> > + goto error;
> > + if (!spec)
> > + return 0;
> > + for (i = 0; i < size; i++) {
>
>
> I think inline comment which explains what each code section below
> verifies would much help.
Adding it,
> > + if (spec)
> > + if (((spec[i] & m[i]) | mask[i]) != mask[i])
> > + goto error;
>
> Am wondering.
> Which the below check of m ...
>
> > + if (last)
> > + if last[i] & m[i]) | mask[i]) != mask[i]) ||
> > + ((spec[i] & m[i]) != (last[i] & m[i])))
> > + goto error;
> > + if (m)
> > + if ((m[i] | mask[i]) != mask[i])
> > + goto error;
>
> Do we really need to spec check?
> Meaning if above one passes it is guarantee m is contained in mask.
> And if so, then the spec check will always succeed.
Indeed,
> > }
> > return 0;
> > +error:
> > + rte_errno = ENOTSUP;
> > + return -rte_errno;
> > }
> >
> > /**
> > --
> > 2.17.0
I am making a v2 accordingly.
Thanks,
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH] net/mlx5: fix device reference in secondary process
On Tue, May 01, 2018 at 11:13:20PM -0700, Yongseok Koh wrote:
> rte_eth_devices[] is not shared between primary and secondary process, but
> a static array to each process. The backward pointer of device (priv->dev)
> must be reset when a secondary process attaches to a device.
>
> Fixes: f8b9a3bad467 ("net/mlx5: install a socket to exchange a file
> descriptor")
> Cc: sta...@dpdk.org
>
> Signed-off-by: Yongseok Koh
Acked-by: Nelio Laranjeiro
> ---
> drivers/net/mlx5/mlx5.c | 6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
> index 8f983061a..f606e3dd4 100644
> --- a/drivers/net/mlx5/mlx5.c
> +++ b/drivers/net/mlx5/mlx5.c
> @@ -799,6 +799,12 @@ mlx5_pci_probe(struct rte_pci_driver *pci_drv
> __rte_unused,
> }
> eth_dev->device = &pci_dev->device;
> eth_dev->dev_ops = &mlx5_dev_sec_ops;
> + /*
> + * rte_eth_devices[] is not shared but static to each
> + * process. The backward pointer should be reset.
> + */
> + priv = eth_dev->data->dev_private;
> + priv->dev = eth_dev;
> err = mlx5_uar_init_secondary(eth_dev);
> if (err)
> goto error;
> --
> 2.11.0
>
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH 1/2] net/mlx5: fix socket connection return value
On Tue, May 01, 2018 at 02:18:05PM +0300, Shahaf Shuler wrote:
> Upon success, mlx5_socket_connect should return the fd descriptor of the
> primary process
>
> Fixes: a6d83b6a9209 ("net/mlx5: standardize on negative errno values")
> Cc: nelio.laranje...@6wind.com
>
> Signed-off-by: Yongseok Koh
> Signed-off-by: Shahaf Shuler
Acked-by: Nelio Laranjeiro
> ---
> drivers/net/mlx5/mlx5.c| 2 +-
> drivers/net/mlx5/mlx5_socket.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
> index 8f983061a0..46cb370a29 100644
> --- a/drivers/net/mlx5/mlx5.c
> +++ b/drivers/net/mlx5/mlx5.c
> @@ -804,7 +804,7 @@ mlx5_pci_probe(struct rte_pci_driver *pci_drv
> __rte_unused,
> goto error;
> /* Receive command fd from primary process */
> err = mlx5_socket_connect(eth_dev);
> - if (err)
> + if (err < 0)
> goto error;
> /* Remap UAR for Tx queues. */
> err = mlx5_tx_uar_remap(eth_dev, err);
> diff --git a/drivers/net/mlx5/mlx5_socket.c b/drivers/net/mlx5/mlx5_socket.c
> index e12c4cb2e1..99297d5c43 100644
> --- a/drivers/net/mlx5/mlx5_socket.c
> +++ b/drivers/net/mlx5/mlx5_socket.c
> @@ -294,7 +294,7 @@ mlx5_socket_connect(struct rte_eth_dev *dev)
> }
> ret = *fd;
> close(socket_fd);
> - return 0;
> + return ret;
> error:
> if (socket_fd != -1)
> close(socket_fd);
> --
> 2.12.0
>
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH 2/2] net/mlx5: fix probe return value polarity
On Wed, May 02, 2018 at 01:54:37AM +, Yongseok Koh wrote:
>
> > On May 1, 2018, at 4:18 AM, Shahaf Shuler wrote:
> >
> > mlx5 prefixed function returns a negative errno value.
> > the error handler on mlx5_pci_probe is doing the same.
> >
> > Fixes: a6d83b6a9209 ("net/mlx5: standardize on negative errno values")
> > Cc: nelio.laranje...@6wind.com
> >
> > Signed-off-by: Shahaf Shuler
> > ---
> > drivers/net/mlx5/mlx5.c | 8 ++--
> > 1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
> > index 46cb370a29..ab860b5985 100644
> > --- a/drivers/net/mlx5/mlx5.c
> > +++ b/drivers/net/mlx5/mlx5.c
> > @@ -804,12 +804,16 @@ mlx5_pci_probe(struct rte_pci_driver *pci_drv
> > __rte_unused,
> > goto error;
>
> Shouldn't you do the same for mlx5_uar_init_secondary()?
> Looks like a few more. E.g. mlx5_args(), mlx5_get_mtu() and
> mlx5_uar_init_primary().
> What about ibv_query_port() and mlx5_flow_create_drop_queue()?
>
> Thanks
>
> > /* Receive command fd from primary process */
> > err = mlx5_socket_connect(eth_dev);
> > - if (err < 0)
> > + if (err < 0) {
> > + err = -err;
>
> Instead of changing sign, how about 'err = rte_errno;' ?
+1
> However, err looks redundant to me because mlx5_* funcs set rte_errno.
Not it is not, rte_errno is the strict equivalent of errno but instead
of being global to the process, it is global per logical core, its
cannot be determined nor modified at the beginning of the function thus
the function must track any failure and report it accordingly.
> Here, err is used to set rte_errno at the end.
It is just a optimization to avoid a lot of rte_errno sets among the
function, it must only be set if err != 0.
> Thanks,
> Yongseok
>
> > goto error;
> > + }
> > /* Remap UAR for Tx queues. */
> > err = mlx5_tx_uar_remap(eth_dev, err);
> > - if (err)
> > + if (err) {
> > + err = -err;
> > goto error;
> > + }
> > /*
> > * Ethdev pointer is still required as input since
> > * the primary device is not accessible from the
> > --
> > 2.12.0
> >
Regards,
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH v2 2/2] net/mlx5: add Rx and Tx tuning parameters
On Tue, May 01, 2018 at 12:58:49PM +0300, Shahaf Shuler wrote:
> A new ethdev API was exposed by
> commit 3be82f5cc5e3 ("ethdev: support PMD-tuned Tx/Rx parameters")
>
> Enabling the PMD to provide default parameters in case no strict request
> from application in order to improve the out of the box experience.
>
> While the current API lacks the means for the PMD to provide the best
> possible value, providing the best default the PMD can guess.
> The values are based on Mellanox performance report and depends on the
> underlying NIC capabilities.
>
> Cc: ere...@mellanox.com
> Cc: am...@mellanox.com
> Cc: ol...@mellanox.com
>
> Signed-off-by: Shahaf Shuler
Acked-by: Nelio Laranjeiro
> ---
>
> On v2:
> - refactor the code to ease the maintenance.
>
> ---
> drivers/net/mlx5/mlx5_ethdev.c | 40
> 1 file changed, 40 insertions(+)
>
> diff --git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c
> index 588d4ba627..3fad199a60 100644
> --- a/drivers/net/mlx5/mlx5_ethdev.c
> +++ b/drivers/net/mlx5/mlx5_ethdev.c
> @@ -417,6 +417,45 @@ mlx5_dev_configure(struct rte_eth_dev *dev)
> }
>
> /**
> + * Sets default tuning parameters.
> + *
> + * @param dev
> + * Pointer to Ethernet device.
> + * @param[out] info
> + * Info structure output buffer.
> + */
> +static void
> +mlx5_set_default_params(struct rte_eth_dev *dev, struct rte_eth_dev_info
> *info)
> +{
> + struct priv *priv = dev->data->dev_private;
> +
> + /* Minimum CPU utilization. */
> + info->default_rxportconf.ring_size = 256;
> + info->default_txportconf.ring_size = 256;
> + info->default_rxportconf.burst_size = 64;
> + info->default_txportconf.burst_size = 64;
> + if (priv->link_speed_capa & ETH_LINK_SPEED_100G) {
> + info->default_rxportconf.nb_queues = 16;
> + info->default_txportconf.nb_queues = 16;
> + if (dev->data->nb_rx_queues > 2 ||
> + dev->data->nb_tx_queues > 2) {
> + /* Max Throughput. */
> + info->default_rxportconf.ring_size = 2048;
> + info->default_txportconf.ring_size = 2048;
> + }
> + } else {
> + info->default_rxportconf.nb_queues = 8;
> + info->default_txportconf.nb_queues = 8;
> + if (dev->data->nb_rx_queues > 2 ||
> + dev->data->nb_tx_queues > 2) {
> + /* Max Throughput. */
> + info->default_rxportconf.ring_size = 4096;
> + info->default_txportconf.ring_size = 4096;
> + }
> + }
> +}
> +
> +/**
> * DPDK callback to get information about the device.
> *
> * @param dev
> @@ -458,6 +497,7 @@ mlx5_dev_infos_get(struct rte_eth_dev *dev, struct
> rte_eth_dev_info *info)
> info->hash_key_size = rss_hash_default_key_len;
> info->speed_capa = priv->link_speed_capa;
> info->flow_type_rss_offloads = ~MLX5_RSS_HF_MASK;
> + mlx5_set_default_params(dev, info);
> }
>
> /**
> --
> 2.12.0
>
Thanks,
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH 1/2] net/mlx5: fix ethtool link setting call order
On Sun, Apr 29, 2018 at 09:03:07PM +0300, Shahaf Shuler wrote:
> According to ethtool_link_setting API recommendation ETHTOOL_GLINKSETTINGS
> should be called before ETHTOOL_GSET as the later one deprecated.
>
> Fixes: f47ba80080ab ("net/mlx5: remove kernel version check")
> Cc: nelio.laranje...@6wind.com
>
> Signed-off-by: Shahaf Shuler
Acked-by: Nelio Laranjeiro
> ---
> drivers/net/mlx5/mlx5_ethdev.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c
> index 746b94f734..588d4ba627 100644
> --- a/drivers/net/mlx5/mlx5_ethdev.c
> +++ b/drivers/net/mlx5/mlx5_ethdev.c
> @@ -697,9 +697,9 @@ mlx5_link_update(struct rte_eth_dev *dev, int
> wait_to_complete)
> time_t start_time = time(NULL);
>
> do {
> - ret = mlx5_link_update_unlocked_gset(dev, &dev_link);
> + ret = mlx5_link_update_unlocked_gs(dev, &dev_link);
> if (ret)
> - ret = mlx5_link_update_unlocked_gs(dev, &dev_link);
> + ret = mlx5_link_update_unlocked_gset(dev, &dev_link);
> if (ret == 0)
> break;
> /* Handle wait to complete situation. */
> --
> 2.12.0
>
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH 2/2] net/mlx5: add Rx and Tx tuning parameters
On Sun, Apr 29, 2018 at 09:03:08PM +0300, Shahaf Shuler wrote:
> A new ethdev API was exposed by
> commit 3be82f5cc5e3 ("ethdev: support PMD-tuned Tx/Rx parameters")
>
> Enabling the PMD to provide default parameters in case no strict request
> from application in order to improve the out of the box experience.
>
> While the current API lacks the means for the PMD to provide the best
> possible value, providing the best default the PMD can guess.
> The values are based on Mellanox performance report and depends on the
> underlying NIC capabilities.
>
> Cc: ere...@mellanox.com
> Cc: am...@mellanox.com
> Cc: ol...@mellanox.com
>
> Signed-off-by: Shahaf Shuler
> ---
> drivers/net/mlx5/mlx5_ethdev.c | 51
> ++
> 1 file changed, 51 insertions(+)
>
> diff --git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c
> index 588d4ba627..78354922b0 100644
> --- a/drivers/net/mlx5/mlx5_ethdev.c
> +++ b/drivers/net/mlx5/mlx5_ethdev.c
> @@ -417,6 +417,56 @@ mlx5_dev_configure(struct rte_eth_dev *dev)
> }
>
> /**
> + * Sets default tuning parameters.
> + *
> + * @param dev
> + * Pointer to Ethernet device.
> + * @param[out] info
> + * Info structure output buffer.
> + */
> +static void
> +mlx5_set_default_params(struct rte_eth_dev *dev, struct rte_eth_dev_info
> *info)
> +{
> + struct priv *priv = dev->data->dev_private;
> +
> + if (priv->link_speed_capa & ETH_LINK_SPEED_100G) {
> + if (dev->data->nb_rx_queues <= 2 &&
> + dev->data->nb_tx_queues <= 2) {
> + /* Minimum CPU utilization. */
> + info->default_rxportconf.ring_size = 256;
> + info->default_txportconf.ring_size = 256;
> + /* Don't care as queue num is set. */
> + info->default_rxportconf.nb_queues = 0;
> + info->default_txportconf.nb_queues = 0;
> + } else {
> + /* Max Throughput. */
> + info->default_rxportconf.ring_size = 2048;
> + info->default_txportconf.ring_size = 2048;
> + info->default_rxportconf.nb_queues = 16;
> + info->default_txportconf.nb_queues = 16;
> + }
> + } else {
> + if (dev->data->nb_rx_queues <= 2 &&
> + dev->data->nb_tx_queues <= 2) {
> + /* Minimum CPU utilization. */
> + info->default_rxportconf.ring_size = 256;
> + info->default_txportconf.ring_size = 256;
> + /* Don't care as queue num is set. */
> + info->default_rxportconf.nb_queues = 0;
> + info->default_txportconf.nb_queues = 0;
> + } else {
> + /* Max Throughput. */
> + info->default_rxportconf.ring_size = 4096;
> + info->default_txportconf.ring_size = 4096;
> + info->default_rxportconf.nb_queues = 8;
> + info->default_txportconf.nb_queues = 8;
> + }
> + }
> + info->default_rxportconf.burst_size = 64;
> + info->default_txportconf.burst_size = 64;
This can be fully re-written to simplify and ease the maintenance,
default values i.e. "Minimum CPU utilization" are duplicated, this can
be used as default values and just tweak in case the amount of queues
are different from 2 according to the link speed.
> +}
> +
> +/**
> * DPDK callback to get information about the device.
> *
> * @param dev
> @@ -458,6 +508,7 @@ mlx5_dev_infos_get(struct rte_eth_dev *dev, struct
> rte_eth_dev_info *info)
> info->hash_key_size = rss_hash_default_key_len;
> info->speed_capa = priv->link_speed_capa;
> info->flow_type_rss_offloads = ~MLX5_RSS_HF_MASK;
> + mlx5_set_default_params(dev, info);
> }
>
> /**
> --
> 2.12.0
Thanks,
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH v1] net/mlx5: fix flow director rule deletion crash
On Thu, Apr 26, 2018 at 06:17:46PM +0200, Adrien Mazarguil wrote:
> Flow director rules matching traffic properties above layer 2 do not
> target a fixed hash Rx queue (HASH_RXQ_ETH), it actually depends on the
> highest protocol layer specified by each flow rule.
>
> mlx5_fdir_filter_delete() makes this wrong assumption and causes a crash
> when attempting to destroy flow rules with L3/L4 specifications.
>
> Fixes: 4c3e9bcdd52e ("net/mlx5: support flow director")
> Cc: Nelio Laranjeiro
> Cc: sta...@dpdk.org
>
> Signed-off-by: Adrien Mazarguil
Acked-by: Nelio Laranjeiro
> ---
> drivers/net/mlx5/mlx5_flow.c | 10 +-
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
> index 8f5fcf4d6..05def2b14 100644
> --- a/drivers/net/mlx5/mlx5_flow.c
> +++ b/drivers/net/mlx5/mlx5_flow.c
> @@ -3409,13 +3409,13 @@ mlx5_fdir_filter_delete(struct rte_eth_dev *dev,
> if (parser.drop) {
> struct ibv_flow_spec_action_drop *drop;
>
> - drop = (void *)((uintptr_t)parser.queue[HASH_RXQ_ETH].ibv_attr +
> - parser.queue[HASH_RXQ_ETH].offset);
> + drop = (void *)((uintptr_t)parser.queue[parser.layer].ibv_attr +
> + parser.queue[parser.layer].offset);
> *drop = (struct ibv_flow_spec_action_drop){
> .type = IBV_FLOW_SPEC_ACTION_DROP,
> .size = sizeof(struct ibv_flow_spec_action_drop),
> };
> - parser.queue[HASH_RXQ_ETH].ibv_attr->num_of_specs++;
> + parser.queue[parser.layer].ibv_attr->num_of_specs++;
> }
> TAILQ_FOREACH(flow, &priv->flows, next) {
> struct ibv_flow_attr *attr;
> @@ -3426,8 +3426,8 @@ mlx5_fdir_filter_delete(struct rte_eth_dev *dev,
> void *flow_spec;
> unsigned int specs_n;
>
> - attr = parser.queue[HASH_RXQ_ETH].ibv_attr;
> - flow_attr = flow->frxq[HASH_RXQ_ETH].ibv_attr;
> + attr = parser.queue[parser.layer].ibv_attr;
> + flow_attr = flow->frxq[parser.layer].ibv_attr;
> /* Compare first the attributes. */
> if (memcmp(attr, flow_attr, sizeof(struct ibv_flow_attr)))
> continue;
> --
> 2.11.0
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH v6 00/11] mlx5 Rx tunnel offloading
On Mon, Apr 23, 2018 at 08:32:59PM +0800, Xueming Li wrote: > > Important note: > please note that this patchset relies on Adrien's patchset of flow API > overhaul: http://www.dpdk.org/dev/patchwork/patch/38508/ > v6: > - Fixed commit log of tunnel type identification > v5: > - Removed %lx prints > - Per review request, clear mbuf tunnel type in case of multiple tunnel types. > - Rebase on Adriens flow API overhaul patchset > - Split feature requirement document into patches of L3 VXLAN and VXLAN-GPE > - Per review request, add device parameter to enable L3 VXLAN and VXLAN-GPE > v4: > - Fix RSS level according to value defination > - Add "Inner RSS" column to NIC feature doc > - Fixed flow creation error in case of ipv4 rss on ipv6 pattern > - new patch: enforce IP protocol of GRE to be 47. > - Removed MPLS-in-UDP and MPLS-in-GRE replated patchset > - Removed invalid RSS type check > v3: > - Refactor 16 Verbs priority detection. > - Other updates according to ML discussion. > v2: > - Split into 2 series: public api and mlx5, this one is the second. > - Rebased on Adrien's rte flow overhaul: > http://www.dpdk.org/ml/archives/dev/2018-April/095774.html > v1: > - Support new tunnel type MPLS-in-GRE and MPLS-in-UDP > - Remove deprecation notes of rss level > > This patchset supports MLX5 Rx tunnel checksum, inner rss, inner ptype > offloading of following tunnel types: > - Standard VXLAN > - L3 VXLAN (no inner ethernet header) > - VXLAN-GPE > > Xueming Li (11): > net/mlx5: support 16 hardware priorities > net/mlx5: support GRE tunnel flow > net/mlx5: support L3 VXLAN flow > net/mlx5: support Rx tunnel type identification > net/mlx5: cleanup tunnel checksum offloads > net/mlx5: split flow RSS handling logic > net/mlx5: support tunnel RSS level > net/mlx5: add hardware flow debug dump > net/mlx5: introduce VXLAN-GPE tunnel type > net/mlx5: allow flow tunnel ID 0 with outer pattern > doc: update mlx5 guide on tunnel offloading > > doc/guides/nics/features/default.ini | 1 + > doc/guides/nics/features/mlx5.ini | 3 + > doc/guides/nics/mlx5.rst | 30 +- > drivers/net/mlx5/Makefile | 2 +- > drivers/net/mlx5/mlx5.c | 24 + > drivers/net/mlx5/mlx5.h | 6 + > drivers/net/mlx5/mlx5_flow.c | 844 > +++--- > drivers/net/mlx5/mlx5_glue.c | 16 + > drivers/net/mlx5/mlx5_glue.h | 8 + > drivers/net/mlx5/mlx5_rxq.c | 89 +++- > drivers/net/mlx5/mlx5_rxtx.c | 33 +- > drivers/net/mlx5/mlx5_rxtx.h | 11 +- > drivers/net/mlx5/mlx5_rxtx_vec_neon.h | 21 +- > drivers/net/mlx5/mlx5_rxtx_vec_sse.h | 17 +- > drivers/net/mlx5/mlx5_trigger.c | 8 - > drivers/net/mlx5/mlx5_utils.h | 6 + > 16 files changed, 896 insertions(+), 223 deletions(-) > > -- > 2.13.3 > I think we have caught almost all issues, if something remains fixes can be added. Acked-by: Nelio Laranjeiro -- Nélio Laranjeiro 6WIND
Re: [dpdk-dev] [PATCH v6 02/11] net/mlx5: support GRE tunnel flow
On Tue, Apr 24, 2018 at 07:40:24AM +, Xueming(Steven) Li wrote:
>
>
> > -Original Message-
> > From: Nélio Laranjeiro
> > Sent: Monday, April 23, 2018 9:46 PM
> > To: Xueming(Steven) Li
> > Cc: Shahaf Shuler ; dev@dpdk.org
> > Subject: Re: [PATCH v6 02/11] net/mlx5: support GRE tunnel flow
> >
> > On Mon, Apr 23, 2018 at 01:32:23PM +, Xueming(Steven) Li wrote:
> > > Hi Nelio,
> > >
> > > > -Original Message-
> > > > From: Nélio Laranjeiro
> > > > Sent: Monday, April 23, 2018 8:56 PM
> > > > To: Xueming(Steven) Li
> > > > Cc: Shahaf Shuler ; dev@dpdk.org
> > > > Subject: Re: [PATCH v6 02/11] net/mlx5: support GRE tunnel flow
> > > >
> > > > On Mon, Apr 23, 2018 at 08:33:01PM +0800, Xueming Li wrote:
> > > > > Signed-off-by: Xueming Li
> > > > > ---
> > > > > drivers/net/mlx5/mlx5_flow.c | 101
> > > > > ---
> > > > > 1 file changed, 94 insertions(+), 7 deletions(-)
> > > > >
> > > > > diff --git a/drivers/net/mlx5/mlx5_flow.c
> > > > > b/drivers/net/mlx5/mlx5_flow.c index 5402cb148..b365f9868 100644
> > > > > --- a/drivers/net/mlx5/mlx5_flow.c
> > > > > +++ b/drivers/net/mlx5/mlx5_flow.c
> > > > > @@ -37,6 +37,7 @@
> > > > > /* Internet Protocol versions. */ #define MLX5_IPV4 4 #define
> > > > > MLX5_IPV6 6
> > > > > +#define MLX5_GRE 47
> > > > >
> > > > > #ifndef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT
> > > > > struct ibv_flow_spec_counter_action { @@ -89,6 +90,11 @@
> > > > > mlx5_flow_create_vxlan(const struct rte_flow_item *item,
> > > > > const void *default_mask,
> > > > > struct mlx5_flow_data *data);
> > > > >
> > > > > +static int
> > > > > +mlx5_flow_create_gre(const struct rte_flow_item *item,
> > > > > + const void *default_mask,
> > > > > + struct mlx5_flow_data *data);
> > > > > +
> > > > > struct mlx5_flow_parse;
> > > > >
> > > > > static void
> > > > > @@ -231,6 +237,10 @@ struct rte_flow {
> > > > > __VA_ARGS__, RTE_FLOW_ITEM_TYPE_END, \
> > > > > }
> > > > >
> > > > > +#define IS_TUNNEL(type) ( \
> > > > > + (type) == RTE_FLOW_ITEM_TYPE_VXLAN || \
> > > > > + (type) == RTE_FLOW_ITEM_TYPE_GRE)
> > > > > +
> > > > > /** Structure to generate a simple graph of layers supported by
> > > > > the NIC. */ struct mlx5_flow_items {
> > > > > /** List of possible actions for these items. */ @@ -284,7
> > > > > +294,8 @@ static const enum rte_flow_action_type valid_actions[] =
> > > > > { static const struct mlx5_flow_items mlx5_flow_items[] = {
> > > > > [RTE_FLOW_ITEM_TYPE_END] = {
> > > > > .items = ITEMS(RTE_FLOW_ITEM_TYPE_ETH,
> > > > > -RTE_FLOW_ITEM_TYPE_VXLAN),
> > > > > +RTE_FLOW_ITEM_TYPE_VXLAN,
> > > > > +RTE_FLOW_ITEM_TYPE_GRE),
> > > > > },
> > > > > [RTE_FLOW_ITEM_TYPE_ETH] = {
> > > > > .items = ITEMS(RTE_FLOW_ITEM_TYPE_VLAN, @@ -316,7
> > > > > +327,8 @@
> > > > > static const struct mlx5_flow_items mlx5_flow_items[] = {
> > > > > },
> > > > > [RTE_FLOW_ITEM_TYPE_IPV4] = {
> > > > > .items = ITEMS(RTE_FLOW_ITEM_TYPE_UDP,
> > > > > -RTE_FLOW_ITEM_TYPE_TCP),
> > > > > +RTE_FLOW_ITEM_TYPE_TCP,
> > > > > +RTE_FLOW_ITEM_TYPE_GRE),
> > > > > .actions = valid_actions,
> > > > > .mask = &(const struct rte_flow_item_ipv4){
> > > > > .hdr = {
> > > > > @@ -333,7 +345,8 @@ static const struct mlx5_flow_items
> > > > > mlx5_flow_items[] = {
> > > > > },
> > > > > [RTE_FLOW_ITEM_TYPE_IPV6] = {
> > > > > .items = ITEMS(RTE_FLOW_ITEM_TYPE_U
Re: [dpdk-dev] [PATCH v5 07/16] ethdev: flatten RSS configuration in flow API
On Thu, Apr 19, 2018 at 12:16:39PM +0200, Adrien Mazarguil wrote:
> Since its inception, the rte_flow RSS action has been relying in part on
> external struct rte_eth_rss_conf for compatibility with the legacy RSS API.
> This structure lacks parameters such as the hash algorithm to use, and more
> recently, a method to tell which layer RSS should be performed on [1].
>
> Given struct rte_eth_rss_conf will never be flexible enough to represent a
> complete RSS configuration (e.g. RETA table), this patch supersedes it by
> extending the rte_flow RSS action directly.
>
> A subsequent patch will add a field to use a non-default RSS hash
> algorithm. To that end, a field named "types" replaces the field formerly
> known as "rss_hf" and standing for "RSS hash functions" as it was
> confusing. Actual RSS hash function types are defined by enum
> rte_eth_hash_function.
>
> This patch updates all PMDs and example applications accordingly.
>
> It breaks ABI compatibility for the following public functions:
>
> - rte_flow_copy()
> - rte_flow_create()
> - rte_flow_query()
> - rte_flow_validate()
>
> [1] commit 676b605182a5 ("doc: announce ethdev API change for RSS
> configuration")
>
> Signed-off-by: Adrien Mazarguil
> Acked-by: Andrew Rybchenko
> Cc: Xueming Li
> Cc: Ferruh Yigit
> Cc: Thomas Monjalon
> Cc: Wenzhuo Lu
> Cc: Jingjing Wu
> Cc: Beilei Xing
> Cc: Qi Zhang
> Cc: Konstantin Ananyev
> Cc: Nelio Laranjeiro
> Cc: Yongseok Koh
> Cc: Andrew Rybchenko
> Cc: Pascal Mazon
> Cc: Radu Nicolau
> Cc: Akhil Goyal
>
> ---
>
> v3 changes:
>
> Documentation update regarding the meaning of a 0 value for RSS types in
> flow rules.
>
> It used to implicitly mean "no RSS" but is redefined as requesting a kind
> of "best-effort" mode from PMDs, i.e. anything ranging from empty to
> all-inclusive RSS; what matters is it provides safe defaults that will work
> regardless of PMD capabilities.
> ---
>[...]
> diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
> @@ -642,17 +599,7 @@ mlx5_flow_convert_actions(struct rte_eth_dev *dev,
> enum { FATE = 1, MARK = 2, COUNT = 4, };
> uint32_t overlap = 0;
> struct priv *priv = dev->data->dev_private;
> - int ret;
>
> - /*
> - * Add default RSS configuration necessary for Verbs to create QP even
> - * if no RSS is necessary.
> - */
> - ret = mlx5_flow_convert_rss_conf(parser,
> - (const struct rte_eth_rss_conf *)
> - &priv->rss_conf);
This is still needed for QUEUE action, Verbs refuses to create an hash
Rx queue if no RSS key is provided even if the hash field is 0.
This can be fully moved to mlx5_hrxq_new() who can use the default key
when the rss_key is not provided.
Regards,
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH v6 02/11] net/mlx5: support GRE tunnel flow
On Mon, Apr 23, 2018 at 01:32:23PM +, Xueming(Steven) Li wrote:
> Hi Nelio,
>
> > -Original Message-
> > From: Nélio Laranjeiro
> > Sent: Monday, April 23, 2018 8:56 PM
> > To: Xueming(Steven) Li
> > Cc: Shahaf Shuler ; dev@dpdk.org
> > Subject: Re: [PATCH v6 02/11] net/mlx5: support GRE tunnel flow
> >
> > On Mon, Apr 23, 2018 at 08:33:01PM +0800, Xueming Li wrote:
> > > Signed-off-by: Xueming Li
> > > ---
> > > drivers/net/mlx5/mlx5_flow.c | 101
> > > ---
> > > 1 file changed, 94 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/net/mlx5/mlx5_flow.c
> > > b/drivers/net/mlx5/mlx5_flow.c index 5402cb148..b365f9868 100644
> > > --- a/drivers/net/mlx5/mlx5_flow.c
> > > +++ b/drivers/net/mlx5/mlx5_flow.c
> > > @@ -37,6 +37,7 @@
> > > /* Internet Protocol versions. */
> > > #define MLX5_IPV4 4
> > > #define MLX5_IPV6 6
> > > +#define MLX5_GRE 47
> > >
> > > #ifndef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT
> > > struct ibv_flow_spec_counter_action { @@ -89,6 +90,11 @@
> > > mlx5_flow_create_vxlan(const struct rte_flow_item *item,
> > > const void *default_mask,
> > > struct mlx5_flow_data *data);
> > >
> > > +static int
> > > +mlx5_flow_create_gre(const struct rte_flow_item *item,
> > > + const void *default_mask,
> > > + struct mlx5_flow_data *data);
> > > +
> > > struct mlx5_flow_parse;
> > >
> > > static void
> > > @@ -231,6 +237,10 @@ struct rte_flow {
> > > __VA_ARGS__, RTE_FLOW_ITEM_TYPE_END, \
> > > }
> > >
> > > +#define IS_TUNNEL(type) ( \
> > > + (type) == RTE_FLOW_ITEM_TYPE_VXLAN || \
> > > + (type) == RTE_FLOW_ITEM_TYPE_GRE)
> > > +
> > > /** Structure to generate a simple graph of layers supported by the
> > > NIC. */ struct mlx5_flow_items {
> > > /** List of possible actions for these items. */ @@ -284,7 +294,8 @@
> > > static const enum rte_flow_action_type valid_actions[] = { static
> > > const struct mlx5_flow_items mlx5_flow_items[] = {
> > > [RTE_FLOW_ITEM_TYPE_END] = {
> > > .items = ITEMS(RTE_FLOW_ITEM_TYPE_ETH,
> > > -RTE_FLOW_ITEM_TYPE_VXLAN),
> > > +RTE_FLOW_ITEM_TYPE_VXLAN,
> > > +RTE_FLOW_ITEM_TYPE_GRE),
> > > },
> > > [RTE_FLOW_ITEM_TYPE_ETH] = {
> > > .items = ITEMS(RTE_FLOW_ITEM_TYPE_VLAN, @@ -316,7 +327,8 @@
> > > static
> > > const struct mlx5_flow_items mlx5_flow_items[] = {
> > > },
> > > [RTE_FLOW_ITEM_TYPE_IPV4] = {
> > > .items = ITEMS(RTE_FLOW_ITEM_TYPE_UDP,
> > > -RTE_FLOW_ITEM_TYPE_TCP),
> > > +RTE_FLOW_ITEM_TYPE_TCP,
> > > +RTE_FLOW_ITEM_TYPE_GRE),
> > > .actions = valid_actions,
> > > .mask = &(const struct rte_flow_item_ipv4){
> > > .hdr = {
> > > @@ -333,7 +345,8 @@ static const struct mlx5_flow_items mlx5_flow_items[]
> > > = {
> > > },
> > > [RTE_FLOW_ITEM_TYPE_IPV6] = {
> > > .items = ITEMS(RTE_FLOW_ITEM_TYPE_UDP,
> > > -RTE_FLOW_ITEM_TYPE_TCP),
> > > +RTE_FLOW_ITEM_TYPE_TCP,
> > > +RTE_FLOW_ITEM_TYPE_GRE),
> > > .actions = valid_actions,
> > > .mask = &(const struct rte_flow_item_ipv6){
> > > .hdr = {
> > > @@ -386,6 +399,19 @@ static const struct mlx5_flow_items
> > > mlx5_flow_items[] = {
> > > .convert = mlx5_flow_create_tcp,
> > > .dst_sz = sizeof(struct ibv_flow_spec_tcp_udp),
> > > },
> > > + [RTE_FLOW_ITEM_TYPE_GRE] = {
> > > + .items = ITEMS(RTE_FLOW_ITEM_TYPE_ETH,
> > > +RTE_FLOW_ITEM_TYPE_IPV4,
> > > +RTE_FLOW_ITEM_TYPE_IPV6),
> > > + .actions = valid_actions,
> > > + .mask = &(const struct rte_flow_item_gre){
> > > + .protocol = -1,
> > > + },
> > > + .default_mask = &rte_flow_item_gre_mask,
> > > + .mask_sz = sizeof(struct rte_flow_item_gre),
> > > + .convert =
Re: [dpdk-dev] [PATCH v6 02/11] net/mlx5: support GRE tunnel flow
if (IS_TUNNEL(items->type)) {
> if (parser->inner) {
> rte_flow_error_set(error, ENOTSUP,
> RTE_FLOW_ERROR_TYPE_ITEM,
> items,
> -"cannot recognize multiple"
> -" VXLAN encapsulations");
> +"Cannot recognize multiple"
> +" tunnel encapsulations.");
> return -rte_errno;
> }
> parser->inner = IBV_FLOW_SPEC_INNER;
> @@ -1641,6 +1667,67 @@ mlx5_flow_create_vxlan(const struct rte_flow_item
> *item,
> }
>
> /**
> + * Convert GRE item to Verbs specification.
> + *
> + * @param item[in]
> + * Item specification.
> + * @param default_mask[in]
> + * Default bit-masks to use when item->mask is not provided.
> + * @param data[in, out]
> + * User structure.
> + *
> + * @return
> + * 0 on success, a negative errno value otherwise and rte_errno is set.
> + */
> +static int
> +mlx5_flow_create_gre(const struct rte_flow_item *item __rte_unused,
> + const void *default_mask __rte_unused,
> + struct mlx5_flow_data *data)
> +{
> + struct mlx5_flow_parse *parser = data->parser;
> + unsigned int size = sizeof(struct ibv_flow_spec_tunnel);
> + struct ibv_flow_spec_tunnel tunnel = {
> + .type = parser->inner | IBV_FLOW_SPEC_VXLAN_TUNNEL,
> + .size = size,
> + };
> + struct ibv_flow_spec_ipv4_ext *ipv4;
> + struct ibv_flow_spec_ipv6 *ipv6;
> + unsigned int i;
> +
> + parser->inner = IBV_FLOW_SPEC_INNER;
> + /* Update encapsulation IP layer protocol. */
> + for (i = 0; i != hash_rxq_init_n; ++i) {
> + if (!parser->queue[i].ibv_attr)
> + continue;
> + if (parser->out_layer == HASH_RXQ_IPV4) {
> + ipv4 = (void *)((uintptr_t)parser->queue[i].ibv_attr +
> + parser->queue[i].offset -
> + sizeof(struct ibv_flow_spec_ipv4_ext));
> + if (ipv4->mask.proto && ipv4->val.proto != MLX5_GRE)
> + break;
> + ipv4->val.proto = MLX5_GRE;
> + ipv4->mask.proto = 0xff;
> + } else if (parser->out_layer == HASH_RXQ_IPV6) {
> + ipv6 = (void *)((uintptr_t)parser->queue[i].ibv_attr +
> + parser->queue[i].offset -
> + sizeof(struct ibv_flow_spec_ipv6));
> + if (ipv6->mask.next_hdr &&
> + ipv6->val.next_hdr != MLX5_GRE)
> + break;
> + ipv6->val.next_hdr = MLX5_GRE;
> + ipv6->mask.next_hdr = 0xff;
> + }
> + }
> + if (i != hash_rxq_init_n)
> + return rte_flow_error_set(data->error, EINVAL,
> + RTE_FLOW_ERROR_TYPE_ITEM,
> + item,
> + "IP protocol of GRE must be 47");
> + mlx5_flow_create_copy(parser, &tunnel, size);
> + return 0;
> +}
There is something strange, item is not unused as it is at least used in
the rte_flow_error_set().
In the other series you are pushing, there is no new RTE_FLOW_ITEM_GRE
and in the current code there is also no RTE_FLOW_ITEM_GRE.
I don't see how this code can match the missing item, what am I missing?
> +/**
> * Convert mark/flag action to Verbs specification.
> *
> * @param parser
> --
> 2.13.3
Thanks,
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH v2 3/3] net/mlx5: implement multicast add list devop
On Mon, Apr 23, 2018 at 07:57:36AM +, Shahaf Shuler wrote:
> Monday, April 23, 2018 10:33 AM, Nélio Laranjeiro:
> [...]
> > > > +/**
> > > > + * DPDK callback to set multicast addresses list.
> > > > + *
> > > > + * @param dev
> > > > + * Pointer to Ethernet device structure.
> > > > + * @param mac_addr_set
> > > > + * Multicast MAC address pointer array.
> > > > + * @param nb_mac_addr
> > > > + * Number of entries in the array.
> > > > + *
> > > > + * @return
> > > > + * 0 on success, a negative errno value otherwise and rte_errno is
> > > > set.
> > > > + */
> > > > +int
> > > > +mlx5_set_mc_addr_list(struct rte_eth_dev *dev,
> > > > + struct ether_addr *mc_addr_set, uint32_t
> > > > nb_mc_addr) {
> > > > + uint32_t i;
> > > > + int ret;
> > > > +
> > >
> > > We should check nb_mc_addr < MLX5_MAX_MC_MAC_ADDRESSES
> > before we start
> > > operate.
> >
> > This verification is done in the sub function.
>
> I see only assert. Did I missed anything?
No.
> > Considering an application calling such API wants to remove/replace the old
> > list with new entries.
> > That this new one can be just an addition or totally different list or even
> > empty.
> > This new list can be larger than the amount of MAC addresses the PMD can
> > support.
> >
> > There are two possibilities:
> >
> > 1. The list is too large: the application will enable the all multicast
> > mode to
> > receive the extra mac addresses it needs.
>
> How can application know the size of the MC list?
> only the UC size is being reported:
> info->max_mac_addrs = MLX5_MAX_UC_MAC_ADDRESSES
Such information is not reported at all. The application has to guess.
> > 2. The list fits (or empty): no issues.
> >
> > At the end the application can also call this API with an empty list to
> > clear it
> > before/after enabling the "all multicast" mode.
> > The final result being the same, does it worse to add a duplicated
> > verification?
>
> At the current code if the list is too large and the PMD was compiled
> w/o debug mode it will results in seg fault.
Right it needs a verification.
> > Note: if an error happens the new list is not committed yet i.e. the traffic
> > remains untouched.
> >
> > > > + for (i = MLX5_MAX_UC_MAC_ADDRESSES; i !=
> > > > MLX5_MAX_MAC_ADDRESSES; ++i)
> > > > + mlx5_internal_mac_addr_remove(dev, i);
> > > > + i = MLX5_MAX_UC_MAC_ADDRESSES;
> > > > + while (nb_mc_addr--) {
> > >
> > > Maybe worth checking is_multicast_ether_addr(mc_addr_set) and to skip
> > > + warn if it is not.
> >
> > Such verification should be done in the public API i.e. ethdev.
>
> I don't understand.
> In the first patch of the series you add extra verification to check
> the mac address validity.
It only verify the MAC address is not zero, it does not verify the MAC
address is valid in the function context (e.g. unicast in
mlx5_mac_addr_add()).
> But for the MC you claim it should be done on ethdev layer.
Dito.
> It should be consistant. Either ethdev verify the MAC address or the
> PMD. If the first one, then there is no need to add the
> is_zero_ether_addr check on the first patch.
It is consistent, the PMD only verify the MAC address is not zero and
this in both API.
> > > > + ret = mlx5_internal_mac_addr_add(dev, mc_addr_set++,
> > > > i++);
> > > > + if (ret)
> > > > + return ret;
> > > > + }
> > > > + if (!dev->data->promiscuous)
> > > > + return mlx5_traffic_restart(dev);
> > > > + return 0;
> > > > +}
> > > > --
> > > > 2.17.0
Regards,
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH v5 04/11] net/mlx5: support Rx tunnel type identification
On Fri, Apr 20, 2018 at 08:23:33PM +0800, Xueming Li wrote: > This patch introduced tunnel type identification based on flow rules. > If flows of multiple tunnel types built on same queue, > RTE_PTYPE_TUNNEL_MASK will be returned, user application could use bits > in flow mark as tunnel type identifier. >[...] There is still the issue about returning this wrong bits in the mbuf. Bit in the mbuf ptypes must only reflect what is present in the mbuf, using RTE_PTYPE_TUNNEL_MASK means all tunnels are present in the packet which is absolutely wrong. This behavior was not announce and breaks API/ABI. It cannot be accepted yet. I'll suggest to add a new RTE_PTYPE_TUNNEL_UNKNOWN which does not break the ABI or don't add such bits in the mbuf. Regards, -- Nélio Laranjeiro 6WIND
Re: [dpdk-dev] [PATCH v2 3/3] net/mlx5: implement multicast add list devop
lication will enable the all multicast
mode to receive the extra mac addresses it needs.
2. The list fits (or empty): no issues.
At the end the application can also call this API with an empty list to
clear it before/after enabling the "all multicast" mode.
The final result being the same, does it worse to add a duplicated
verification?
Note: if an error happens the new list is not committed yet i.e. the
traffic remains untouched.
> > + for (i = MLX5_MAX_UC_MAC_ADDRESSES; i !=
> > MLX5_MAX_MAC_ADDRESSES; ++i)
> > + mlx5_internal_mac_addr_remove(dev, i);
> > + i = MLX5_MAX_UC_MAC_ADDRESSES;
> > + while (nb_mc_addr--) {
>
> Maybe worth checking is_multicast_ether_addr(mc_addr_set) and to skip
> + warn if it is not.
Such verification should be done in the public API i.e. ethdev.
> > + ret = mlx5_internal_mac_addr_add(dev, mc_addr_set++,
> > i++);
> > + if (ret)
> > + return ret;
> > + }
> > + if (!dev->data->promiscuous)
> > + return mlx5_traffic_restart(dev);
> > + return 0;
> > +}
> > --
> > 2.17.0
Regards,
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH v4 03/11] net/mlx5: support L3 VXLAN flow
On Thu, Apr 19, 2018 at 12:49:41PM +, Xueming(Steven) Li wrote:
>
>
> > -Original Message-
> > From: Nélio Laranjeiro
> > Sent: Thursday, April 19, 2018 8:19 PM
> > To: Xueming(Steven) Li
> > Cc: Shahaf Shuler ; dev@dpdk.org
> > Subject: Re: [PATCH v4 03/11] net/mlx5: support L3 VXLAN flow
> >
> > On Thu, Apr 19, 2018 at 11:53:05AM +, Xueming(Steven) Li wrote:
> > >
> > >
> > > > -Original Message-
> > > > From: Nélio Laranjeiro
> > > > Sent: Thursday, April 19, 2018 7:15 PM
> > > > To: Xueming(Steven) Li
> > > > Cc: Shahaf Shuler ; dev@dpdk.org
> > > > Subject: Re: [PATCH v4 03/11] net/mlx5: support L3 VXLAN flow
> > > >
> > > > On Thu, Apr 19, 2018 at 10:21:26AM +, Xueming(Steven) Li wrote:
> > > > >
> > > > >
> > > > > > -Original Message-
> > > > > > From: Nélio Laranjeiro
> > > > > > Sent: Thursday, April 19, 2018 2:56 PM
> > > > > > To: Xueming(Steven) Li
> > > > > > Cc: Shahaf Shuler ; dev@dpdk.org
> > > > > > Subject: Re: [PATCH v4 03/11] net/mlx5: support L3 VXLAN flow
> > > > > >
> > > > > > On Thu, Apr 19, 2018 at 06:20:50AM +, Xueming(Steven) Li wrote:
> > > > > > >
> > > > > > >
> > > > > > > > -Original Message-
> > > > > > > > From: Nélio Laranjeiro
> > > > > > > > Sent: Wednesday, April 18, 2018 11:09 PM
> > > > > > > > To: Xueming(Steven) Li
> > > > > > > > Cc: Shahaf Shuler ; dev@dpdk.org
> > > > > > > > Subject: Re: [PATCH v4 03/11] net/mlx5: support L3 VXLAN
> > > > > > > > flow
> > > > > > > >
> > > > > > > > On Wed, Apr 18, 2018 at 02:43:30PM +, Xueming(Steven) Li
> > > > > > > > wrote:
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > > -Original Message-
> > > > > > > > > > From: Nélio Laranjeiro
> > > > > > > > > > Sent: Wednesday, April 18, 2018 2:49 PM
> > > > > > > > > > To: Xueming(Steven) Li
> > > > > > > > > > Cc: Shahaf Shuler ; dev@dpdk.org
> > > > > > > > > > Subject: Re: [PATCH v4 03/11] net/mlx5: support L3 VXLAN
> > > > > > > > > > flow
> > > > > > > > > >
> > > > > > > > > > On Tue, Apr 17, 2018 at 11:14:28PM +0800, Xueming Li wrote:
> > > > > > > > > > > This patch support L3 VXLAN, no inner L2 header
> > > > > > > > > > > comparing to standard VXLAN protocol. L3 VXLAN using
> > > > > > > > > > > specific overlay UDP destination port to discriminate
> > > > > > > > > > > against standard VXLAN, FW has to be configured to
> > > > > > > > > > > support
> > > > > > > > > > > it:
> > > > > > > > > > > sudo mlxconfig -d -y s IP_OVER_VXLAN_EN=1
> > > > > > > > > > > sudo mlxconfig -d -y s
> > > > > > > > > > > IP_OVER_VXLAN_PORT=
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Xueming Li
> > > > > > > > > > > ---
> > > > > > > > > > > drivers/net/mlx5/mlx5_flow.c | 4 +++-
> > > > > > > > > > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > > > > > > > > > >
> > > > > > > > > > > diff --git a/drivers/net/mlx5/mlx5_flow.c
> > > > > > > > > > > b/drivers/net/mlx5/mlx5_flow.c index
> > > > > > > > > > > 771d5f14d..d7a921dff
> > > > > > > > > > > 100644
> > > > > > > > > > > --- a/drivers/net/mlx5/mlx5_flow.c
> > > > > > > > > > > +++ b/drivers/net/mlx5/mlx5_flow.c
> > > > > > > > > > > @@ -413,7 +413,9 @@ static const struct mlx5_flow_items
> > > > > > > > > > > mlx5_flow_items[] = {
> > > &g
Re: [dpdk-dev] [PATCH v4 03/11] net/mlx5: support L3 VXLAN flow
On Thu, Apr 19, 2018 at 11:53:05AM +, Xueming(Steven) Li wrote:
>
>
> > -Original Message-
> > From: Nélio Laranjeiro
> > Sent: Thursday, April 19, 2018 7:15 PM
> > To: Xueming(Steven) Li
> > Cc: Shahaf Shuler ; dev@dpdk.org
> > Subject: Re: [PATCH v4 03/11] net/mlx5: support L3 VXLAN flow
> >
> > On Thu, Apr 19, 2018 at 10:21:26AM +, Xueming(Steven) Li wrote:
> > >
> > >
> > > > -Original Message-
> > > > From: Nélio Laranjeiro
> > > > Sent: Thursday, April 19, 2018 2:56 PM
> > > > To: Xueming(Steven) Li
> > > > Cc: Shahaf Shuler ; dev@dpdk.org
> > > > Subject: Re: [PATCH v4 03/11] net/mlx5: support L3 VXLAN flow
> > > >
> > > > On Thu, Apr 19, 2018 at 06:20:50AM +, Xueming(Steven) Li wrote:
> > > > >
> > > > >
> > > > > > -Original Message-
> > > > > > From: Nélio Laranjeiro
> > > > > > Sent: Wednesday, April 18, 2018 11:09 PM
> > > > > > To: Xueming(Steven) Li
> > > > > > Cc: Shahaf Shuler ; dev@dpdk.org
> > > > > > Subject: Re: [PATCH v4 03/11] net/mlx5: support L3 VXLAN flow
> > > > > >
> > > > > > On Wed, Apr 18, 2018 at 02:43:30PM +, Xueming(Steven) Li wrote:
> > > > > > >
> > > > > > >
> > > > > > > > -Original Message-
> > > > > > > > From: Nélio Laranjeiro
> > > > > > > > Sent: Wednesday, April 18, 2018 2:49 PM
> > > > > > > > To: Xueming(Steven) Li
> > > > > > > > Cc: Shahaf Shuler ; dev@dpdk.org
> > > > > > > > Subject: Re: [PATCH v4 03/11] net/mlx5: support L3 VXLAN
> > > > > > > > flow
> > > > > > > >
> > > > > > > > On Tue, Apr 17, 2018 at 11:14:28PM +0800, Xueming Li wrote:
> > > > > > > > > This patch support L3 VXLAN, no inner L2 header comparing
> > > > > > > > > to standard VXLAN protocol. L3 VXLAN using specific
> > > > > > > > > overlay UDP destination port to discriminate against
> > > > > > > > > standard VXLAN, FW has to be configured to support
> > > > > > > > > it:
> > > > > > > > > sudo mlxconfig -d -y s IP_OVER_VXLAN_EN=1
> > > > > > > > > sudo mlxconfig -d -y s
> > > > > > > > > IP_OVER_VXLAN_PORT=
> > > > > > > > >
> > > > > > > > > Signed-off-by: Xueming Li
> > > > > > > > > ---
> > > > > > > > > drivers/net/mlx5/mlx5_flow.c | 4 +++-
> > > > > > > > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/net/mlx5/mlx5_flow.c
> > > > > > > > > b/drivers/net/mlx5/mlx5_flow.c index 771d5f14d..d7a921dff
> > > > > > > > > 100644
> > > > > > > > > --- a/drivers/net/mlx5/mlx5_flow.c
> > > > > > > > > +++ b/drivers/net/mlx5/mlx5_flow.c
> > > > > > > > > @@ -413,7 +413,9 @@ static const struct mlx5_flow_items
> > > > > > > > > mlx5_flow_items[] = {
> > > > > > > > > .dst_sz = sizeof(struct ibv_flow_spec_tunnel),
> > > > > > > > > },
> > > > > > > > > [RTE_FLOW_ITEM_TYPE_VXLAN] = {
> > > > > > > > > - .items = ITEMS(RTE_FLOW_ITEM_TYPE_ETH),
> > > > > > > > > + .items = ITEMS(RTE_FLOW_ITEM_TYPE_ETH,
> > > > > > > > > +RTE_FLOW_ITEM_TYPE_IPV4, /* For
> > > > > > > > > L3 VXLAN. */
> > > > > > > > > +RTE_FLOW_ITEM_TYPE_IPV6), /* For
> > > > > > > > > L3 VXLAN. */
> > > > > > > > > .actions = valid_actions,
> > > > > > > > > .mask = &(const struct rte_flow_item_vxlan){
> > > > > > > > > .vni = "\xff\xff\xff",
> > > > > > > > > --
> > > > > > > > > 2.13.3
> > > > > >
Re: [dpdk-dev] [PATCH v4 03/11] net/mlx5: support L3 VXLAN flow
On Thu, Apr 19, 2018 at 10:21:26AM +, Xueming(Steven) Li wrote:
>
>
> > -Original Message-
> > From: Nélio Laranjeiro
> > Sent: Thursday, April 19, 2018 2:56 PM
> > To: Xueming(Steven) Li
> > Cc: Shahaf Shuler ; dev@dpdk.org
> > Subject: Re: [PATCH v4 03/11] net/mlx5: support L3 VXLAN flow
> >
> > On Thu, Apr 19, 2018 at 06:20:50AM +, Xueming(Steven) Li wrote:
> > >
> > >
> > > > -Original Message-
> > > > From: Nélio Laranjeiro
> > > > Sent: Wednesday, April 18, 2018 11:09 PM
> > > > To: Xueming(Steven) Li
> > > > Cc: Shahaf Shuler ; dev@dpdk.org
> > > > Subject: Re: [PATCH v4 03/11] net/mlx5: support L3 VXLAN flow
> > > >
> > > > On Wed, Apr 18, 2018 at 02:43:30PM +, Xueming(Steven) Li wrote:
> > > > >
> > > > >
> > > > > > -Original Message-
> > > > > > From: Nélio Laranjeiro
> > > > > > Sent: Wednesday, April 18, 2018 2:49 PM
> > > > > > To: Xueming(Steven) Li
> > > > > > Cc: Shahaf Shuler ; dev@dpdk.org
> > > > > > Subject: Re: [PATCH v4 03/11] net/mlx5: support L3 VXLAN flow
> > > > > >
> > > > > > On Tue, Apr 17, 2018 at 11:14:28PM +0800, Xueming Li wrote:
> > > > > > > This patch support L3 VXLAN, no inner L2 header comparing to
> > > > > > > standard VXLAN protocol. L3 VXLAN using specific overlay UDP
> > > > > > > destination port to discriminate against standard VXLAN, FW
> > > > > > > has to be configured to support
> > > > > > > it:
> > > > > > > sudo mlxconfig -d -y s IP_OVER_VXLAN_EN=1
> > > > > > > sudo mlxconfig -d -y s IP_OVER_VXLAN_PORT=
> > > > > > >
> > > > > > > Signed-off-by: Xueming Li
> > > > > > > ---
> > > > > > > drivers/net/mlx5/mlx5_flow.c | 4 +++-
> > > > > > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/drivers/net/mlx5/mlx5_flow.c
> > > > > > > b/drivers/net/mlx5/mlx5_flow.c index 771d5f14d..d7a921dff
> > > > > > > 100644
> > > > > > > --- a/drivers/net/mlx5/mlx5_flow.c
> > > > > > > +++ b/drivers/net/mlx5/mlx5_flow.c
> > > > > > > @@ -413,7 +413,9 @@ static const struct mlx5_flow_items
> > > > > > > mlx5_flow_items[] = {
> > > > > > > .dst_sz = sizeof(struct ibv_flow_spec_tunnel),
> > > > > > > },
> > > > > > > [RTE_FLOW_ITEM_TYPE_VXLAN] = {
> > > > > > > - .items = ITEMS(RTE_FLOW_ITEM_TYPE_ETH),
> > > > > > > + .items = ITEMS(RTE_FLOW_ITEM_TYPE_ETH,
> > > > > > > +RTE_FLOW_ITEM_TYPE_IPV4, /* For L3
> > > > > > > VXLAN. */
> > > > > > > +RTE_FLOW_ITEM_TYPE_IPV6), /* For L3
> > > > > > > VXLAN. */
> > > > > > > .actions = valid_actions,
> > > > > > > .mask = &(const struct rte_flow_item_vxlan){
> > > > > > > .vni = "\xff\xff\xff",
> > > > > > > --
> > > > > > > 2.13.3
> > > > > >
> > > > > > Such support must be under device parameter has it depends on
> > > > > > the configuration of the firmware. If the firmware is not
> > > > > > correctly configured the PMD must refuse
> > > > such rule.
> > > > > >
> > > > > > Thanks,
> > > > > >
> > > > > > --
> > > > > > Nélio Laranjeiro
> > > > > > 6WIND
> > > > >
> > > > > Are you suggesting Verbs parameter? I'm afraid we can't have it in
> > > > > short time, need new patch in later release when Verbs ready.
> > > >
> > > > Take a look at [1], this is what I mean.
> > >
> > > Enabling a new device parameter can't make L3 VXLAN packet get
> > > received if fw configuration not set.
> >
> > So you expect than the user will enable a feature without reading the PMD
> > documentation?
> > If it is the case, the answer it pretty simple, it is the
Re: [dpdk-dev] [PATCH v4 03/11] net/mlx5: support L3 VXLAN flow
On Thu, Apr 19, 2018 at 06:20:50AM +, Xueming(Steven) Li wrote:
>
>
> > -Original Message-
> > From: Nélio Laranjeiro
> > Sent: Wednesday, April 18, 2018 11:09 PM
> > To: Xueming(Steven) Li
> > Cc: Shahaf Shuler ; dev@dpdk.org
> > Subject: Re: [PATCH v4 03/11] net/mlx5: support L3 VXLAN flow
> >
> > On Wed, Apr 18, 2018 at 02:43:30PM +, Xueming(Steven) Li wrote:
> > >
> > >
> > > > -Original Message-
> > > > From: Nélio Laranjeiro
> > > > Sent: Wednesday, April 18, 2018 2:49 PM
> > > > To: Xueming(Steven) Li
> > > > Cc: Shahaf Shuler ; dev@dpdk.org
> > > > Subject: Re: [PATCH v4 03/11] net/mlx5: support L3 VXLAN flow
> > > >
> > > > On Tue, Apr 17, 2018 at 11:14:28PM +0800, Xueming Li wrote:
> > > > > This patch support L3 VXLAN, no inner L2 header comparing to
> > > > > standard VXLAN protocol. L3 VXLAN using specific overlay UDP
> > > > > destination port to discriminate against standard VXLAN, FW has to
> > > > > be configured to support
> > > > > it:
> > > > > sudo mlxconfig -d -y s IP_OVER_VXLAN_EN=1
> > > > > sudo mlxconfig -d -y s IP_OVER_VXLAN_PORT=
> > > > >
> > > > > Signed-off-by: Xueming Li
> > > > > ---
> > > > > drivers/net/mlx5/mlx5_flow.c | 4 +++-
> > > > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/net/mlx5/mlx5_flow.c
> > > > > b/drivers/net/mlx5/mlx5_flow.c index 771d5f14d..d7a921dff 100644
> > > > > --- a/drivers/net/mlx5/mlx5_flow.c
> > > > > +++ b/drivers/net/mlx5/mlx5_flow.c
> > > > > @@ -413,7 +413,9 @@ static const struct mlx5_flow_items
> > > > > mlx5_flow_items[] = {
> > > > > .dst_sz = sizeof(struct ibv_flow_spec_tunnel),
> > > > > },
> > > > > [RTE_FLOW_ITEM_TYPE_VXLAN] = {
> > > > > - .items = ITEMS(RTE_FLOW_ITEM_TYPE_ETH),
> > > > > + .items = ITEMS(RTE_FLOW_ITEM_TYPE_ETH,
> > > > > +RTE_FLOW_ITEM_TYPE_IPV4, /* For L3
> > > > > VXLAN. */
> > > > > +RTE_FLOW_ITEM_TYPE_IPV6), /* For L3
> > > > > VXLAN. */
> > > > > .actions = valid_actions,
> > > > > .mask = &(const struct rte_flow_item_vxlan){
> > > > > .vni = "\xff\xff\xff",
> > > > > --
> > > > > 2.13.3
> > > >
> > > > Such support must be under device parameter has it depends on the
> > > > configuration of the firmware. If the firmware is not correctly
> > > > configured the PMD must refuse
> > such rule.
> > > >
> > > > Thanks,
> > > >
> > > > --
> > > > Nélio Laranjeiro
> > > > 6WIND
> > >
> > > Are you suggesting Verbs parameter? I'm afraid we can't have it in
> > > short time, need new patch in later release when Verbs ready.
> >
> > Take a look at [1], this is what I mean.
>
> Enabling a new device parameter can't make L3 VXLAN packet get
> received if fw configuration not set.
So you expect than the user will enable a feature without reading the
PMD documentation?
If it is the case, the answer it pretty simple, it is the same as above,
read the PMD documentation.
> On the other hand, if fw continuation enabled and device parameter not
> set, packet could be received but failed to create rule.
Again a user using a NIC should read the documentation.
> I'm afraid that a device parameter will introduce complexity of using
> this feature w/o real benefits.
Add this missing device parameter and update accordingly the
documentation, or wait for Verbs to add the missing query feature.
If the firmware it not configured this rule must be refused, as there is
no way in the PMD to know if the firmware is configured, it must rely on
a device parameter.
Regards,
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH v4 03/11] net/mlx5: support L3 VXLAN flow
On Wed, Apr 18, 2018 at 02:43:30PM +, Xueming(Steven) Li wrote:
>
>
> > -Original Message-
> > From: Nélio Laranjeiro
> > Sent: Wednesday, April 18, 2018 2:49 PM
> > To: Xueming(Steven) Li
> > Cc: Shahaf Shuler ; dev@dpdk.org
> > Subject: Re: [PATCH v4 03/11] net/mlx5: support L3 VXLAN flow
> >
> > On Tue, Apr 17, 2018 at 11:14:28PM +0800, Xueming Li wrote:
> > > This patch support L3 VXLAN, no inner L2 header comparing to standard
> > > VXLAN protocol. L3 VXLAN using specific overlay UDP destination port
> > > to discriminate against standard VXLAN, FW has to be configured to
> > > support
> > > it:
> > > sudo mlxconfig -d -y s IP_OVER_VXLAN_EN=1
> > > sudo mlxconfig -d -y s IP_OVER_VXLAN_PORT=
> > >
> > > Signed-off-by: Xueming Li
> > > ---
> > > drivers/net/mlx5/mlx5_flow.c | 4 +++-
> > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/net/mlx5/mlx5_flow.c
> > > b/drivers/net/mlx5/mlx5_flow.c index 771d5f14d..d7a921dff 100644
> > > --- a/drivers/net/mlx5/mlx5_flow.c
> > > +++ b/drivers/net/mlx5/mlx5_flow.c
> > > @@ -413,7 +413,9 @@ static const struct mlx5_flow_items mlx5_flow_items[]
> > > = {
> > > .dst_sz = sizeof(struct ibv_flow_spec_tunnel),
> > > },
> > > [RTE_FLOW_ITEM_TYPE_VXLAN] = {
> > > - .items = ITEMS(RTE_FLOW_ITEM_TYPE_ETH),
> > > + .items = ITEMS(RTE_FLOW_ITEM_TYPE_ETH,
> > > +RTE_FLOW_ITEM_TYPE_IPV4, /* For L3 VXLAN. */
> > > +RTE_FLOW_ITEM_TYPE_IPV6), /* For L3 VXLAN. */
> > > .actions = valid_actions,
> > > .mask = &(const struct rte_flow_item_vxlan){
> > > .vni = "\xff\xff\xff",
> > > --
> > > 2.13.3
> >
> > Such support must be under device parameter has it depends on the
> > configuration of the firmware. If
> > the firmware is not correctly configured the PMD must refuse such rule.
> >
> > Thanks,
> >
> > --
> > Nélio Laranjeiro
> > 6WIND
>
> Are you suggesting Verbs parameter? I'm afraid we can't have it in
> short time, need new patch in later release when Verbs ready.
Take a look at [1], this is what I mean.
Regards,
[1] https://dpdk.org/doc/guides/nics/mlx5.html#run-time-configuration
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH v4 04/11] net/mlx5: support Rx tunnel type identification
On Wed, Apr 18, 2018 at 02:33:01PM +, Xueming(Steven) Li wrote: > > > > -Original Message- > > From: Nélio Laranjeiro > > Sent: Wednesday, April 18, 2018 2:51 PM > > To: Xueming(Steven) Li > > Cc: Shahaf Shuler ; dev@dpdk.org > > Subject: Re: [PATCH v4 04/11] net/mlx5: support Rx tunnel type > > identification > > > > On Tue, Apr 17, 2018 at 11:14:29PM +0800, Xueming Li wrote: > > > This patch introduced tunnel type identification based on flow rules. > > > If flows of multiple tunnel types built on same queue, > > > RTE_PTYPE_TUNNEL_MASK will be returned, user application could use > > > bits in flow mark as tunnel type identifier. > > > > As discussed in the previous thread, you cannot set all tunnel bits in the > > mbuf, it will break > > existing applications. > > > > This is an non announce API breakage. > > > > Thanks, > > > > -- > > Nélio Laranjeiro > > 6WIND > > There was another issue in code, please check below comments: > http://www.dpdk.org/ml/archives/dev/2018-April/096991.html I've already read this and I don't see anything related to changing the meaning of the mbuf flags. Such change is not announce, thus it cannot be accepted you must not set any flag in the mbuf if it cannot clearly identify what is in the packet. Regards, -- Nélio Laranjeiro 6WIND
Re: [dpdk-dev] [PATCH v4 11/11] doc: update mlx5 guide on tunnel offloading
On Tue, Apr 17, 2018 at 11:14:36PM +0800, Xueming Li wrote: > Remove tunnel limitations, add new hardware tunnel offload features. > > Signed-off-by: Xueming Li > --- > doc/guides/nics/features/default.ini | 1 + > doc/guides/nics/features/mlx5.ini| 3 +++ > doc/guides/nics/mlx5.rst | 22 -- > 3 files changed, 24 insertions(+), 2 deletions(-) > > diff --git a/doc/guides/nics/features/default.ini > b/doc/guides/nics/features/default.ini > index dae2ad776..49be81450 100644 > --- a/doc/guides/nics/features/default.ini > +++ b/doc/guides/nics/features/default.ini > @@ -29,6 +29,7 @@ Multicast MAC filter = > RSS hash = > RSS key update = > RSS reta update = > +Inner RSS= > VMDq = > SR-IOV = > DCB = > diff --git a/doc/guides/nics/features/mlx5.ini > b/doc/guides/nics/features/mlx5.ini > index f8ce08770..e75b14bdc 100644 > --- a/doc/guides/nics/features/mlx5.ini > +++ b/doc/guides/nics/features/mlx5.ini > @@ -21,6 +21,7 @@ Multicast MAC filter = Y > RSS hash = Y > RSS key update = Y > RSS reta update = Y > +Inner RSS= Y > SR-IOV = Y > VLAN filter = Y > Flow director= Y > @@ -30,6 +31,8 @@ VLAN offload = Y > L3 checksum offload = Y > L4 checksum offload = Y > Timestamp offload= Y > +Inner L3 checksum= Y > +Inner L4 checksum= Y > Packet type parsing = Y > Rx descriptor status = Y > Tx descriptor status = Y > diff --git a/doc/guides/nics/mlx5.rst b/doc/guides/nics/mlx5.rst > index c28c83278..51590b0a3 100644 > --- a/doc/guides/nics/mlx5.rst > +++ b/doc/guides/nics/mlx5.rst > @@ -74,12 +74,12 @@ Features > - RX interrupts. > - Statistics query including Basic, Extended and per queue. > - Rx HW timestamp. > +- Tunnel types: VXLAN, L3 VXLAN, VXLAN-GPE, GRE, MPLS-in-GRE, MPLS-in-UDP. > +- Tunnel HW offloads: packet type, inner/outer RSS, IP and UDP checksum > verification. > > Limitations > --- > > -- Inner RSS for VXLAN frames is not supported yet. > -- Hardware checksum RX offloads for VXLAN inner header are not supported yet. > - For secondary process: > >- Forked secondary process not supported. > @@ -327,6 +327,24 @@ Run-time configuration > >Enabled by default, valid only on VF devices ignored otherwise. > > +Firmware configuration > +~~ > + > +- L3 VXLAN and VXLAN-GPE destination UDP port > + > + .. code-block:: console > + > + mlxconfig -d set IP_OVER_VXLAN_EN=1 > + mlxconfig -d set IP_OVER_VXLAN_PORT= > + > + Verify configurations are set: > + > + .. code-block:: console > + > + mlxconfig -d query | grep IP_OVER_VXLAN > + IP_OVER_VXLAN_EN True(1) > + IP_OVER_VXLAN_PORT 4790 > + > Prerequisites > - > > -- > 2.13.3 The documentation modification related to the L3 VXLAN should be in the same patch as the code in mlx5. Thanks, -- Nélio Laranjeiro 6WIND
Re: [dpdk-dev] [PATCH v4 09/11] net/mlx5: introduce VXLAN-GPE tunnel type
On Tue, Apr 17, 2018 at 11:14:34PM +0800, Xueming Li wrote: > Signed-off-by: Xueming Li Acked-by: Nelio Laranjeiro > --- > drivers/net/mlx5/mlx5_flow.c | 99 > +++- > drivers/net/mlx5/mlx5_rxtx.c | 3 +- > 2 files changed, 99 insertions(+), 3 deletions(-) >[...] -- Nélio Laranjeiro 6WIND
Re: [dpdk-dev] [PATCH v4 08/11] net/mlx5: add hardware flow debug dump
On Tue, Apr 17, 2018 at 11:14:33PM +0800, Xueming Li wrote:
> Dump verb flow detail including flow spec type and size for debugging
> purpose.
>
> Signed-off-by: Xueming Li
> ---
> drivers/net/mlx5/mlx5_flow.c | 68
> ---
> drivers/net/mlx5/mlx5_rxq.c | 25 +---
> drivers/net/mlx5/mlx5_utils.h | 6
> 3 files changed, 85 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
> index a6791c525..371d029c8 100644
> --- a/drivers/net/mlx5/mlx5_flow.c
> +++ b/drivers/net/mlx5/mlx5_flow.c
> @@ -2050,6 +2050,57 @@ mlx5_flow_create_update_rxqs(struct rte_eth_dev *dev,
> struct rte_flow *flow)
> }
>
> /**
> + * Dump flow hash RX queue detail.
> + *
> + * @param dev
> + * Pointer to Ethernet device.
> + * @param flow
> + * Pointer to the rte_flow.
> + * @param i
> + * Hash RX queue index.
> + */
> +static void
> +mlx5_flow_dump(struct rte_eth_dev *dev __rte_unused,
> +struct rte_flow *flow __rte_unused,
> +unsigned int i __rte_unused)
Can this "i" be renamed to hrxq_idx to have something more
understandable across the code?
> +{
> +#ifndef NDEBUG
> + uintptr_t spec_ptr;
> + uint16_t j;
> + char buf[256];
> + uint8_t off;
> +
> + spec_ptr = (uintptr_t)(flow->frxq[i].ibv_attr + 1);
> + for (j = 0, off = 0; j < flow->frxq[i].ibv_attr->num_of_specs;
> + j++) {
> + struct ibv_flow_spec *spec = (void *)spec_ptr;
> + off += sprintf(buf + off, " %x(%hu)", spec->hdr.type,
> +spec->hdr.size);
> + spec_ptr += spec->hdr.size;
> + }
> + DRV_LOG(DEBUG,
> + "port %u Verbs flow %p type %u: hrxq:%p qp:%p ind:%p,
> hash:%lx/%u"
> + " specs:%hhu(%hu), priority:%hu, type:%d, flags:%x,"
> + " comp_mask:%x specs:%s",
> + dev->data->port_id, (void *)flow, i,
> + (void *)flow->frxq[i].hrxq,
> + (void *)flow->frxq[i].hrxq->qp,
> + (void *)flow->frxq[i].hrxq->ind_table,
> + flow->frxq[i].hash_fields |
> + (flow->tunnel &&
> + flow->rss_conf.level > 1 ? (uint32_t)IBV_RX_HASH_INNER : 0),
> + flow->rss_conf.queue_num,
> + flow->frxq[i].ibv_attr->num_of_specs,
> + flow->frxq[i].ibv_attr->size,
> + flow->frxq[i].ibv_attr->priority,
> + flow->frxq[i].ibv_attr->type,
> + flow->frxq[i].ibv_attr->flags,
> + flow->frxq[i].ibv_attr->comp_mask,
> + buf);
> +#endif
>[...]
Thanks,
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH v4 07/11] net/mlx5: support tunnel RSS level
On Tue, Apr 17, 2018 at 11:14:32PM +0800, Xueming Li wrote: > Tunnel RSS level of flow RSS action offers user a choice to do RSS hash > calculation on inner or outer RSS fields. Testpmd flow command examples: > > GRE flow inner RSS: > flow create 0 ingress pattern eth / ipv4 proto is 47 / gre / end > actions rss queues 1 2 end level 1 / end > > GRE tunnel flow outer RSS: > flow create 0 ingress pattern eth / ipv4 proto is 47 / gre / end > actions rss queues 1 2 end level 0 / end > > Signed-off-by: Xueming Li Acked-by: Nelio Laranjeiro -- Nélio Laranjeiro 6WIND
Re: [dpdk-dev] [PATCH v4 04/11] net/mlx5: support Rx tunnel type identification
On Tue, Apr 17, 2018 at 11:14:29PM +0800, Xueming Li wrote: > This patch introduced tunnel type identification based on flow rules. > If flows of multiple tunnel types built on same queue, > RTE_PTYPE_TUNNEL_MASK will be returned, user application could use bits > in flow mark as tunnel type identifier. As discussed in the previous thread, you cannot set all tunnel bits in the mbuf, it will break existing applications. This is an non announce API breakage. Thanks, -- Nélio Laranjeiro 6WIND
Re: [dpdk-dev] [PATCH v4 03/11] net/mlx5: support L3 VXLAN flow
On Tue, Apr 17, 2018 at 11:14:28PM +0800, Xueming Li wrote:
> This patch support L3 VXLAN, no inner L2 header comparing to standard
> VXLAN protocol. L3 VXLAN using specific overlay UDP destination port to
> discriminate against standard VXLAN, FW has to be configured to support
> it:
> sudo mlxconfig -d -y s IP_OVER_VXLAN_EN=1
> sudo mlxconfig -d -y s IP_OVER_VXLAN_PORT=
>
> Signed-off-by: Xueming Li
> ---
> drivers/net/mlx5/mlx5_flow.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
> index 771d5f14d..d7a921dff 100644
> --- a/drivers/net/mlx5/mlx5_flow.c
> +++ b/drivers/net/mlx5/mlx5_flow.c
> @@ -413,7 +413,9 @@ static const struct mlx5_flow_items mlx5_flow_items[] = {
> .dst_sz = sizeof(struct ibv_flow_spec_tunnel),
> },
> [RTE_FLOW_ITEM_TYPE_VXLAN] = {
> - .items = ITEMS(RTE_FLOW_ITEM_TYPE_ETH),
> + .items = ITEMS(RTE_FLOW_ITEM_TYPE_ETH,
> +RTE_FLOW_ITEM_TYPE_IPV4, /* For L3 VXLAN. */
> +RTE_FLOW_ITEM_TYPE_IPV6), /* For L3 VXLAN. */
> .actions = valid_actions,
> .mask = &(const struct rte_flow_item_vxlan){
> .vni = "\xff\xff\xff",
> --
> 2.13.3
Such support must be under device parameter has it depends on the
configuration of the firmware. If the firmware is not correctly
configured the PMD must refuse such rule.
Thanks,
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH v4 06/16] ethdev: remove C99 flexible arrays from flow API
On Tue, Apr 17, 2018 at 10:18:22PM +0200, Thomas Monjalon wrote: > 16/04/2018 18:22, Adrien Mazarguil: > > This patch replaces C99-style flexible arrays in struct rte_flow_action_rss > > and struct rte_flow_item_raw with standard pointers to the same data. > > > > They proved difficult to use in the field (e.g. no possibility of static > > initialization) and unsuitable for C++ applications. > > > > Affected PMDs and examples are updated accordingly. > > > > This breaks ABI compatibility for the following public functions: > > > > - rte_flow_copy() > > - rte_flow_create() > > - rte_flow_query() > > - rte_flow_validate() > > > > Signed-off-by: Adrien Mazarguil > > --- > > app/test-pmd/cmdline_flow.c| 117 +--- > > app/test-pmd/config.c | 25 --- > > doc/guides/prog_guide/rte_flow.rst | 18 ++--- > > drivers/net/mlx4/mlx4_flow.c | 22 +++--- > > drivers/net/mlx5/mlx5_flow.c | 20 +++--- > > examples/ipsec-secgw/ipsec.c | 17 ++--- > > lib/librte_ether/rte_flow.c| 25 --- > > lib/librte_ether/rte_flow.h| 8 ++- > > 8 files changed, 135 insertions(+), 117 deletions(-) > > There are almost as much insertions as deletions. > So it's probably not a bad move. > > Acked-by: Thomas Monjalon For mlx5: Acked-by: Nelio Laranjeiro -- Nélio Laranjeiro 6WIND
Re: [dpdk-dev] [PATCH 1/2] net/mlx5: fix alignment of Memory Region
On Tue, Apr 17, 2018 at 11:39:13AM -0700, Yongseok Koh wrote:
> The memory region is [start, end), so if the memseg of 'end' isn't
> allocated yet, the returned memseg will have zero entries and this will
> make 'end' zero (nil).
>
> Fixes: 718e35999c96 ("net/mlx5: use virt2memseg instead of iteration")
> Cc: Anatoly Burakov
>
> Signed-off-by: Yongseok Koh
Acked-by: Nelio Laranjeiro
> ---
> drivers/net/mlx5/mlx5_mr.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/net/mlx5/mlx5_mr.c b/drivers/net/mlx5/mlx5_mr.c
> index fdf7b3e88..39bbe2481 100644
> --- a/drivers/net/mlx5/mlx5_mr.c
> +++ b/drivers/net/mlx5/mlx5_mr.c
> @@ -265,9 +265,7 @@ mlx5_mr_new(struct rte_eth_dev *dev, struct rte_mempool
> *mp)
> ms = rte_mem_virt2memseg((void *)start, NULL);
> if (ms != NULL)
> start = RTE_ALIGN_FLOOR(start, ms->hugepage_sz);
> - ms = rte_mem_virt2memseg((void *)end, NULL);
> - if (ms != NULL)
> - end = RTE_ALIGN_CEIL(end, ms->hugepage_sz);
> + end = RTE_ALIGN_CEIL(end, ms->hugepage_sz);
>
> DRV_LOG(DEBUG,
> "port %u mempool %p using start=%p end=%p size=%zu for memory"
> --
> 2.11.0
>
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH v5 02/11] net/mlx4: fix ignored RSS hash types
On Mon, Apr 16, 2018 at 06:21:50PM +0200, Adrien Mazarguil wrote:
> When an unsupported hash type is part of a RSS configuration structure, it
> is silently ignored instead of triggering an error. This may lead
> applications to assume that such types are accepted, while they are in fact
> not part of the resulting flow rules.
>
> Fixes: 078b8b452e6b ("net/mlx4: add RSS flow rule action support")
> Cc: sta...@dpdk.org
>
> Signed-off-by: Adrien Mazarguil
> Cc: Shahaf Shuler
Acked-by: Nelio Laranjeiro
> ---
> drivers/net/mlx4/mlx4_flow.c | 12 +---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/mlx4/mlx4_flow.c b/drivers/net/mlx4/mlx4_flow.c
> index 2b4bf7094..67fd568bc 100644
> --- a/drivers/net/mlx4/mlx4_flow.c
> +++ b/drivers/net/mlx4/mlx4_flow.c
> @@ -706,6 +706,7 @@ mlx4_flow_prepare(struct priv *priv,
> const struct rte_flow_action_queue *queue;
> const struct rte_flow_action_rss *rss;
> const struct rte_eth_rss_conf *rss_conf;
> + uint64_t fields;
> unsigned int i;
>
> case RTE_FLOW_ACTION_TYPE_VOID:
> @@ -780,10 +781,15 @@ mlx4_flow_prepare(struct priv *priv,
> " of the context size";
> goto exit_action_not_supported;
> }
> + rte_errno = 0;
> + fields = mlx4_conv_rss_hf(priv, rss_conf->rss_hf);
> + if (fields == (uint64_t)-1 && rte_errno) {
> + msg = "unsupported RSS hash type requested";
> + goto exit_action_not_supported;
> + }
> flow->rss = mlx4_rss_get
> - (priv,
> - mlx4_conv_rss_hf(priv, rss_conf->rss_hf),
> - rss_conf->rss_key, rss->num, rss->queue);
> + (priv, fields, rss_conf->rss_key, rss->num,
> + rss->queue);
> if (!flow->rss) {
> msg = "either invalid parameters or not enough"
> " resources for additional multi-queue"
> --
> 2.11.0
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH v5 01/11] net/mlx4: fix RSS resource leak in case of error
On Mon, Apr 16, 2018 at 06:21:48PM +0200, Adrien Mazarguil wrote:
> When memory cannot be allocated for a flow rule, its RSS context reference
> is not dropped.
>
> Fixes: 078b8b452e6b ("net/mlx4: add RSS flow rule action support")
> Cc: sta...@dpdk.org
>
> Signed-off-by: Adrien Mazarguil
> Cc: Shahaf Shuler
Acked-by: Nelio Laranjeiro
> ---
> drivers/net/mlx4/mlx4_flow.c | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/mlx4/mlx4_flow.c b/drivers/net/mlx4/mlx4_flow.c
> index f3063ee8a..2b4bf7094 100644
> --- a/drivers/net/mlx4/mlx4_flow.c
> +++ b/drivers/net/mlx4/mlx4_flow.c
> @@ -820,11 +820,14 @@ mlx4_flow_prepare(struct priv *priv,
> },
> };
>
> - if (!mlx4_zmallocv(__func__, vec, RTE_DIM(vec)))
> + if (!mlx4_zmallocv(__func__, vec, RTE_DIM(vec))) {
> + if (temp.rss)
> + mlx4_rss_put(temp.rss);
> return rte_flow_error_set
> (error, -rte_errno,
>RTE_FLOW_ERROR_TYPE_UNSPECIFIED, NULL,
>"flow rule handle allocation failure");
> + }
> /* Most fields will be updated by second pass. */
> *flow = (struct rte_flow){
> .ibv_attr = temp.ibv_attr,
> --
> 2.11.0
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [dpdk-stable] [PATCH v5 09/11] ethdev: fix shallow copy of flow API RSS action
On Mon, Apr 16, 2018 at 06:22:03PM +0200, Adrien Mazarguil wrote:
> The rss_conf field is defined as a pointer to struct rte_eth_rss_conf.
>
> Even assuming it is permanently allocated and a pointer copy is safe,
> pointed data may change and not reflect an applied flow rule anymore.
>
> This patch aligns with testpmd by making a deep copy instead.
>
> Fixes: 18da437b5f63 ("ethdev: add flow rule copy function")
> Cc: sta...@dpdk.org
> Cc: Gaetan Rivet
>
> Signed-off-by: Adrien Mazarguil
> Cc: Thomas Monjalon
Acked-by: Nelio Laranjeiro
--
Nélio Laranjeiro
6WIND
Re: [dpdk-dev] [PATCH v3 04/14] net/mlx5: support Rx tunnel type identification
On Tue, Apr 17, 2018 at 04:53:15AM +, Xueming(Steven) Li wrote: > > > > -Original Message- > > From: Adrien Mazarguil > > Sent: Tuesday, April 17, 2018 12:03 AM > > To: Xueming(Steven) Li > > Cc: Nélio Laranjeiro ; Shahaf Shuler > > ; dev@dpdk.org; > > Olivier Matz > > Subject: Re: [PATCH v3 04/14] net/mlx5: support Rx tunnel type > > identification > > > > On Mon, Apr 16, 2018 at 03:27:37PM +, Xueming(Steven) Li wrote: > > > > > > > > > > -Original Message- > > > > From: Adrien Mazarguil > > > > Sent: Monday, April 16, 2018 9:48 PM > > > > To: Xueming(Steven) Li > > > > Cc: Nélio Laranjeiro ; Shahaf Shuler > > > > ; dev@dpdk.org; Olivier Matz > > > > > > > > Subject: Re: [PATCH v3 04/14] net/mlx5: support Rx tunnel type > > > > identification > > > > > > > > On Mon, Apr 16, 2018 at 01:32:49PM +0000, Xueming(Steven) Li wrote: > > > > > > > > > > > -Original Message- > > > > > > From: Adrien Mazarguil > > > > > > Sent: Monday, April 16, 2018 5:28 PM > > > > > > To: Xueming(Steven) Li > > > > > > Cc: Nélio Laranjeiro ; Shahaf Shuler > > > > > > ; dev@dpdk.org; Olivier Matz > > > > > > > > > > > > Subject: Re: [PATCH v3 04/14] net/mlx5: support Rx tunnel type > > > > > > identification > > > > > > > > > > > > On Mon, Apr 16, 2018 at 08:05:13AM +, Xueming(Steven) Li wrote: > > > > > > > > > > > > > > > > > > > > > > -Original Message- > > > > > > > > From: Nélio Laranjeiro > > > > > > > > Sent: Monday, April 16, 2018 3:29 PM > > > > > > > > To: Xueming(Steven) Li > > > > > > > > Cc: Shahaf Shuler ; dev@dpdk.org; > > > > > > > > Olivier Matz ; Adrien Mazarguil > > > > > > > > > > > > > > > > Subject: Re: [PATCH v3 04/14] net/mlx5: support Rx tunnel > > > > > > > > type identification > > > > > > > > > > > > > > > > On Sat, Apr 14, 2018 at 12:57:58PM +, Xueming(Steven) Li > > > > > > > > wrote: > > > > > > > > > +Adrien > > > > > > > > > > > > > > > > > > > -Original Message- > > > > > > > > > > From: Nélio Laranjeiro > > > > > > > > > > Sent: Friday, April 13, 2018 9:03 PM > > > > > > > > > > To: Xueming(Steven) Li > > > > > > > > > > Cc: Shahaf Shuler ; dev@dpdk.org; > > > > > > > > > > Olivier Matz > > > > > > > > > > Subject: Re: [PATCH v3 04/14] net/mlx5: support Rx > > > > > > > > > > tunnel type identification > > > > > > > > > > > > > > > > > > > > +Olivier, > > > > > > > > > > > > > > > > > > > > On Fri, Apr 13, 2018 at 07:20:13PM +0800, Xueming Li wrote: > > > > > > > > > > > This patch introduced tunnel type identification based on > > > > > > > > > > > flow rules. > > > > > > > > > > > If flows of multiple tunnel types built on same queue, > > > > > > > > > > > RTE_PTYPE_TUNNEL_MASK will be returned, user > > > > > > > > > > > application could use bits in flow mark as tunnel type > > > > > > > > > > > identifier. > > > > > > > > > > > > > > > > > > > > For an application it will mean the packet embed all > > > > > > > > > > tunnel types defined in DPDK, to make such thing you > > > > > > > > > > need a RTE_PTYPE_TUNNEL_UNKNOWN which does not exists > > > > > > > > > > currently. > > > > > > > > > > > > > > > > > > There was a RTE_PTYPE_TUNNEL_UNKNOWN definition, but > > > > > > > > > removed due to > > > > > > > > discussion. > > > > > > > > > So I think it good to add it in the patchse
Re: [dpdk-dev] [PATCH v2 07/15] net/mlx5: support tunnel RSS level
On Mon, Apr 16, 2018 at 10:06:06AM +, Xueming(Steven) Li wrote: > > > > -Original Message- > > From: Nélio Laranjeiro > > Sent: Monday, April 16, 2018 4:09 PM > > To: Xueming(Steven) Li > > Cc: Shahaf Shuler ; dev@dpdk.org > > Subject: Re: [PATCH v2 07/15] net/mlx5: support tunnel RSS level > > > > On Mon, Apr 16, 2018 at 07:46:08AM +, Xueming(Steven) Li wrote: > > >[...] > > > > > > > @@ -1386,6 +1386,8 @@ mlx5_ind_table_ibv_verify(struct > > > > > > > rte_eth_dev > > > > *dev) > > > > > > > * Number of queues. > > > > > > > * @param tunnel > > > > > > > * Tunnel type. > > > > > > > + * @param rss_level > > > > > > > + * RSS hash on tunnel level. > > > > > > > * > > > > > > > * @return > > > > > > > * The Verbs object initialised, NULL otherwise and rte_errno > > is > > > > set. > > > > > > > @@ -1394,13 +1396,17 @@ struct mlx5_hrxq * > > > > > > > mlx5_hrxq_new(struct rte_eth_dev *dev, > > > > > > > const uint8_t *rss_key, uint32_t rss_key_len, > > > > > > > uint64_t hash_fields, > > > > > > > - const uint16_t *queues, uint32_t queues_n, uint32_t > > > > tunnel) > > > > > > > + const uint16_t *queues, uint32_t queues_n, > > > > > > > + uint32_t tunnel, uint32_t rss_level) > > > > > > > > > > > > tunnel and rss_level seems to be redundant here. > > > > > > > > > > > > rss_level > 1 is equivalent to tunnel, there is no need to have > > both. > > > > > > > > > > There is a case of tunnel and outer rss(1). > > > > > > > > Why cannot it be handled by a regular Hash Rx queue, i.e. what is > > > > the benefit of creating a tunnel hash Rx queue to make the same job > > > > as a legacy one? > > > > > > Tunnel checksum, ptype and rss offloading demand a QP to be created by > > > DV api with tunnel offload flags. > > > > I was expecting such answer, such information should be present in the > > function documentation, can you add it? > > You mean https://dpdk.org/doc/guides/nics/overview.html? > "Inner L3 checksum" and "Inner L4 checksum" defined. > I added "Inner RSS" per your suggestion, The only thing missing is > "Innner packet type", make sense? No I mean adding in this function doxygen documentation the fact than tunnel is to have the checksum offload whereas the rss_level will be to enable the RSS in the inner. Thanks, -- Nélio Laranjeiro 6WIND
Re: [dpdk-dev] [PATCH v3 07/14] net/mlx5: support tunnel RSS level
On Sat, Apr 14, 2018 at 10:12:58AM +, Xueming(Steven) Li wrote: > Hi Nelio, >[...] > > > + if (!found) > > > + DRV_LOG(WARNING, > > > + "port %u rss hash function doesn't match " > > > + "pattern", dev->data->port_id); > > > > The hash function is toeplitz, xor, it is not applied on the pattern but > > used to compute an hash result using some information from the packet. > > This comment is totally wrong. > > Thanks, I'll replace "hash function" to "hash fields". > > > > > Another point, such log will trigger on an application using MLX5 PMD but > > not on MLX4 PMD and this specifically because on how the NIC using the > > MLX5 PMD are made internally (MLX4 can use a single Hash RX queue whereas > > MLX5 needs an Hash Rx queue per kind of protocol). > > The fact being it will have the exact same behavior I'll *strongly* > > suggest to remove such annoying warning. > > After some test on mlx5 current code, the behavior in previous code doesn't > seem to be consistent, not sure whether it same in mlx4 PMD: > - Pattern: eth/ipv4/tcp, RSS: UDP, creation success. > - Pattern: eth/ipv4,RSS: IPv6, creation failed. Seems there is a bug. > This patch support the 2nd case w/o hash, and warn upon the first case. > Take example of first case, a packet that matches the pattern must be TCP, > no reason to hash it as TCP, same to the 2nd case. They are totally > wrong configuration, but to be robust, warning is used here, and users > have to learn that NO hash result because HF configuration mismatch through > this warning message. > > Please note that below cases are valid and no warning: > - Pattern: eth/ipv4, RSS: UDP > - Pattern: eth/ipv4/udp, RSS: IPv4 This log will not raise for non IP protocols defined by the user, or it will raise when the user already expects it to not make RSS. It will more annoying than helping. Example: flow create 0 ingress eth ethertype is 0x0806 / end actions rss .... won't raise such log, whereas ARP is not an IP protocol and thus can be RSS'ed. flow create 0 ingress eth / ipv4 / end actions rss type ipv6... will raise the log, but it is obvious the user won't have RSS. Regards, -- Nélio Laranjeiro 6WIND
