date:20240218

RE: [PATCH] net/nfp: add support of UDP fragmentation offload

2024-02-18 Thread Morten Brørup

> From: Stephen Hemminger [mailto:step...@networkplumber.org]
> Sent: Saturday, 17 February 2024 19.11
> 
> On Sat, 17 Feb 2024 19:02:30 +0100
> Morten Brørup  wrote:
> 
> > Not formally... it follows the official DPDK Coding Style [1].
> >
> > [1]:
> https://doc.dpdk.org/guides/contributing/coding_style.html#general
> >
> > > Should be:
> > >
> > >   if ((ol_flags & RTE_MBUF_F_TX_TCP_SEG) == 0 &&
> > >   (ol_flags & RTE_MBUF_F_TX_UDP_SEG) == 0)
> > >   goto clean_txd;
> >
> > This indentation style is mentioned as an alternative in the guide.
> But the example in the guide also uses two tabs for a similar long
> comparison.
> >
> > Personally, I also prefer the style suggested by Stephen, so we might
> want to update the Coding Style. ;-)
> 
> 
> The two tabs is an Intel thing, and I prefer the kernel, line up the
> conditional style.

I prefer 4 space indentation, which is sufficient to notice the indentation. 8 
spaces seems overkill to me, and quickly makes the lines too long.
With the editor configured to show tab as 4 spaces, the kernel alignment style 
ends up with the same indentation for the condition and the code block:

if (a &&
b)
ctr++;

Whereas with the "tab as 4 spaces" editor configuration, the double indentation 
style clearly separates the continued condition from code block:

if (a &&
b)
ctr++;

On the other hand, complex conditions are easier readable when aligning 
logically instead of by a fixed number of tabs, e.g.:

if (a |
(b &
 (c ^ d)) |
(e ^ f) |
g)
ctr++;

Placing the operators at the beginning also makes the code more readable:

if (a
| (b
   & (c ^ d))
| (e ^ f)
| g)
ctr++;

I guess that coding styles are mostly a matter of taste.

I wonder if any research into coding styles has reached any conclusions or 
recommendations, beyond mixing coding styles is bad for readability.

> We really should have a style that can be describe by clang format.
> Other projects like VPP have a target that reformats the code and uses
> one of the clang format templates.

Automated code formatting seems like a good idea.

Re: [PATCH v2 0/6] use rte atomic thread fence

2024-02-18 Thread Thomas Monjalon

> > Replace use of __atomic_thread_fence with rte_atomic_thread_fence.
> > 
> > Notes:
> > 
> >   The rest of lib/lpm will be converted to rte_atomic in a separate
> >   series (to be submitted soon).
> > 
> >   There are existing checkpatches checks that catch use of both
> >   __atomic_thread_fence and __rte_atomic_thread_fence in new
> >   submissions.
> > 
> > v2:
> > * change series to use rte_atomic_thread_fence instead of
> >   __rte_atomic_thread_fence (internal)
> > * also change __atomic_thread_fence in lib/lpm
> > 
> > Tyler Retzlaff (6):
> >   distributor: use rte atomic thread fence
> >   eal: use rte atomic thread fence
> >   hash: use rte atomic thread fence
> >   ring: use rte atomic thread fence
> >   stack: use rte atomic thread fence
> >   lpm: use rte atomic thread fence
> 
> Series-acked-by: Chengwen Feng 

Acked-by: Thomas Monjalon 

Squashed and applied, thanks.

Re: [PATCH] eal: provide rte attribute macro for GCC attribute

2024-02-18 Thread Thomas Monjalon

15/02/2024 23:20, Tyler Retzlaff:
> Provide a new macro __rte_attribute(a) that when directly used
> compiles to empty for MSVC and to __attribute__(a) when using GCC/LLVM.
> 
> Replace direct use of __attribute__ in __rte_xxx macros where there is
> existing empty expansion of the macro for MSVC allowing removal of
> repeated #ifdef RTE_TOOLCHAIN_MSVC per macro to expand empty.

I'm not sure it makes sense.
I prefer seeing clearly what is empty with MSVC.

Re: [PATCH v2 0/4] more replacement of zero length array

2024-02-18 Thread Dodji Seketeli

Hello,

David Marchand  writes:

> Dodji confirmed the issue in libabigail and prepared a fix.

Yes, that is correct. Sorry for the inconvenience.

The patch fixing this issue in libabigail has been applied to the
mainline and is
https://sourceware.org/git/?p=libabigail.git;a=commit;h=3cc1c3423c89c2cfd9d451ab99b71f3a74b35127.

> DPDK still needs a suppression rule (like the one proposed above) if
> we want to merge this change before the libabigail fix makes it to all
> distribs.

We can discuss further if you need a "quick" 2.4.1 release as a vehicle
to consume the fix or if you can wait for a 2.5 one that might come later.

I agree however that in the mean time, a temporary suppression
specification might be warranted.

[...]

Cheers,

-- 
Dodji

Re: [PATCH v4 01/18] mbuf: deprecate GCC marker in rte mbuf struct

2024-02-18 Thread Thomas Monjalon

15/02/2024 07:21, Tyler Retzlaff:
> Provide a macro that allows conditional expansion of RTE_MARKER fields
> to empty to allow rte_mbuf to be used with MSVC. It is proposed that
> we announce the fields to be __rte_deprecated (currently disabled).
> 
> Introduce C11 anonymous unions to permit aliasing of well-known
> offsets by name into the rte_mbuf structure by a *new* name and to
> provide padding for cache alignment.
> 
> Signed-off-by: Tyler Retzlaff 
> ---
>  doc/guides/rel_notes/deprecation.rst |  20 ++
>  lib/eal/include/rte_common.h |   6 +
>  lib/mbuf/rte_mbuf_core.h | 375 
> +++
>  3 files changed, 233 insertions(+), 168 deletions(-)
> 
> diff --git a/doc/guides/rel_notes/deprecation.rst 
> b/doc/guides/rel_notes/deprecation.rst
> index 10630ba..8594255 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -17,6 +17,26 @@ Other API and ABI deprecation notices are to be posted 
> below.
>  Deprecation Notices
>  ---
>  
> +* mbuf: Named zero sized fields of type ``RTE_MARKER`` and ``RTE_MARKER64``
> +  will be removed from ``struct rte_mbuf`` and replaced with new fields
> +  in anonymous unions.
> +
> +  The names of the fields impacted are:
> +
> +old name  new name
> +
> +  ``cacheline0````mbuf_cachelin0``
> +  ``rearm_data````mbuf_rearm_data``
> +  ``rx_descriptor_fields1`` ``mbuf_rx_descriptor_fields1``
> +  ``cacheline1````mbuf_cachelin1``
> +
> +  Contributions to DPDK should immediately start using the new names,
> +  applications should adapt to new names as soon as possible as the
> +  old names will be removed in a future DPDK release.
> +
> +  Note: types of the new names are not API compatible with the old and
> +  some code conversion is required to maintain correct behavior.
> +
>  * build: The ``enable_kmods`` option is deprecated and will be removed in a 
> future release.
>Setting/clearing the option has no impact on the build.
>Instead, kernel modules will be always built for OS's where out-of-tree 
> kernel modules
> diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
> index d7d6390..af73f67 100644
> --- a/lib/eal/include/rte_common.h
> +++ b/lib/eal/include/rte_common.h
> @@ -582,6 +582,12 @@ static void __attribute__((destructor(RTE_PRIO(prio)), 
> used)) func(void)
>  /** Marker for 8B alignment in a structure. */
>  __extension__ typedef uint64_t RTE_MARKER64[0];
>  
> +#define __rte_marker(type, name) type name /* __rte_deprecated */
> +
> +#else
> +
> +#define __rte_marker(type, name)
> +
>  #endif
>  
>  /*** Macros for calculating min and max **/
> diff --git a/lib/mbuf/rte_mbuf_core.h b/lib/mbuf/rte_mbuf_core.h
> index 5688683..9e9590b 100644
> --- a/lib/mbuf/rte_mbuf_core.h
> +++ b/lib/mbuf/rte_mbuf_core.h
> @@ -16,7 +16,10 @@
>   * New fields and flags should fit in the "dynamic space".
>   */
>  
> +#include 
> +#include 
>  #include 
> +#include 
>  
>  #include 
>  #include 
> @@ -464,204 +467,240 @@ enum {
>   * The generic rte_mbuf, containing a packet mbuf.
>   */
>  struct rte_mbuf {
> - RTE_MARKER cacheline0;
> -
> - void *buf_addr;   /**< Virtual address of segment buffer. */
> + __rte_marker(RTE_MARKER, cacheline0);

You don't need to keep the first argument.
This would be simpler:
__rte_marker(cacheline0);
You just need to create 2 functions: __rte_marker and __rte_marker64.

You should replace all occurrences of RTE_MARKER in DPDK in one patch,
and mark RTE_MARKER as deprecated (use #pragma GCC poison)

> + union {
> + char mbuf_cacheline0[RTE_CACHE_LINE_MIN_SIZE];
> + __extension__
> + struct {
> + void *buf_addr;   /**< Virtual address of 
> segment buffer. 

I think it is ugly.

Changing mbuf API is a serious issue.

RE: [PATCH] eal: provide rte attribute macro for GCC attribute

2024-02-18 Thread Morten Brørup

> From: Thomas Monjalon [mailto:tho...@monjalon.net]
> Sent: Sunday, 18 February 2024 13.24
> 
> 15/02/2024 23:20, Tyler Retzlaff:
> > Provide a new macro __rte_attribute(a) that when directly used
> > compiles to empty for MSVC and to __attribute__(a) when using
> GCC/LLVM.
> >
> > Replace direct use of __attribute__ in __rte_xxx macros where there
> is
> > existing empty expansion of the macro for MSVC allowing removal of
> > repeated #ifdef RTE_TOOLCHAIN_MSVC per macro to expand empty.
> 
> I'm not sure it makes sense.
> I prefer seeing clearly what is empty with MSVC.

This topic has previously been discussed in another context - adding external 
libraries [1].

Like you do here, I generally preferred #ifdefs in the code, but the majority 
preferred stubs "promoting improved code readability".

I might argue that Tyler is following that guidance here; and perhaps the 
decision should be reconsidered, now that we have a real-life example of how it 
affects code readability. ;-)

[1]: https://inbox.dpdk.org/dev/20240109141009.497807-1-jer...@marvell.com/

RE: [PATCH v4 01/18] mbuf: deprecate GCC marker in rte mbuf struct

2024-02-18 Thread Morten Brørup

> From: Thomas Monjalon [mailto:tho...@monjalon.net]
> Sent: Sunday, 18 February 2024 13.40
> 
> 15/02/2024 07:21, Tyler Retzlaff:
> > Provide a macro that allows conditional expansion of RTE_MARKER
> fields
> > to empty to allow rte_mbuf to be used with MSVC. It is proposed that
> > we announce the fields to be __rte_deprecated (currently disabled).
> >
> > Introduce C11 anonymous unions to permit aliasing of well-known
> > offsets by name into the rte_mbuf structure by a *new* name and to
> > provide padding for cache alignment.
> >
> > Signed-off-by: Tyler Retzlaff 
> > ---
> >  doc/guides/rel_notes/deprecation.rst |  20 ++
> >  lib/eal/include/rte_common.h |   6 +
> >  lib/mbuf/rte_mbuf_core.h | 375 +++--
> --
> >  3 files changed, 233 insertions(+), 168 deletions(-)
> >
> > diff --git a/doc/guides/rel_notes/deprecation.rst
> b/doc/guides/rel_notes/deprecation.rst
> > index 10630ba..8594255 100644
> > --- a/doc/guides/rel_notes/deprecation.rst
> > +++ b/doc/guides/rel_notes/deprecation.rst
> > @@ -17,6 +17,26 @@ Other API and ABI deprecation notices are to be
> posted below.
> >  Deprecation Notices
> >  ---
> >
> > +* mbuf: Named zero sized fields of type ``RTE_MARKER`` and
> ``RTE_MARKER64``
> > +  will be removed from ``struct rte_mbuf`` and replaced with new
> fields
> > +  in anonymous unions.
> > +
> > +  The names of the fields impacted are:
> > +
> > +old name  new name
> > +
> > +  ``cacheline0````mbuf_cachelin0``
> > +  ``rearm_data````mbuf_rearm_data``
> > +  ``rx_descriptor_fields1`` ``mbuf_rx_descriptor_fields1``
> > +  ``cacheline1````mbuf_cachelin1``
> > +
> > +  Contributions to DPDK should immediately start using the new
> names,
> > +  applications should adapt to new names as soon as possible as the
> > +  old names will be removed in a future DPDK release.
> > +
> > +  Note: types of the new names are not API compatible with the old
> and
> > +  some code conversion is required to maintain correct behavior.
> > +
> >  * build: The ``enable_kmods`` option is deprecated and will be
> removed in a future release.
> >Setting/clearing the option has no impact on the build.
> >Instead, kernel modules will be always built for OS's where out-
> of-tree kernel modules
> > diff --git a/lib/eal/include/rte_common.h
> b/lib/eal/include/rte_common.h
> > index d7d6390..af73f67 100644
> > --- a/lib/eal/include/rte_common.h
> > +++ b/lib/eal/include/rte_common.h
> > @@ -582,6 +582,12 @@ static void
> __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
> >  /** Marker for 8B alignment in a structure. */
> >  __extension__ typedef uint64_t RTE_MARKER64[0];
> >
> > +#define __rte_marker(type, name) type name /* __rte_deprecated */
> > +
> > +#else
> > +
> > +#define __rte_marker(type, name)
> > +
> >  #endif
> >
> >  /*** Macros for calculating min and max **/
> > diff --git a/lib/mbuf/rte_mbuf_core.h b/lib/mbuf/rte_mbuf_core.h
> > index 5688683..9e9590b 100644
> > --- a/lib/mbuf/rte_mbuf_core.h
> > +++ b/lib/mbuf/rte_mbuf_core.h
> > @@ -16,7 +16,10 @@
> >   * New fields and flags should fit in the "dynamic space".
> >   */
> >
> > +#include 
> > +#include 
> >  #include 
> > +#include 
> >
> >  #include 
> >  #include 
> > @@ -464,204 +467,240 @@ enum {
> >   * The generic rte_mbuf, containing a packet mbuf.
> >   */
> >  struct rte_mbuf {
> > -   RTE_MARKER cacheline0;
> > -
> > -   void *buf_addr;   /**< Virtual address of segment buffer.
> */
> > +   __rte_marker(RTE_MARKER, cacheline0);
> 
> You don't need to keep the first argument.
> This would be simpler:
>   __rte_marker(cacheline0);
> You just need to create 2 functions: __rte_marker and __rte_marker64.
> 
> You should replace all occurrences of RTE_MARKER in DPDK in one patch,
> and mark RTE_MARKER as deprecated (use #pragma GCC poison)

I like this suggestion.
However, some applications might use RTE_MARKER in their own structures.
Wouldn't it be considered API breakage to mark RTE_MARKER as deprecated?

> 
> > +   union {
> > +   char mbuf_cacheline0[RTE_CACHE_LINE_MIN_SIZE];
> > +   __extension__
> > +   struct {
> > +   void *buf_addr;   /**< Virtual address of
> segment buffer.
> 
> I think it is ugly.
> 
> Changing mbuf API is a serious issue.

Re: [PATCH 00/15] use GCC/MSVC compatible __VA_ARGS__

2024-02-18 Thread Thomas Monjalon

12/02/2024 22:49, Tyler Retzlaff:
> MSVC does not support GCC args... forwarding of args replace
> with ... and __VA_ARGS__ when forwarding.  Both forms of
> forwarding are a compiler extension but the latter is supported
> by both MSVC and GCC.

You use the non-standard ", ## __VA_ARGS__"
which would not compile in pedantic modes,
and possibly with other compilers.

I remember a private chat where I recommended you to use this:

lib/eal/include/rte_common.h
/**
 * ISO C helpers to modify format strings using variadic macros.
 * This is a replacement for the ", ## __VA_ARGS__" GNU extension.
 * An empty %s argument is appended to avoid a dangling comma.
 */
#define RTE_FMT(fmt, ...) fmt "%.0s", __VA_ARGS__ ""
#define RTE_FMT_HEAD(fmt, ...) fmt
#define RTE_FMT_TAIL(fmt, ...) __VA_ARGS__

Why not using it?

Re: [PATCH v2 1/2] build: fix list_dir_globs failure in MSYS2

2024-02-18 Thread Thomas Monjalon

Any update?
Would you like to provide a v3?


24/10/2023 18:08, Ric Li:
> 
> On 2023/10/11 23:34, Bruce Richardson wrote:
> > On Wed, Oct 11, 2023 at 05:27:22PM +0200, Thomas Monjalon wrote:
> >> 20/09/2023 16:18, Ric Li:
> >>> When running 'meson setup' on Windows with MSYS2,
> >>> "list-dir-globs.py * failed with status 1".
> >>
> >> We don't know why it is failing?
> >> What about other usages of list_dir_globs in drivers and lib?
> 
> Looks lile MSYS2 shell expands this wildcard automatically
> before passing it to the child process.
> 
> I print the args in list-dir-globs.py and found that args are
> the expanded dir names, so the len(sys.argv) is larger than 2,
> which makes this script fail. The '*/*' arg in drivers/meson.build
> works well just as expected, and no '*' used in lib.
> 
> This is from MSYS2 documentation:
> "Windows programs parse the command line themselves, it isn't parsed for them 
> by
> the calling process, as on Linux. This means that if wildcards (glob 
> patterns) are
> to be accepted by the program, it has to be able to expand them somehow. 
> MinGW-w64
> supplies the correct start-up code, so it happens automatically, in a manner
> compatible with MSVC-compiled programs. If undesirable, the behavior can be
> disabled at program build."
> 
> I think this fix is not needed if we can find a way to disable the 
> auto-expanding
> behaviour of the MSYS2 program. I've tried the runtime way by setting
> "MSYS=noglob" envvar but not working here...
> 
> >>
> >>> Avoid using globbing to get components for app build
> >>> since they are already listed in the meson file.
> >>
> >> I don't understand the logic.
> >>
> >>> +disable_apps = ',' + get_option('disable_apps')
> >>> +disable_apps = run_command(list_dir_globs, disable_apps, check: 
> >>> true).stdout().split()
> >>
> >> This could fail.>>
> >>> +
> >>> +enable_apps = ',' + get_option('enable_apps')
> >>> +enable_apps = run_command(list_dir_globs, enable_apps, check: 
> >>> true).stdout().split()
> >>> +if enable_apps.length() == 0
> >>> +enable_apps = apps
> >>> +endif
> >>
> >> If nothing is enabled, we enable all?
> >>
> > Yes, if the enable_apps list is empty we should enable everything.
> > However, on reviewing the v2, I missed the fact that this patch is
> > removing the expansion of the disable_apps value.> 
> > Given your comment, this check can probably also be improved by checking
> > the get_option('enable_apps') length, rather than the expanded version.
> > 
> > /Bruce
>

Re: [PATCH v2 01/16] eal: verify strdup return value

2024-02-18 Thread Thomas Monjalon

21/11/2023 04:44, lihuisong (C):
> 在 2023/11/10 18:01, Chengwen Feng 写道:
> > --- a/lib/eal/linux/eal_dev.c
> > +++ b/lib/eal/linux/eal_dev.c
> > @@ -181,7 +181,10 @@ dev_uev_parse(const char *buf, struct rte_dev_event 
> > *event, int length)
> > buf += 14;
> > i += 14;
> > strlcpy(pci_slot_name, buf, sizeof(subsystem));
> > +   free(event->devname);
> It seems that above free for devname is unnecessary.

You didn't reply to this comment, so I will drop this free call.

> > event->devname = strdup(pci_slot_name);
> > +   if (event->devname == NULL)
> > +   return -1;

Re: [PATCH] eal: provide rte attribute macro for GCC attribute

2024-02-18 Thread Mattias Rönnblom


On 2024-02-18 13:24, Thomas Monjalon wrote:

15/02/2024 23:20, Tyler Retzlaff:

Provide a new macro __rte_attribute(a) that when directly used
compiles to empty for MSVC and to __attribute__(a) when using GCC/LLVM.

Replace direct use of __attribute__ in __rte_xxx macros where there is
existing empty expansion of the macro for MSVC allowing removal of
repeated #ifdef RTE_TOOLCHAIN_MSVC per macro to expand empty.


I'm not sure it makes sense.
I prefer seeing clearly what is empty with MSVC.




Anything __rte_attribute() is empty on MSVC. You could rename it 
__rte_attribute_ignored_by_msvc() for clarity.


One could note that on the ignore list are things like "may_alias" and 
"packed", so whatever comes out of a MSVC build should not be expected 
to actually work.


Unless I'm missing something, for all the attributes that will have 
MSVC-propriety equivalent, the usage pattern would have to change, since 
the syntax is different in incompatible ways.


Wouldn't it be better to ask the MSVC team to add support GCC 
attributes? ICC and LLVM managed, so why not Microsoft. Then you would 
solve this issue for all Open Source projects, not only DPDK.

Re: [PATCH v4 01/18] mbuf: deprecate GCC marker in rte mbuf struct

2024-02-18 Thread Thomas Monjalon

18/02/2024 14:07, Morten Brørup:
> From: Thomas Monjalon [mailto:tho...@monjalon.net]
> > 15/02/2024 07:21, Tyler Retzlaff:
> > > --- a/lib/eal/include/rte_common.h
> > > +++ b/lib/eal/include/rte_common.h
> > > @@ -582,6 +582,12 @@ static void
> > __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
> > >  /** Marker for 8B alignment in a structure. */
> > >  __extension__ typedef uint64_t RTE_MARKER64[0];
> > >
> > > +#define __rte_marker(type, name) type name /* __rte_deprecated */
> > > +
> > > +#else
> > > +
> > > +#define __rte_marker(type, name)
> > > +
> > >  #endif
> > >
> > >  /*** Macros for calculating min and max **/
> > > diff --git a/lib/mbuf/rte_mbuf_core.h b/lib/mbuf/rte_mbuf_core.h
> > > index 5688683..9e9590b 100644
> > > --- a/lib/mbuf/rte_mbuf_core.h
> > > +++ b/lib/mbuf/rte_mbuf_core.h
> > > @@ -16,7 +16,10 @@
> > >   * New fields and flags should fit in the "dynamic space".
> > >   */
> > >
> > > +#include 
> > > +#include 
> > >  #include 
> > > +#include 
> > >
> > >  #include 
> > >  #include 
> > > @@ -464,204 +467,240 @@ enum {
> > >   * The generic rte_mbuf, containing a packet mbuf.
> > >   */
> > >  struct rte_mbuf {
> > > - RTE_MARKER cacheline0;
> > > -
> > > - void *buf_addr;   /**< Virtual address of segment buffer.
> > */
> > > + __rte_marker(RTE_MARKER, cacheline0);
> > 
> > You don't need to keep the first argument.
> > This would be simpler:
> > __rte_marker(cacheline0);
> > You just need to create 2 functions: __rte_marker and __rte_marker64.
> > 
> > You should replace all occurrences of RTE_MARKER in DPDK in one patch,
> > and mark RTE_MARKER as deprecated (use #pragma GCC poison)
> 
> I like this suggestion.
> However, some applications might use RTE_MARKER in their own structures.
> Wouldn't it be considered API breakage to mark RTE_MARKER as deprecated?

Yes it is an API breakage.
Do we prefer waiting 24.11 for poisoning this macro?

Re: [PATCH] eal: provide rte attribute macro for GCC attribute

2024-02-18 Thread Thomas Monjalon

18/02/2024 15:51, Mattias Rönnblom:
> On 2024-02-18 13:24, Thomas Monjalon wrote:
> > 15/02/2024 23:20, Tyler Retzlaff:
> >> Provide a new macro __rte_attribute(a) that when directly used
> >> compiles to empty for MSVC and to __attribute__(a) when using GCC/LLVM.
> >>
> >> Replace direct use of __attribute__ in __rte_xxx macros where there is
> >> existing empty expansion of the macro for MSVC allowing removal of
> >> repeated #ifdef RTE_TOOLCHAIN_MSVC per macro to expand empty.
> > 
> > I'm not sure it makes sense.
> > I prefer seeing clearly what is empty with MSVC.
> 
> Anything __rte_attribute() is empty on MSVC. You could rename it 
> __rte_attribute_ignored_by_msvc() for clarity.

Yes it would bring more clarity.
But I still prefer #ifdef which may work with more compilers.

> One could note that on the ignore list are things like "may_alias" and 
> "packed", so whatever comes out of a MSVC build should not be expected 
> to actually work.
> 
> Unless I'm missing something, for all the attributes that will have 
> MSVC-propriety equivalent, the usage pattern would have to change, since 
> the syntax is different in incompatible ways.
> 
> Wouldn't it be better to ask the MSVC team to add support GCC 
> attributes? ICC and LLVM managed, so why not Microsoft. Then you would 
> solve this issue for all Open Source projects, not only DPDK.

We can expect MSVC to improve.
That's another reason why I prefer to keep #ifdef to keep track easily.

Re: [PATCH] eal: provide rte attribute macro for GCC attribute

2024-02-18 Thread Thomas Monjalon

18/02/2024 13:53, Morten Brørup:
> > From: Thomas Monjalon [mailto:tho...@monjalon.net]
> > Sent: Sunday, 18 February 2024 13.24
> > 
> > 15/02/2024 23:20, Tyler Retzlaff:
> > > Provide a new macro __rte_attribute(a) that when directly used
> > > compiles to empty for MSVC and to __attribute__(a) when using
> > GCC/LLVM.
> > >
> > > Replace direct use of __attribute__ in __rte_xxx macros where there
> > is
> > > existing empty expansion of the macro for MSVC allowing removal of
> > > repeated #ifdef RTE_TOOLCHAIN_MSVC per macro to expand empty.
> > 
> > I'm not sure it makes sense.
> > I prefer seeing clearly what is empty with MSVC.
> 
> This topic has previously been discussed in another context - adding external 
> libraries [1].
> 
> Like you do here, I generally preferred #ifdefs in the code, but the majority 
> preferred stubs "promoting improved code readability".

Stubs may make sense in many places,
but here we are talking about rte_common.h
where we abstract differences between arch and compilers,
so it is the right place to be explicit with compilers support.

> I might argue that Tyler is following that guidance here; and perhaps the 
> decision should be reconsidered, now that we have a real-life example of how 
> it affects code readability. ;-)
> 
> [1]: https://inbox.dpdk.org/dev/20240109141009.497807-1-jer...@marvell.com/

Re: [PATCH v2 00/16] verify strdup return value

2024-02-18 Thread Thomas Monjalon

10/11/2023 11:01, Chengwen Feng:
> This patchset mainly fix the return value of strdup not checked which
> may lead to segment fault. It also include two commits which fix memory
> leak of strdup.
> 
> Chengwen Feng (16):
>   eal: verify strdup return value
>   bus/dpaa: verify strdup return value
>   bus/fslmc: verify strdup return value
>   bus/vdev: verify strdup return value
>   dma/idxd: verify strdup return value
>   event/cnxk: verify strdup return value
>   net/failsafe: fix memory leak when parse args
>   net/nfp: verify strdup return value
>   app/dumpcap: verify strdup return value
>   app/pdump: verify strdup return value
>   app/test: verify strdup return value
>   app/test-crypto-perf: verify strdup return value
>   app/test-dma-perf: verify strdup return value
>   app/testpmd: verify strdup return value
>   examples/qos_sched: fix memory leak when parse args
>   examples/vhost: verify strdup return value

Applied, with small change in EAL as suggested by Huisong, thanks.

Re: [PATCH v2 1/8] pipeline: fix calloc parameters

2024-02-18 Thread Thomas Monjalon

24/01/2024 19:53, Ferruh Yigit:
> gcc [1] generates warning [2] about calloc usage, because calloc
> parameter order is wrong, fixing it by replacing parameters.

Series applied, thanks.

RE: [PATCH v4 01/18] mbuf: deprecate GCC marker in rte mbuf struct

2024-02-18 Thread Morten Brørup

> From: Thomas Monjalon [mailto:tho...@monjalon.net]
> Sent: Sunday, 18 February 2024 16.22
> 
> 18/02/2024 14:07, Morten Brørup:
> > From: Thomas Monjalon [mailto:tho...@monjalon.net]
> > > 15/02/2024 07:21, Tyler Retzlaff:
> > > > --- a/lib/eal/include/rte_common.h
> > > > +++ b/lib/eal/include/rte_common.h
> > > > @@ -582,6 +582,12 @@ static void
> > > __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
> > > >  /** Marker for 8B alignment in a structure. */
> > > >  __extension__ typedef uint64_t RTE_MARKER64[0];
> > > >
> > > > +#define __rte_marker(type, name) type name /* __rte_deprecated
> */
> > > > +
> > > > +#else
> > > > +
> > > > +#define __rte_marker(type, name)
> > > > +
> > > >  #endif
> > > >
> > > >  /*** Macros for calculating min and max **/
> > > > diff --git a/lib/mbuf/rte_mbuf_core.h b/lib/mbuf/rte_mbuf_core.h
> > > > index 5688683..9e9590b 100644
> > > > --- a/lib/mbuf/rte_mbuf_core.h
> > > > +++ b/lib/mbuf/rte_mbuf_core.h
> > > > @@ -16,7 +16,10 @@
> > > >   * New fields and flags should fit in the "dynamic space".
> > > >   */
> > > >
> > > > +#include 
> > > > +#include 
> > > >  #include 
> > > > +#include 
> > > >
> > > >  #include 
> > > >  #include 
> > > > @@ -464,204 +467,240 @@ enum {
> > > >   * The generic rte_mbuf, containing a packet mbuf.
> > > >   */
> > > >  struct rte_mbuf {
> > > > -   RTE_MARKER cacheline0;
> > > > -
> > > > -   void *buf_addr;   /**< Virtual address of segment
> buffer.
> > > */
> > > > +   __rte_marker(RTE_MARKER, cacheline0);
> > >
> > > You don't need to keep the first argument.
> > > This would be simpler:
> > >   __rte_marker(cacheline0);
> > > You just need to create 2 functions: __rte_marker and
> __rte_marker64.
> > >
> > > You should replace all occurrences of RTE_MARKER in DPDK in one
> patch,
> > > and mark RTE_MARKER as deprecated (use #pragma GCC poison)
> >
> > I like this suggestion.
> > However, some applications might use RTE_MARKER in their own
> structures.
> > Wouldn't it be considered API breakage to mark RTE_MARKER as
> deprecated?
> 
> Yes it is an API breakage.
> Do we prefer waiting 24.11 for poisoning this macro?

Personally, I generally don't mind API breakages, assuming that they are 
visible and thus don't silently introduce bugs.
The distro people might have a different opinion.

RE: [PATCH] eal: provide rte attribute macro for GCC attribute

2024-02-18 Thread Morten Brørup

> From: Thomas Monjalon [mailto:tho...@monjalon.net]
> Sent: Sunday, 18 February 2024 16.35
> 
> 18/02/2024 13:53, Morten Brørup:
> > > From: Thomas Monjalon [mailto:tho...@monjalon.net]
> > > Sent: Sunday, 18 February 2024 13.24
> > >
> > > 15/02/2024 23:20, Tyler Retzlaff:
> > > > Provide a new macro __rte_attribute(a) that when directly used
> > > > compiles to empty for MSVC and to __attribute__(a) when using
> > > GCC/LLVM.
> > > >
> > > > Replace direct use of __attribute__ in __rte_xxx macros where
> there
> > > is
> > > > existing empty expansion of the macro for MSVC allowing removal
> of
> > > > repeated #ifdef RTE_TOOLCHAIN_MSVC per macro to expand empty.
> > >
> > > I'm not sure it makes sense.
> > > I prefer seeing clearly what is empty with MSVC.
> >
> > This topic has previously been discussed in another context - adding
> external libraries [1].
> >
> > Like you do here, I generally preferred #ifdefs in the code, but the
> majority preferred stubs "promoting improved code readability".
> 
> Stubs may make sense in many places,
> but here we are talking about rte_common.h
> where we abstract differences between arch and compilers,
> so it is the right place to be explicit with compilers support.

Very strong point. I'm convinced.

Should the new rte_attribute() macro still be introduced for other uses of 
__attribute__(), e.g. the somewhat exotic attributes in 
eal/include/rte_lock_annotations.h?

The not-so-exotic attributes could have new macros added, e.g. __rte_const and 
__rte_pure.

> 
> > I might argue that Tyler is following that guidance here; and perhaps
> the decision should be reconsidered, now that we have a real-life
> example of how it affects code readability. ;-)
> >
> > [1]: https://inbox.dpdk.org/dev/20240109141009.497807-1-
> jer...@marvell.com/
> 
>

Re: [PATCH 1/1] maintainers: update RISC-V maintainer email

2024-02-18 Thread Thomas Monjalon

13/02/2024 19:14, Stanislaw Kardach:
> Change my email to a personal one.
> 
> Signed-off-by: Stanislaw Kardach 
> ---
>  RISC-V
> -M: Stanislaw Kardach 
> +M: Stanislaw Kardach 

Applied and updated the .mailmap file at the same time.

Re: [PATCH] eal: provide rte attribute macro for GCC attribute

2024-02-18 Thread Thomas Monjalon

18/02/2024 17:38, Morten Brørup:
> > From: Thomas Monjalon [mailto:tho...@monjalon.net]
> > Sent: Sunday, 18 February 2024 16.35
> > 
> > 18/02/2024 13:53, Morten Brørup:
> > > > From: Thomas Monjalon [mailto:tho...@monjalon.net]
> > > > Sent: Sunday, 18 February 2024 13.24
> > > >
> > > > 15/02/2024 23:20, Tyler Retzlaff:
> > > > > Provide a new macro __rte_attribute(a) that when directly used
> > > > > compiles to empty for MSVC and to __attribute__(a) when using
> > > > GCC/LLVM.
> > > > >
> > > > > Replace direct use of __attribute__ in __rte_xxx macros where
> > there
> > > > is
> > > > > existing empty expansion of the macro for MSVC allowing removal
> > of
> > > > > repeated #ifdef RTE_TOOLCHAIN_MSVC per macro to expand empty.
> > > >
> > > > I'm not sure it makes sense.
> > > > I prefer seeing clearly what is empty with MSVC.
> > >
> > > This topic has previously been discussed in another context - adding
> > external libraries [1].
> > >
> > > Like you do here, I generally preferred #ifdefs in the code, but the
> > majority preferred stubs "promoting improved code readability".
> > 
> > Stubs may make sense in many places,
> > but here we are talking about rte_common.h
> > where we abstract differences between arch and compilers,
> > so it is the right place to be explicit with compilers support.
> 
> Very strong point. I'm convinced.
> 
> Should the new rte_attribute() macro still be introduced for other uses of 
> __attribute__(), e.g. the somewhat exotic attributes in 
> eal/include/rte_lock_annotations.h?

They are all wrapped in a meaningful macro.

> The not-so-exotic attributes could have new macros added, e.g. __rte_const 
> and __rte_pure.

Yes we need wrappers for all attributes.


> > > I might argue that Tyler is following that guidance here; and perhaps
> > the decision should be reconsidered, now that we have a real-life
> > example of how it affects code readability. ;-)
> > >
> > > [1]: https://inbox.dpdk.org/dev/20240109141009.497807-1-
> > jer...@marvell.com/

Re: [PATCH] maintainers: add myself to usertools

2024-02-18 Thread Thomas Monjalon

13/02/2024 10:58, Robin Jarry:
> in the past months, I saw that the usertools scripts didn't receive much
> reviews. Unless somebody is against it, I'd like to become maintainer
> for the usertools folder.
> 
> Signed-off-by: Robin Jarry 

Acked-by: Bruce Richardson 
Acked-by: Thomas Monjalon 
Acked-by: Ciara Power 
Acked-by: Ferruh Yigit 

Applied, thanks.

Re: [PATCH] build: fix linker warnings about undefined symbols

2024-02-18 Thread Thomas Monjalon

10/01/2024 17:58, Tyler Retzlaff:
> On Wed, Jan 10, 2024 at 03:01:03PM +, Bruce Richardson wrote:
> > The default behaviour of "ld.lld" has changed, so it now prints out
> > warnings about entries in the version.map file which don't exist in
> > the current build. Since we use our version.map file simply to filter
> > out the functions we don't want made public, we include in it all
> > functions across all OS's and builds that we want public if present.
> > This causes these ld warnings to be emitted, e.g. on BSD, which is
> > missing functionality found on Linux. For example:
> > 
> > * hpet functions in EAL
> > * regexdev enqueue and dequeue burst
> > * eventdev event_timer functions
> > 
> > Easiest solution, without major rework of how we use our version.map
> > files, and without dynamically generating them per-build, is to pass
> > the --undefined-version flag to the linker, to restore the old
> > behaviour.
> > 
> > Signed-off-by: Bruce Richardson 
> 
> Acked-by: Tyler Retzlaff 

+Cc stable

Applied as easy solution for this linker.

The discussion about Windows linker and use of macros
deserve to continue, maybe in a new dedicated thread?

> i don't know if has ever been discussed but a way to achieve a similar
> outcome would be to introduce a visibility macro allowing the data and
> function symbols to be explicitly made visible while making the build
> default hidden.
> 
> https://gcc.gnu.org/wiki/Visibility
> 
> on windows symbols are hidden by default (opposite of gnu toolchain) and
> allowing the symbols that are intended to be public be annotated in code
> with a macro instead of a separate map/def file would arguably appear
> more consistent.  additionally, the current process of converting a map
> file to a def file for windows has limitations with respect to things
> like marking the type of symbol being exported.

Re: [PATCH] meson: link static libs with whole-archive in subproject

2024-02-18 Thread Thomas Monjalon

29/01/2024 14:17, Bruce Richardson:
> On Mon, Jan 29, 2024 at 01:55:36PM +0100, David Marchand wrote:
> > On Mon, Jan 29, 2024 at 1:47 PM Robin Jarry  wrote:
> > >
> > > When DPDK is used as a subproject, declare static libs to be linked with
> > > -Wl,--whole-archive along with the drivers. This is already done this
> > > way in pkg-config.
> > >
> > > Fixes: f93a605f2d6e ("build: add definitions for use as Meson subproject")
> > > Cc: sta...@dpdk.org
> > >
> > > Signed-off-by: Robin Jarry 
> > Tested-by: David Marchand 
> Acked-by: Bruce Richardson 

Applied, thanks.

Re: [PATCH v4 2/2] config/arm: use common cpu arch for cross files

2024-02-18 Thread Thomas Monjalon

05/12/2023 04:52, Joyce Kong:
> The cpu info in some cross files is inconsistent with
> that in SoC flags. The mismatch doesn't cause any issue
> because the cpu field in the cross file takes no effect
> and machine_args in config/arm/meson.build actually works.
> Use a common one in cross files to remove any confusion.
> 
> Reported-by: Honnappa Nagarahalli 
> Signed-off-by: Joyce Kong 
> Reviewed-by: Ruifeng Wang 

[...]
> --- a/config/arm/arm64_armada_linux_gcc
> +++ b/config/arm/arm64_armada_linux_gcc
> @@ -10,7 +10,7 @@ pcap-config = ''
>  [host_machine]
>  system = 'linux'
>  cpu_family = 'aarch64'
> -cpu = 'armv8-a'
> +cpu = 'aarch64'

Why aarch64 here and below?

> --- a/config/arm/arm64_armv8_linux_clang_ubuntu
> +++ b/config/arm/arm64_armv8_linux_clang_ubuntu
> @@ -10,7 +10,7 @@ pkgconfig = 'aarch64-linux-gnu-pkg-config'
>  [host_machine]
>  system = 'linux'
>  cpu_family = 'aarch64'
> -cpu = 'armv8-a'
> +cpu = 'auto'

[...]
> --- a/config/arm/arm64_hip10_linux_gcc
> +++ b/config/arm/arm64_hip10_linux_gcc
> @@ -9,7 +9,7 @@ pcap-config = ''
>  [host_machine]
>  system = 'linux'
>  cpu_family = 'aarch64'
> -cpu = 'armv8-a'
> +cpu = 'aarch64'

Re: [PATCH v5] mempool: test performance with larger bursts

2024-02-18 Thread Thomas Monjalon

24/01/2024 12:21, Morten Brørup:
> --- a/app/test/test_mempool_perf.c
> +++ b/app/test/test_mempool_perf.c
> @@ -1,6 +1,6 @@
>  /* SPDX-License-Identifier: BSD-3-Clause
>   * Copyright(c) 2010-2014 Intel Corporation
> - * Copyright(c) 2022 SmartShare Systems
> + * Copyright(c) 2022-2024 SmartShare Systems

You don't need to update copyright year.
The first year is the only important one.

reading: 
https://matija.suklje.name/how-and-why-to-properly-write-copyright-statements-in-your-code#why-not-bump-the-year-on-change

[...]
>  REGISTER_PERF_TEST(mempool_perf_autotest, test_mempool_perf);
> +REGISTER_PERF_TEST(mempool_perf_autotest_1core, test_mempool_perf_1core);
> +REGISTER_PERF_TEST(mempool_perf_autotest_2cores, test_mempool_perf_2cores);

How do we make sure the test is skipped if we have only 1 core?

> +REGISTER_PERF_TEST(mempool_perf_autotest_allcores, 
> test_mempool_perf_allcores);

How the test duration is changed after this patch?

Re: [RESEND v7 1/3] ring: fix unmatched type definition and usage

2024-02-18 Thread Thomas Monjalon

09/11/2023 11:20, Jie Hai:
> Field 'flags' of struct rte_ring is defined as int type. However,
> it is used as unsigned int. To ensure consistency, change the
> type of flags to unsigned int. Since these two types has the
> same byte size, this change is not an ABI change.
> 
> Fixes: af75078fece3 ("first public release")
> 
> Signed-off-by: Jie Hai 
> Acked-by: Konstantin Ananyev 
> Acked-by: Chengwen Feng 
> Acked-by: Morten Brørup 
> ---
>  lib/ring/rte_ring_core.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/ring/rte_ring_core.h b/lib/ring/rte_ring_core.h
> index b7708730658a..14dac6495d83 100644
> --- a/lib/ring/rte_ring_core.h
> +++ b/lib/ring/rte_ring_core.h
> @@ -119,7 +119,7 @@ struct rte_ring_hts_headtail {
>  struct rte_ring {
>   char name[RTE_RING_NAMESIZE] __rte_cache_aligned;
>   /**< Name of the ring. */
> - int flags;   /**< Flags supplied at creation. */
> + uint32_t flags;   /**< Flags supplied at creation. */

This triggers a warning in our ABI checker:

  in pointed to type 'struct rte_ring' at rte_ring_core.h:119:1:
type size hasn't changed
1 data member change:
  type of 'int flags' changed:
entity changed from 'int' to compatible type 'typedef uint32_t' at 
stdint-uintn.h:26:1
  type name changed from 'int' to 'unsigned int'
  type size hasn't changed

I guess we were supposed to merge this in 23.11, sorry about this.

How can we proceed?

Re: [PATCH 2/2] rcu: use correct value of acked token in debug print

2024-02-18 Thread Thomas Monjalon

05/02/2024 05:59, Honnappa Nagarahalli:
> Debug print should use acked token value that was used for comparison.
> 
> Fixes: bf386ae377aa ("rcu: add additional debug logs")
> Cc: sta...@dpdk.org
> 
> Reported-by: Nathan Brown 
> Signed-off-by: Honnappa Nagarahalli 
> Reviewed-by: Nathan Brown 

We trust you about RCU :)

Series applied, thanks.

Re: [PATCH] doc: remove cmdline deprecation notice

2024-02-18 Thread Thomas Monjalon

07/02/2024 12:09, Dariusz Sosnowski:
> Remove mention of cmdline_poll() function from deprecation notice,
> because it was removed in 23.11 release.
> 
> Fixes: f44f2edd198a ("cmdline: remove poll function")
> Cc: step...@networkplumber.org
> Cc: sta...@dpdk.org
> 
> Signed-off-by: Dariusz Sosnowski 

Marking as superseded in patchwork,
because Stephen had sent a similar patch in December.

Re: [PATCH 0/3] add support for additional data types

2024-02-18 Thread Thomas Monjalon

07/01/2024 16:28, Srikanth Yalavarthi:
> Added support for 64-bit integer data types for inference input and
> output. Extended support for quantization of 32-bit and 64-bit integer
> data types.
> 
> Srikanth Yalavarthi (3):
>   mldev: add conversion routines for 32-bit integers
>   mldev: add support for 64-integer data type
>   ml/cnxk: add support for additional integer types

Applied, thanks.

Re: [PATCH v10 3/3] app/graph: implement port forward usecase

2024-02-18 Thread Thomas Monjalon

02/01/2024 08:30, Rakesh Kudurumalla:
> --- /dev/null
> +++ b/doc/guides/tools/img/graph-usecase-l2fwd.svg
> @@ -0,0 +1,92 @@
> +
> + + "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd";>
> +
> +
> +
> +

We could include this syntax directly in .rst file
with this Sphinx extension:
https://www.sphinx-doc.org/en/master/usage/extensions/graphviz.html

[PATCH v2] common/qat: add symmetric crypto virtual qat device (vQAT)

2024-02-18 Thread Arkadiusz Kusztal

This commit adds virtual QAT device to the Intel
QuickAssist Technology PMD. Only symmetric crypto service
is enabled with this commit.

Signed-off-by: Arkadiusz Kusztal 
---
 doc/guides/rel_notes/release_24_03.rst   |  3 ++
 drivers/common/qat/dev/qat_dev_gen4.c| 39 +++-
 drivers/common/qat/qat_common.h  |  1 +
 drivers/common/qat/qat_device.c  |  5 +++
 drivers/crypto/qat/dev/qat_crypto_pmd_gen4.c | 12 --
 drivers/crypto/qat/qat_sym_session.c | 13 ---
 6 files changed, 62 insertions(+), 11 deletions(-)

diff --git a/doc/guides/rel_notes/release_24_03.rst 
b/doc/guides/rel_notes/release_24_03.rst
index e9c9717706..3b3977a0b6 100644
--- a/doc/guides/rel_notes/release_24_03.rst
+++ b/doc/guides/rel_notes/release_24_03.rst
@@ -55,6 +55,9 @@ New Features
  Also, make sure to start the actual text at the margin.
  ===
 
+* **Updated Intel QuickAssist Technology driver.**
+
+  * Enabled support for virtual QAT - vQAT (0da5) devices in QAT symmetric 
crypto driver.
 
 Removed Items
 -
diff --git a/drivers/common/qat/dev/qat_dev_gen4.c 
b/drivers/common/qat/dev/qat_dev_gen4.c
index 1ce262f715..8f59fa2d31 100644
--- a/drivers/common/qat/dev/qat_dev_gen4.c
+++ b/drivers/common/qat/dev/qat_dev_gen4.c
@@ -143,6 +143,26 @@ qat_dev_read_config_gen4(struct qat_pci_device *qat_dev)
return 0;
 }
 
+static int
+qat_dev_read_config_vqat(struct qat_pci_device *qat_dev)
+{
+   int i = 0;
+   struct qat_dev_gen4_extra *dev_extra = qat_dev->dev_private;
+   struct qat_qp_hw_data *hw_data;
+
+   for (; i < QAT_GEN4_BUNDLE_NUM; i++) {
+   hw_data = &dev_extra->qp_gen4_data[i][0];
+   memset(hw_data, 0, sizeof(*hw_data));
+   hw_data->service_type = QAT_SERVICE_SYMMETRIC;
+   hw_data->tx_msg_size = 128;
+   hw_data->rx_msg_size = 32;
+   hw_data->tx_ring_num = 0;
+   hw_data->rx_ring_num = 1;
+   hw_data->hw_bundle_num = i;
+   }
+   return 0;
+}
+
 static void
 qat_qp_build_ring_base_gen4(void *io_addr,
struct qat_queue *queue)
@@ -268,6 +288,12 @@ qat_reset_ring_pairs_gen4(struct qat_pci_device 
*qat_pci_dev)
return 0;
 }
 
+static int
+qat_reset_ring_pairs_vqat(struct qat_pci_device *qat_pci_dev __rte_unused)
+{
+   return 0;
+}
+
 static const struct rte_mem_resource *
 qat_dev_get_transport_bar_gen4(struct rte_pci_device *pci_dev)
 {
@@ -304,10 +330,21 @@ static struct qat_dev_hw_spec_funcs qat_dev_hw_spec_gen4 
= {
.qat_dev_get_slice_map = qat_dev_get_slice_map_gen4,
 };
 
+static struct qat_dev_hw_spec_funcs qat_dev_hw_spec_vqat = {
+   .qat_dev_reset_ring_pairs = qat_reset_ring_pairs_vqat,
+   .qat_dev_get_transport_bar = qat_dev_get_transport_bar_gen4,
+   .qat_dev_get_misc_bar = qat_dev_get_misc_bar_gen4,
+   .qat_dev_read_config = qat_dev_read_config_vqat,
+   .qat_dev_get_extra_size = qat_dev_get_extra_size_gen4,
+   .qat_dev_get_slice_map = qat_dev_get_slice_map_gen4,
+};
+
 RTE_INIT(qat_dev_gen_4_init)
 {
-   qat_qp_hw_spec[QAT_GEN4] = &qat_qp_hw_spec_gen4;
+   qat_qp_hw_spec[QAT_VQAT] = qat_qp_hw_spec[QAT_GEN4] = 
&qat_qp_hw_spec_gen4;
qat_dev_hw_spec[QAT_GEN4] = &qat_dev_hw_spec_gen4;
+   qat_dev_hw_spec[QAT_VQAT] = &qat_dev_hw_spec_vqat;
qat_gen_config[QAT_GEN4].dev_gen = QAT_GEN4;
+   qat_gen_config[QAT_VQAT].dev_gen = QAT_VQAT;
qat_gen_config[QAT_GEN4].pf2vf_dev = &qat_pf2vf_gen4;
 }
diff --git a/drivers/common/qat/qat_common.h b/drivers/common/qat/qat_common.h
index 9411a79301..53799ce174 100644
--- a/drivers/common/qat/qat_common.h
+++ b/drivers/common/qat/qat_common.h
@@ -21,6 +21,7 @@ enum qat_device_gen {
QAT_GEN2,
QAT_GEN3,
QAT_GEN4,
+   QAT_VQAT,
QAT_N_GENS
 };
 
diff --git a/drivers/common/qat/qat_device.c b/drivers/common/qat/qat_device.c
index f55dc3c6f0..afcf2b5e99 100644
--- a/drivers/common/qat/qat_device.c
+++ b/drivers/common/qat/qat_device.c
@@ -62,6 +62,9 @@ static const struct rte_pci_id pci_id_qat_map[] = {
{
RTE_PCI_DEVICE(0x8086, 0x4945),
},
+   {
+   RTE_PCI_DEVICE(0x8086, 0x0da5),
+   },
{.device_id = 0},
 };
 
@@ -199,6 +202,8 @@ pick_gen(const struct rte_pci_device *pci_dev)
case 0x4943:
case 0x4945:
return QAT_GEN4;
+   case 0x0da5:
+   return QAT_VQAT;
default:
QAT_LOG(ERR, "Invalid dev_id, can't determine generation");
return QAT_N_GENS;
diff --git a/drivers/crypto/qat/dev/qat_crypto_pmd_gen4.c 
b/drivers/crypto/qat/dev/qat_crypto_pmd_gen4.c
index de72383d4b..5798594b4c 100644
--- a/drivers/crypto/qat/dev/qat_crypto_pmd_gen4.c
+++ b/drivers/crypto/qat/dev/qat_crypto_pmd_gen4.c
@@

Re: [PATCH v10 1/3] node: support to add next node to ethdev Rx node

2024-02-18 Thread Thomas Monjalon

02/01/2024 08:30, Rakesh Kudurumalla:
> +/**
> + * Update ethdev rx next node.
> + *
> + * @param id
> + *   Node id whose edge is to be updated.
> + * @param edge_name
> + *   Name of the next node.
> + *
> + * @return
> + *   ENINVAL: Either of input parameters are invalid

typo: EINVAL

> + *   ENOMEM: If memory allocation failed
> + *   0 on successful initialization.

The errors are rendered on the same line by Doxygen.
Fixed by making it a list.

Re: [PATCH v10 1/3] node: support to add next node to ethdev Rx node

2024-02-18 Thread Thomas Monjalon

02/01/2024 08:30, Rakesh Kudurumalla:
> By default all packets received on ethdev_rx node
> is forwarded to pkt_cls node.This patch provides
> library support to add a new node as next node to
> ethdev_rx node and forward packet to new node from
> rx node.
> 
> Signed-off-by: Rakesh Kudurumalla 

Series applied with few doxygen fixes.

Re: [PATCH V2] pipeline: remove limitation on number of input ports

2024-02-18 Thread Thomas Monjalon

14/02/2024 21:58, Cristian Dumitrescu:
> Removed the requirement that the number of pipeline input ports be a
> power of 2, which is problematic for many real life use-cases. Also
> adding checks for the output port validity used for sending the
> current packet.
> 
> Signed-off-by: Cristian Dumitrescu 

Applied, thanks.

Re: [PATCH 0/3] pipeline: extend the IPv6 support

2024-02-18 Thread Thomas Monjalon

13/02/2024 17:57, Cristian Dumitrescu:
> So far, the pipeline supports the following operations on IPv6 addresses:
> -using an IPv6 address as a table match field of exact/ternary/LPM match type
> -assignment of IPv6 address to another IPv6 address
> -conversion between an IPv4 address and an IPv6 address
> 
> The reason for this limited support is the fact that CPUs have 64-bit 
> registers,
> so supporting operations on 128-bit variables requires additional logic.
> 
> We are now adding support for the following operations involving IPv6 
> addresses:
> -logic operations: AND, OR, XOR
> -shift left/right
> -equality/inequality tests
> 
> The way we do this is by introducing a new instruction (called "movh") to read
> and write the upper half of a 128-bit operand and optimizing the existing
> instruction (called "mov"). This way, we can split an IPv6 address into its
> upper half and lower half and use the existing 64-bit instructions to handle 
> the
> required operations.
> 
> We are still not supporting the following operations on IPv6 addresses, as 
> they
> seem of very little practical use: addition, subtraction, multiplication and
> division.
> 
> Cristian Dumitrescu (3):
>   pipeline: add new instruction for upper half of IPv6 address
>   pipeline: optimize conversion between IPv4 and IPv6 addresses
>   examples/pipeline: add example for IPv6 address swap

Applied, thanks.

Re: [PATCH V3] examples/pipeline: simplify the L2 forwarding examples

2024-02-18 Thread Thomas Monjalon

15/02/2024 20:26, Cristian Dumitrescu:
> Simplified the L2 forwarding examples by removing all tables and
> actions, as they are not really needed for these simple use-cases.
> 
> Signed-off-by: Cristian Dumitrescu 

Applied, thanks.

Re: [PATCH v5] lib/net: fix tcp/udp cksum with padding data

2024-02-18 Thread Thomas Monjalon

14/12/2023 12:20, Morten Brørup:
> > From: Kaiwen Deng [mailto:kaiwenx.d...@intel.com]
> > Sent: Thursday, 14 December 2023 10.23
> > 
> > IEEE 802 packets may have a minimum size limit. The data fields
> > should be padded when necessary. In some cases, the padding data
> > is not zero.
> > 
> > In 'rte_ipv4_udptcp_cksum_mbuf()', as payload length
> > "mbuf->pkt_len - l4_off" is used, which includes padding and if
> > padding is not zero it will end up producing wrong checksum.
> > 
> > This patch will use IP header to get the payload size to calculate
> > tcp/udp checksum.
> > 
> > Fixes: d178f693bbfe ("net: add UDP/TCP checksum in mbuf segments")
> > Cc: sta...@dpdk.org
> > 
> > Signed-off-by: Kaiwen Deng 
> 
> Reviewed-by: Morten Brørup 

Applied, thanks.

Re: [PATCH v2] dmadev: standardize alignment

2024-02-18 Thread Thomas Monjalon

> > Align fp_objects based on cacheline size defined
> > by build configuration.
> >
> > Signed-off-by: Pavan Nikhilesh
> Acked-by: Chengwen Feng 

Applied, thanks.

RE: [PATCH v2 2/3] drivers: use common min/max macros

2024-02-18 Thread Xu, Rosen

Hi,

> -Original Message-
> From: David Marchand 
> Sent: Friday, February 16, 2024 6:25 PM
> To: dev@dpdk.org
> Cc: Ajit Khaparde ; Somnath Kotur
> ; Devendra Singh Rawat
> ; Alok Prasad ; Xu, Rosen
> 
> Subject: [PATCH v2 2/3] drivers: use common min/max macros
> 
> Use newly introduced macro.
> 
> Signed-off-by: David Marchand 
> ---
>  drivers/net/bnxt/bnxt_ethdev.c   | 12 +---
>  drivers/net/qede/base/bcm_osal.h |  6 ++
>  drivers/raw/ifpga/base/osdep_rte/osdep_generic.h | 11 ++-
>  3 files changed, 5 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/net/bnxt/bnxt_ethdev.c
> b/drivers/net/bnxt/bnxt_ethdev.c index 45d840d7af..8cc012206a 100644
> --- a/drivers/net/bnxt/bnxt_ethdev.c
> +++ b/drivers/net/bnxt/bnxt_ethdev.c
> @@ -4867,17 +4867,7 @@ static void bnxt_free_ctx_mem(struct bnxt *bp)
> 
>  #define bnxt_roundup(x, y)   x) + ((y) - 1)) / (y)) * (y))
> 
> -#define min_t(type, x, y) ({\
> - type __min1 = (x);  \
> - type __min2 = (y);  \
> - __min1 < __min2 ? __min1 : __min2; })
> -
> -#define max_t(type, x, y) ({\
> - type __max1 = (x);  \
> - type __max2 = (y);  \
> - __max1 > __max2 ? __max1 : __max2; })
> -
> -#define clamp_t(type, _x, min, max) min_t(type, max_t(type, _x, min),
> max)
> +#define clamp_t(type, _x, min, max) RTE_MIN_T(RTE_MAX_T(_x, min,
> type),
> +max, type)
> 
>  int bnxt_alloc_ctx_mem(struct bnxt *bp)  { diff --git
> a/drivers/net/qede/base/bcm_osal.h b/drivers/net/qede/base/bcm_osal.h
> index 11019b5623..7869103c63 100644
> --- a/drivers/net/qede/base/bcm_osal.h
> +++ b/drivers/net/qede/base/bcm_osal.h
> @@ -443,10 +443,8 @@ u32 qede_osal_log2(u32);  #define OSAL_IOMEM
> volatile
>  #define OSAL_UNUSED__rte_unused
>  #define OSAL_UNLIKELY(x)  __builtin_expect(!!(x), 0)
> -#define OSAL_MIN_T(type, __min1, __min2) \
> - ((type)(__min1) < (type)(__min2) ? (type)(__min1) : (type)(__min2))
> -#define OSAL_MAX_T(type, __max1, __max2) \
> - ((type)(__max1) > (type)(__max2) ? (type)(__max1) :
> (type)(__max2))
> +#define OSAL_MIN_T(type, __min1, __min2) RTE_MIN_T(__min1, __min2,
> +type) #define OSAL_MAX_T(type, __max1, __max2) RTE_MAX_T(__max1,
> +__max2, type)
> 
>  void qede_get_mcp_proto_stats(struct ecore_dev *, enum
> ecore_mcp_protocol_type,
> union ecore_mcp_protocol_stats *); diff --git
> a/drivers/raw/ifpga/base/osdep_rte/osdep_generic.h
> b/drivers/raw/ifpga/base/osdep_rte/osdep_generic.h
> index 62c5666ca9..427793a578 100644
> --- a/drivers/raw/ifpga/base/osdep_rte/osdep_generic.h
> +++ b/drivers/raw/ifpga/base/osdep_rte/osdep_generic.h
> @@ -44,15 +44,8 @@ extern int ifpga_rawdev_logtype;  #define min(a, b)
> RTE_MIN(a, b)  #define max(a, b) RTE_MAX(a, b)
> 
> -#define min_t(type, x, y) ({\
> - type __min1 = (x);  \
> - type __min2 = (y);  \
> - __min1 < __min2 ? __min1 : __min2; })
> -
> -#define max_t(type, x, y) ({\
> - type __max1 = (x);  \
> - type __max2 = (y);  \
> - __max1 > __max2 ? __max1 : __max2; })
> +#define min_t(type, x, y) RTE_MIN_T(x, y, type) #define max_t(type, x,
> +y) RTE_MAX_T(x, y, type)
> 
>  #define spinlock_t rte_spinlock_t
>  #define spinlock_init(x) rte_spinlock_init(x)
> --
> 2.43.0

Reviewed-by: Rosen Xu

Re: [PATCH v2] build: set rte toolchain macros from predefined macros

2024-02-18 Thread Thomas Monjalon

08/01/2024 12:18, Bruce Richardson:
> On Tue, Jan 02, 2024 at 04:11:15PM -0800, Tyler Retzlaff wrote:
> > Stop writing RTE_TOOLCHAIN_XXX macros to rte_build_config.h. When an
> > application builds it doesn't necessarily use the same toolchain that
> > DPDK was built with.
> > 
> > Instead evaluate toolchain predefined macros and define
> > RTE_TOOLCHAIN_XXX macros as appropriate each time rte_config.h is
> > preprocessed.
> > 
> > Signed-off-by: Tyler Retzlaff 
> 
> I don't see an issue with doing this.
> 
> Acked-by: Bruce Richardson 

Applied, thanks.

Re: [PATCH v2 2/3] drivers: use common min/max macros

2024-02-18 Thread Ajit Khaparde

On Fri, Feb 16, 2024 at 2:25 AM David Marchand
 wrote:
>
> Use newly introduced macro.
>
> Signed-off-by: David Marchand 
Acked-by: Ajit Khaparde 

> ---
>  drivers/net/bnxt/bnxt_ethdev.c   | 12 +---
>  drivers/net/qede/base/bcm_osal.h |  6 ++
>  drivers/raw/ifpga/base/osdep_rte/osdep_generic.h | 11 ++-
>  3 files changed, 5 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c
> index 45d840d7af..8cc012206a 100644
> --- a/drivers/net/bnxt/bnxt_ethdev.c
> +++ b/drivers/net/bnxt/bnxt_ethdev.c
> @@ -4867,17 +4867,7 @@ static void bnxt_free_ctx_mem(struct bnxt *bp)
>
>  #define bnxt_roundup(x, y)   x) + ((y) - 1)) / (y)) * (y))
>
> -#define min_t(type, x, y) ({\
> -   type __min1 = (x);  \
> -   type __min2 = (y);  \
> -   __min1 < __min2 ? __min1 : __min2; })
> -
> -#define max_t(type, x, y) ({\
> -   type __max1 = (x);  \
> -   type __max2 = (y);  \
> -   __max1 > __max2 ? __max1 : __max2; })
> -
> -#define clamp_t(type, _x, min, max) min_t(type, max_t(type, _x, min), 
> max)
> +#define clamp_t(type, _x, min, max) RTE_MIN_T(RTE_MAX_T(_x, min, type), max, 
> type)
>
>  int bnxt_alloc_ctx_mem(struct bnxt *bp)
>  {
> diff --git a/drivers/net/qede/base/bcm_osal.h 
> b/drivers/net/qede/base/bcm_osal.h
> index 11019b5623..7869103c63 100644
> --- a/drivers/net/qede/base/bcm_osal.h
> +++ b/drivers/net/qede/base/bcm_osal.h
> @@ -443,10 +443,8 @@ u32 qede_osal_log2(u32);
>  #define OSAL_IOMEM volatile
>  #define OSAL_UNUSED__rte_unused
>  #define OSAL_UNLIKELY(x)  __builtin_expect(!!(x), 0)
> -#define OSAL_MIN_T(type, __min1, __min2)   \
> -   ((type)(__min1) < (type)(__min2) ? (type)(__min1) : (type)(__min2))
> -#define OSAL_MAX_T(type, __max1, __max2)   \
> -   ((type)(__max1) > (type)(__max2) ? (type)(__max1) : (type)(__max2))
> +#define OSAL_MIN_T(type, __min1, __min2) RTE_MIN_T(__min1, __min2, type)
> +#define OSAL_MAX_T(type, __max1, __max2) RTE_MAX_T(__max1, __max2, type)
>
>  void qede_get_mcp_proto_stats(struct ecore_dev *, enum 
> ecore_mcp_protocol_type,
>   union ecore_mcp_protocol_stats *);
> diff --git a/drivers/raw/ifpga/base/osdep_rte/osdep_generic.h 
> b/drivers/raw/ifpga/base/osdep_rte/osdep_generic.h
> index 62c5666ca9..427793a578 100644
> --- a/drivers/raw/ifpga/base/osdep_rte/osdep_generic.h
> +++ b/drivers/raw/ifpga/base/osdep_rte/osdep_generic.h
> @@ -44,15 +44,8 @@ extern int ifpga_rawdev_logtype;
>  #define min(a, b) RTE_MIN(a, b)
>  #define max(a, b) RTE_MAX(a, b)
>
> -#define min_t(type, x, y) ({\
> -   type __min1 = (x);  \
> -   type __min2 = (y);  \
> -   __min1 < __min2 ? __min1 : __min2; })
> -
> -#define max_t(type, x, y) ({\
> -   type __max1 = (x);  \
> -   type __max2 = (y);  \
> -   __max1 > __max2 ? __max1 : __max2; })
> +#define min_t(type, x, y) RTE_MIN_T(x, y, type)
> +#define max_t(type, x, y) RTE_MAX_T(x, y, type)
>
>  #define spinlock_t rte_spinlock_t
>  #define spinlock_init(x) rte_spinlock_init(x)
> --
> 2.43.0
>


smime.p7s
Description: S/MIME Cryptographic Signature

[PATCH] net/gve: Change ERR to DEBUG to prevent flooding of logs for Tx-Dqo.

2024-02-18 Thread Rushil Gupta

This was causing failure for testpmd runs (for queues >=15)
presumably due to flooding of logs due to descriptor ring being
overwritten.

Fixes: a01854 ("net/gve: fix dqo bug for chained descriptors")
Cc: sta...@dpdk.org

Signed-off-by: Rushil Gupta 
Reviewed-by: Joshua Washington 
---
 drivers/net/gve/gve_tx_dqo.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/gve/gve_tx_dqo.c b/drivers/net/gve/gve_tx_dqo.c
index 1a8eb96ea9..30a1455b20 100644
--- a/drivers/net/gve/gve_tx_dqo.c
+++ b/drivers/net/gve/gve_tx_dqo.c
@@ -116,7 +116,7 @@ gve_tx_burst_dqo(void *tx_queue, struct rte_mbuf **tx_pkts, 
uint16_t nb_pkts)
first_sw_id = sw_id;
do {
if (sw_ring[sw_id] != NULL)
-   PMD_DRV_LOG(ERR, "Overwriting an entry in 
sw_ring");
+   PMD_DRV_LOG(DEBUG, "Overwriting an entry in 
sw_ring");
 
txd = &txr[tx_id];
sw_ring[sw_id] = tx_pkt;
-- 
2.44.0.rc0.258.g7320e95886-goog

Re: [PATCH v2 00/11] add Traffic Manager feature

2024-02-18 Thread lihuisong (C)


Hi Thomas and Ferruh,

Can you take a look at this series adding TM feature in doc?

在 2023/11/28 20:40, Huisong Li 写道:

The traffic management API has been introduced for a long time, please see
commit 5d109deffa87 ("ethdev: add traffic management API").
And many PMD also support this feature. So this series add this feature to
features.rst, default.ini and driver.ini.

---
  v2:
   - fix the order of some feature for patch 2/11, 8/11 and 11/11.

Huisong Li (11):
   doc: add Traffic Manager feature
   doc: add Traffic Manager feature for iavf
   doc: add Traffic Manager feature for i40e
   doc: add Traffic Manager feature for ixgbe
   doc: add Traffic Manager feature for cnxk
   doc: add Traffic Manager feature for hns3
   doc: add Traffic Manager feature for txgbe
   doc: add Traffic Manager feature for mvpp2
   doc: add Traffic Manager feature for dpaa2
   doc: add Traffic Manager feature for ipn3ke
   doc: add Traffic Manager feature for ice

  doc/guides/nics/features.rst | 13 +
  doc/guides/nics/features/cnxk.ini|  1 +
  doc/guides/nics/features/default.ini |  1 +
  doc/guides/nics/features/dpaa2.ini   |  1 +
  doc/guides/nics/features/hns3.ini|  1 +
  doc/guides/nics/features/i40e.ini|  1 +
  doc/guides/nics/features/iavf.ini|  3 ++-
  doc/guides/nics/features/ice.ini |  1 +
  doc/guides/nics/features/ice_dcf.ini |  1 +
  doc/guides/nics/features/ipn3ke.ini  |  1 +
  doc/guides/nics/features/ixgbe.ini   |  1 +
  doc/guides/nics/features/mvpp2.ini   |  3 ++-
  doc/guides/nics/features/txgbe.ini   |  1 +
  13 files changed, 27 insertions(+), 2 deletions(-)

RE: [PATCH v5 3/3] net/mlx5/hws: add compare ESP sequence number support

2024-02-18 Thread Suanming Mou

Hi,

> -Original Message-
> From: Michael Baum 
> Sent: Wednesday, February 14, 2024 3:30 PM
> To: dev@dpdk.org
> Cc: Matan Azrad ; Dariusz Sosnowski
> ; Raslan Darawsheh ; Slava
> Ovsiienko ; Ori Kam ; Suanming
> Mou 
> Subject: [PATCH v5 3/3] net/mlx5/hws: add compare ESP sequence number
> support
> 
> Add support for compare item with "RTE_FLOW_FIELD_ESP_SEQ_NUM" field.


Small comment, please don't forget to add the new supported comparison field to 
rel_notes.

> 
> Signed-off-by: Michael Baum 
Acked-by: Suanming Mou 

> ---
>  doc/guides/nics/mlx5.rst  |  1 +
>  drivers/net/mlx5/hws/mlx5dr_definer.c | 22 --
>  drivers/net/mlx5/mlx5_flow_hw.c   |  3 +++
>  3 files changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/doc/guides/nics/mlx5.rst b/doc/guides/nics/mlx5.rst index
> 43ef8a99dc..b793f1ef58 100644
> --- a/doc/guides/nics/mlx5.rst
> +++ b/doc/guides/nics/mlx5.rst
> @@ -823,6 +823,7 @@ Limitations
>- Only single item is supported per pattern template.
>- Only 32-bit comparison is supported or 16-bits for random field.
>- Only supported for ``RTE_FLOW_FIELD_META``, ``RTE_FLOW_FIELD_TAG``,
> +``RTE_FLOW_FIELD_ESP_SEQ_NUM``,
>  ``RTE_FLOW_FIELD_RANDOM`` and ``RTE_FLOW_FIELD_VALUE``.
>- The field type ``RTE_FLOW_FIELD_VALUE`` must be the base (``b``) field.
>- The field type ``RTE_FLOW_FIELD_RANDOM`` can only be compared with diff
> --git a/drivers/net/mlx5/hws/mlx5dr_definer.c
> b/drivers/net/mlx5/hws/mlx5dr_definer.c
> index 2d86175ca2..b29d7451e7 100644
> --- a/drivers/net/mlx5/hws/mlx5dr_definer.c
> +++ b/drivers/net/mlx5/hws/mlx5dr_definer.c
> @@ -396,10 +396,20 @@ mlx5dr_definer_compare_base_value_set(const void
> *item_spec,
> 
>   value = (const uint32_t *)&b->value[0];
> 
> - if (a->field == RTE_FLOW_FIELD_RANDOM)
> + switch (a->field) {
> + case RTE_FLOW_FIELD_RANDOM:
>   *base = htobe32(*value << 16);
> - else
> + break;
> + case RTE_FLOW_FIELD_TAG:
> + case RTE_FLOW_FIELD_META:
>   *base = htobe32(*value);
> + break;
> + case RTE_FLOW_FIELD_ESP_SEQ_NUM:
> + *base = *value;
> + break;
> + default:
> + break;
> + }
> 
>   MLX5_SET(ste_match_4dw_range_ctrl_dw, ctrl, base0, 1);  } @@ -2887,6
> +2897,14 @@ mlx5dr_definer_conv_item_compare_field(const struct
> rte_flow_field_data *f,
>   fc->compare_idx = dw_offset;
>   DR_CALC_SET_HDR(fc, random_number, random_number);
>   break;
> + case RTE_FLOW_FIELD_ESP_SEQ_NUM:
> + fc = &cd-
> >fc[MLX5DR_DEFINER_FNAME_ESP_SEQUENCE_NUMBER];
> + fc->item_idx = item_idx;
> + fc->tag_set = &mlx5dr_definer_compare_set;
> + fc->tag_mask_set = &mlx5dr_definer_ones_set;
> + fc->compare_idx = dw_offset;
> + DR_CALC_SET_HDR(fc, ipsec, sequence_number);
> + break;
>   default:
>   DR_LOG(ERR, "%u field is not supported", f->field);
>   goto err_notsup;
> diff --git a/drivers/net/mlx5/mlx5_flow_hw.c
> b/drivers/net/mlx5/mlx5_flow_hw.c index b5741f0817..4d6fb489b2 100644
> --- a/drivers/net/mlx5/mlx5_flow_hw.c
> +++ b/drivers/net/mlx5/mlx5_flow_hw.c
> @@ -6725,6 +6725,7 @@ flow_hw_item_compare_field_validate(enum
> rte_flow_field_id arg_field,
>   switch (arg_field) {
>   case RTE_FLOW_FIELD_TAG:
>   case RTE_FLOW_FIELD_META:
> + case RTE_FLOW_FIELD_ESP_SEQ_NUM:
>   break;
>   case RTE_FLOW_FIELD_RANDOM:
>   if (base_field == RTE_FLOW_FIELD_VALUE) @@ -6743,6 +6744,7
> @@ flow_hw_item_compare_field_validate(enum rte_flow_field_id arg_field,
>   case RTE_FLOW_FIELD_TAG:
>   case RTE_FLOW_FIELD_META:
>   case RTE_FLOW_FIELD_VALUE:
> + case RTE_FLOW_FIELD_ESP_SEQ_NUM:
>   break;
>   default:
>   return rte_flow_error_set(error, ENOTSUP, @@ -6759,6 +6761,7
> @@ flow_hw_item_compare_width_supported(enum rte_flow_field_id field)
>   switch (field) {
>   case RTE_FLOW_FIELD_TAG:
>   case RTE_FLOW_FIELD_META:
> + case RTE_FLOW_FIELD_ESP_SEQ_NUM:
>   return 32;
>   case RTE_FLOW_FIELD_RANDOM:
>   return 16;
> --
> 2.25.1

RE: [PATCH v5 2/3] net/mlx5: add support to compare random value

2024-02-18 Thread Suanming Mou




> -Original Message-
> From: Michael Baum 
> Sent: Wednesday, February 14, 2024 3:30 PM
> To: dev@dpdk.org
> Cc: Matan Azrad ; Dariusz Sosnowski
> ; Raslan Darawsheh ; Slava
> Ovsiienko ; Ori Kam ; Suanming
> Mou 
> Subject: [PATCH v5 2/3] net/mlx5: add support to compare random value
> 
> Add support to use "RTE_FLOW_ITEM_TYPE_COMPARE" with
> "RTE_FLOW_FIELD_RAMDOM" as an argument.
> The random field is supported only when base is an immediate value, random
> field cannot be compared with enother field.
> 
> Signed-off-by: Michael Baum 
Acked-by: Suanming Mou 

> ---
>  doc/guides/nics/mlx5.rst|  9 -
>  drivers/net/mlx5/mlx5_flow_hw.c | 70 -
>  2 files changed, 59 insertions(+), 20 deletions(-)
> 
> diff --git a/doc/guides/nics/mlx5.rst b/doc/guides/nics/mlx5.rst index
> fa013b03bb..43ef8a99dc 100644
> --- a/doc/guides/nics/mlx5.rst
> +++ b/doc/guides/nics/mlx5.rst
> @@ -820,8 +820,13 @@ Limitations
> 
>- Only supported in HW steering(``dv_flow_en`` = 2) mode.
>- Only single flow is supported to the flow table.
> -  - Only 32-bit comparison is supported.
> -  - Only match with compare result between packet fields is supported.
> +  - Only single item is supported per pattern template.
> +  - Only 32-bit comparison is supported or 16-bits for random field.
> +  - Only supported for ``RTE_FLOW_FIELD_META``, ``RTE_FLOW_FIELD_TAG``,
> +``RTE_FLOW_FIELD_RANDOM`` and ``RTE_FLOW_FIELD_VALUE``.
> +  - The field type ``RTE_FLOW_FIELD_VALUE`` must be the base (``b``) field.
> +  - The field type ``RTE_FLOW_FIELD_RANDOM`` can only be compared with
> +``RTE_FLOW_FIELD_VALUE``.
> 
> 
>  Statistics
> diff --git a/drivers/net/mlx5/mlx5_flow_hw.c
> b/drivers/net/mlx5/mlx5_flow_hw.c index 3af5e1f160..b5741f0817 100644
> --- a/drivers/net/mlx5/mlx5_flow_hw.c
> +++ b/drivers/net/mlx5/mlx5_flow_hw.c
> @@ -6717,18 +6717,55 @@ flow_hw_prepend_item(const struct rte_flow_item
> *items,
>   return copied_items;
>  }
> 
> -static inline bool
> -flow_hw_item_compare_field_supported(enum rte_flow_field_id field)
> +static int
> +flow_hw_item_compare_field_validate(enum rte_flow_field_id arg_field,
> + enum rte_flow_field_id base_field,
> + struct rte_flow_error *error)
>  {
> - switch (field) {
> + switch (arg_field) {
> + case RTE_FLOW_FIELD_TAG:
> + case RTE_FLOW_FIELD_META:
> + break;
> + case RTE_FLOW_FIELD_RANDOM:
> + if (base_field == RTE_FLOW_FIELD_VALUE)
> + return 0;
> + return rte_flow_error_set(error, EINVAL,
> +
> RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
> +   NULL,
> +   "compare random is supported only
> with immediate value");
> + default:
> + return rte_flow_error_set(error, ENOTSUP,
> +
> RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
> +   NULL,
> +   "compare item argument field is not
> supported");
> + }
> + switch (base_field) {
>   case RTE_FLOW_FIELD_TAG:
>   case RTE_FLOW_FIELD_META:
>   case RTE_FLOW_FIELD_VALUE:
> - return true;
> + break;
> + default:
> + return rte_flow_error_set(error, ENOTSUP,
> +
> RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
> +   NULL,
> +   "compare item base field is not
> supported");
> + }
> + return 0;
> +}
> +
> +static inline uint32_t
> +flow_hw_item_compare_width_supported(enum rte_flow_field_id field) {
> + switch (field) {
> + case RTE_FLOW_FIELD_TAG:
> + case RTE_FLOW_FIELD_META:
> + return 32;
> + case RTE_FLOW_FIELD_RANDOM:
> + return 16;
>   default:
>   break;
>   }
> - return false;
> + return 0;
>  }
> 
>  static int
> @@ -6737,6 +6774,7 @@ flow_hw_validate_item_compare(const struct
> rte_flow_item *item,  {
>   const struct rte_flow_item_compare *comp_m = item->mask;
>   const struct rte_flow_item_compare *comp_v = item->spec;
> + int ret;
> 
>   if (unlikely(!comp_m))
>   return rte_flow_error_set(error, EINVAL, @@ -6748,19 +6786,13
> @@ flow_hw_validate_item_compare(const struct rte_flow_item *item,
>  RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
>  NULL,
>  "compare item only support full mask");
> - if (!flow_hw_item_compare_field_supported(comp_m->a.field) ||
> - !flow_hw_item_compare_field_supported(comp_m->b.field))
> - return rte_flow_error_set(error, ENOTSUP,
> -RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
> -NULL,
> -"compare item field not support");
> - if (comp_m->a.field == RTE_FLOW_FIELD_VAL

RE: [PATCH v5 1/3] net/mlx5/hws: add support for compare matcher

2024-02-18 Thread Suanming Mou




> -Original Message-
> From: Michael Baum 
> Sent: Wednesday, February 14, 2024 3:30 PM
> To: dev@dpdk.org
> Cc: Matan Azrad ; Dariusz Sosnowski
> ; Raslan Darawsheh ; Slava
> Ovsiienko ; Ori Kam ; Suanming
> Mou ; Hamdan Igbaria 
> Subject: [PATCH v5 1/3] net/mlx5/hws: add support for compare matcher
> 
> From: Hamdan Igbaria 
> 
> Add support for compare matcher, this matcher will allow direct comparison
> between two packet fields, or a packet field and a value, with fully masked 
> DW.
> For now this matcher hash table is limited to size 1x1, thus it supports only 
> 1 rule
> STE.
> 
> Signed-off-by: Hamdan Igbaria 
> Signed-off-by: Michael Baum 
Acked-by: Suanming Mou 

> ---
>  drivers/common/mlx5/mlx5_prm.h|  16 ++
>  drivers/net/mlx5/hws/mlx5dr_cmd.c |   9 +-
>  drivers/net/mlx5/hws/mlx5dr_cmd.h |   1 +
>  drivers/net/mlx5/hws/mlx5dr_debug.c   |   4 +-
>  drivers/net/mlx5/hws/mlx5dr_debug.h   |   1 +
>  drivers/net/mlx5/hws/mlx5dr_definer.c | 243 +-
> drivers/net/mlx5/hws/mlx5dr_definer.h |  33 
> drivers/net/mlx5/hws/mlx5dr_matcher.c |  48 +
> drivers/net/mlx5/hws/mlx5dr_matcher.h |  12 +-
>  9 files changed, 358 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/common/mlx5/mlx5_prm.h
> b/drivers/common/mlx5/mlx5_prm.h index 0035a1e616..f8956c8a87 100644
> --- a/drivers/common/mlx5/mlx5_prm.h
> +++ b/drivers/common/mlx5/mlx5_prm.h
> @@ -3459,6 +3459,7 @@ enum mlx5_ifc_rtc_ste_format {
>   MLX5_IFC_RTC_STE_FORMAT_8DW = 0x4,
>   MLX5_IFC_RTC_STE_FORMAT_11DW = 0x5,
>   MLX5_IFC_RTC_STE_FORMAT_RANGE = 0x7,
> + MLX5_IFC_RTC_STE_FORMAT_4DW_RANGE = 0x8,
>  };
> 
>  enum mlx5_ifc_rtc_reparse_mode {
> @@ -3497,6 +3498,21 @@ struct mlx5_ifc_rtc_bits {
>   u8 reserved_at_1a0[0x260];
>  };
> 
> +struct mlx5_ifc_ste_match_4dw_range_ctrl_dw_bits {
> + u8 match[0x1];
> + u8 reserved_at_1[0x2];
> + u8 base1[0x1];
> + u8 inverse1[0x1];
> + u8 reserved_at_5[0x1];
> + u8 operator1[0x2];
> + u8 reserved_at_8[0x3];
> + u8 base0[0x1];
> + u8 inverse0[0x1];
> + u8 reserved_at_a[0x1];
> + u8 operator0[0x2];
> + u8 compare_delta[0x10];
> +};
> +
>  struct mlx5_ifc_alias_context_bits {
>   u8 vhca_id_to_be_accessed[0x10];
>   u8 reserved_at_10[0xd];
> diff --git a/drivers/net/mlx5/hws/mlx5dr_cmd.c
> b/drivers/net/mlx5/hws/mlx5dr_cmd.c
> index 876a47147d..702d6fadac 100644
> --- a/drivers/net/mlx5/hws/mlx5dr_cmd.c
> +++ b/drivers/net/mlx5/hws/mlx5dr_cmd.c
> @@ -370,9 +370,12 @@ mlx5dr_cmd_rtc_create(struct ibv_context *ctx,
>attr, obj_type, MLX5_GENERAL_OBJ_TYPE_RTC);
> 
>   attr = MLX5_ADDR_OF(create_rtc_in, in, rtc);
> - MLX5_SET(rtc, attr, ste_format_0, rtc_attr->is_frst_jumbo ?
> - MLX5_IFC_RTC_STE_FORMAT_11DW :
> - MLX5_IFC_RTC_STE_FORMAT_8DW);
> + if (rtc_attr->is_compare) {
> + MLX5_SET(rtc, attr, ste_format_0,
> MLX5_IFC_RTC_STE_FORMAT_4DW_RANGE);
> + } else {
> + MLX5_SET(rtc, attr, ste_format_0, rtc_attr->is_frst_jumbo ?
> +  MLX5_IFC_RTC_STE_FORMAT_11DW :
> MLX5_IFC_RTC_STE_FORMAT_8DW);
> + }
> 
>   if (rtc_attr->is_scnd_range) {
>   MLX5_SET(rtc, attr, ste_format_1,
> MLX5_IFC_RTC_STE_FORMAT_RANGE); diff --git
> a/drivers/net/mlx5/hws/mlx5dr_cmd.h b/drivers/net/mlx5/hws/mlx5dr_cmd.h
> index 18c2b07fc8..073ffd9633 100644
> --- a/drivers/net/mlx5/hws/mlx5dr_cmd.h
> +++ b/drivers/net/mlx5/hws/mlx5dr_cmd.h
> @@ -82,6 +82,7 @@ struct mlx5dr_cmd_rtc_create_attr {
>   uint8_t reparse_mode;
>   bool is_frst_jumbo;
>   bool is_scnd_range;
> + bool is_compare;
>  };
> 
>  struct mlx5dr_cmd_alias_obj_create_attr { diff --git
> a/drivers/net/mlx5/hws/mlx5dr_debug.c
> b/drivers/net/mlx5/hws/mlx5dr_debug.c
> index 11557bcab8..a9094cd35b 100644
> --- a/drivers/net/mlx5/hws/mlx5dr_debug.c
> +++ b/drivers/net/mlx5/hws/mlx5dr_debug.c
> @@ -99,6 +99,7 @@ static int
>  mlx5dr_debug_dump_matcher_match_template(FILE *f, struct mlx5dr_matcher
> *matcher)  {
>   bool is_root = matcher->tbl->level == MLX5DR_ROOT_LEVEL;
> + bool is_compare = mlx5dr_matcher_is_compare(matcher);
>   enum mlx5dr_debug_res_type type;
>   int i, ret;
> 
> @@ -117,7 +118,8 @@ mlx5dr_debug_dump_matcher_match_template(FILE *f,
> struct mlx5dr_matcher *matcher
>   return rte_errno;
>   }
> 
> - type =
> MLX5DR_DEBUG_RES_TYPE_MATCHER_TEMPLATE_MATCH_DEFINER;
> + type = is_compare ?
> MLX5DR_DEBUG_RES_TYPE_MATCHER_TEMPLATE_COMPARE_MATCH_DEFINE
> R :
> +
> MLX5DR_DEBUG_RES_TYPE_MATCHER_TEMPLATE_MATCH_DEFINER;
>   ret = mlx5dr_debug_dump_matcher_template_definer(f, mt, mt-
> >definer, type);
>   if (ret)
>   return ret;
> diff --git a/drivers/net/mlx5/hws/mlx5dr_debug.h
> b/drivers/net/mlx5/hws/mlx5dr_debug.h
> index 5cffdb10b5..a89a6a0b1d 100644
> --- a/drivers/net/mlx5/hws/mlx5dr_

Re: [PATCH v10 3/3] app/graph: implement port forward usecase

2024-02-18 Thread Jerin Jacob

On Mon, Feb 19, 2024 at 4:00 AM Thomas Monjalon  wrote:
>
> 02/01/2024 08:30, Rakesh Kudurumalla:
> > --- /dev/null
> > +++ b/doc/guides/tools/img/graph-usecase-l2fwd.svg
> > @@ -0,0 +1,92 @@
> > +
> > + > + "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd";>
> > +
> > +
> > +
> > +
>
> We could include this syntax directly in .rst file
> with this Sphinx extension:
> https://www.sphinx-doc.org/en/master/usage/extensions/graphviz.html

I checked that option earlier. Created this scheme to avoid graphviz
package dependency
to DPDK.

commit 597f51c34826b3b12b6a1ac9d54e9160aaa49ef7
Author: Jerin Jacob 
Date:   Fri Jun 23 13:06:00 2023 +0530

doc: add inbuilt graph nodes data flow

Added diagram to depict the data flow between inbuilt
graph nodes.

In order to avoid graphviz package dependency to DPDK
documentation, manual step added to create a svg file
from the dot file. The details for the same is documented
in graph_inbuilt_node_flow.svg as a comment.

Signed-off-by: Jerin Jacob 
Reviewed-by: Zhirun Yan 
>
>
>

Re: [RFC V1 1/1] net: extend VXLAN header to support more extensions

2024-02-18 Thread Gavin Li





On 2/9/2024 11:32 PM, Thomas Monjalon wrote:

09/02/2024 15:58, Ferruh Yigit:

On 2/9/2024 1:44 PM, Thomas Monjalon wrote:

09/02/2024 13:11, Ferruh Yigit:

On 2/9/2024 10:12 AM, Thomas Monjalon wrote:

09/02/2024 00:54, Ferruh Yigit:

On 1/30/2024 11:25 AM, Gavin Li wrote:

Currently, DPDK supports VXLAN and VXLAN-GPE with similar header
structures and we are working on adding support for VXLAN-GBP which is
another extension to VXLAN. More extension of VXLAN may be added in the
future.

VXLAN and VXLAN-GBP use the same UDP port(4789) while VXLAN-GPE uses a
different one, 4790. The three protocols have the same header length and
overall similar header structure as below.
 0   1   2   3
 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|R|R|R|R|I|R|R|R|Reserved   |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|VXLAN Network Identifier (VNI) |   Reserved|
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

Figure 1: VXLAN Header

 0   1   2   3
 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|R|R|Ver|I|P|B|O|   Reserved|Next Protocol  |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|VXLAN Network Identifier (VNI) |   Reserved|
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

  Figure 2: VXLAN-GPE Header

 0   1   2   3
 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|G|R|R|R|I|R|R|R|R|D|R|R|A|R|R|R|Group Policy ID|
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|  VXLAN Network Identifier (VNI)   |   Reserved|
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

   Figure 3: VXLAN-GBP Extension

Both VXLAN-GPE and VXLAN-GBP extended VXLAN by redefining its reserved
bits, which means the packets can be processed with same pattern and most
of the code can be reused. Instead of adding more new items by
copying/pasting code for the VXLAN extensions in the future, it’s better
to use existing VXLAN infrastructure and add support code in it.



Hi Gavin,

The motivation is to prevent code duplication, and the code mentioned is
  the driver code, right?


The motivation is mainly to provide a unified and more explicit API.



 From user perspective, I think existing approach is more explicit,
because it sets VXLAN or VXLAN_GPE flow types.

I am trying to understand the benefit, how unifying flow type in the API
helps to the user?


Overall OK to unify "struct rte_vxlan_hdr" although it makes the struct
a little complex, perhaps we can consider extraction some nested structs
as named struct, no strong opinion.


But not sure about removing the flow item types for VXLAN-GPE, or not
adding for VXLAN-GBP.

Think about a case user adding a rule, which has a item type as VXLAN
and in the protocol header some bits are set, lets say first word, last
byte is set, how driver will know if to take it as GPE "next protocol"
or "group policy id".


The driver may decide depending on the UDP port and some distinguishing flags.
If you want to match on GBP, you should includes the GBP flag in your pattern,
no need to use a separate item.



Why not be more explicit?
It helps to driver to know more about the pattern to be able to create
proper flow rule, if there is an obvious way for driver to differentiate
these protocol extensions, and flow item type is redundant, I can
understand the proposal, but otherwise I think better to keep flow items
for extensions.


In any case we need the simple VXLAN item.
If we have GPE and GBP specialized items,
what means a match on the simple VXLAN?
Does it include packets with other extensions or exclude them?
Matching the bits in the protocol make such decision explicit.


When a rule is set in HW, HW may not care about the protocol, as long as
bits in the rule match with the packet, HW can apply the action.
But for driver to be able to set the rule properly, it needs more
explicit information.


Yes information is in the pattern to match.


Lets assume driver API get a pattern with 'RTE_FLOW_ITEM_TYPE_VXLAN'
type and "struct rte_flow_item_vxlan", at this point driver doesn't know
if it is VXLAN or any of the extensions.


Yes it knows because of the matched bits in the pattern.
If the rule specify a match on GBP flag = 1, it is GBP only.
If the rule specify a match on GBP flag = 0, it excludes GBP.
If the rule does not m

Re: [RFC V1 1/1] net: extend VXLAN header to support more extensions

2024-02-18 Thread Gavin Li





On 2/9/2024 11:32 PM, Thomas Monjalon wrote:

09/02/2024 15:58, Ferruh Yigit:

On 2/9/2024 1:44 PM, Thomas Monjalon wrote:

09/02/2024 13:11, Ferruh Yigit:

On 2/9/2024 10:12 AM, Thomas Monjalon wrote:

09/02/2024 00:54, Ferruh Yigit:

On 1/30/2024 11:25 AM, Gavin Li wrote:

Currently, DPDK supports VXLAN and VXLAN-GPE with similar header
structures and we are working on adding support for VXLAN-GBP which is
another extension to VXLAN. More extension of VXLAN may be added in the
future.

VXLAN and VXLAN-GBP use the same UDP port(4789) while VXLAN-GPE uses a
different one, 4790. The three protocols have the same header length and
overall similar header structure as below.
 0   1   2   3
 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|R|R|R|R|I|R|R|R|Reserved   |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|VXLAN Network Identifier (VNI) |   Reserved|
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

Figure 1: VXLAN Header

 0   1   2   3
 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|R|R|Ver|I|P|B|O|   Reserved|Next Protocol  |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|VXLAN Network Identifier (VNI) |   Reserved|
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

  Figure 2: VXLAN-GPE Header

 0   1   2   3
 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|G|R|R|R|I|R|R|R|R|D|R|R|A|R|R|R|Group Policy ID|
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|  VXLAN Network Identifier (VNI)   |   Reserved|
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

   Figure 3: VXLAN-GBP Extension

Both VXLAN-GPE and VXLAN-GBP extended VXLAN by redefining its reserved
bits, which means the packets can be processed with same pattern and most
of the code can be reused. Instead of adding more new items by
copying/pasting code for the VXLAN extensions in the future, it’s better
to use existing VXLAN infrastructure and add support code in it.



Hi Gavin,

The motivation is to prevent code duplication, and the code mentioned is
  the driver code, right?


The motivation is mainly to provide a unified and more explicit API.



 From user perspective, I think existing approach is more explicit,
because it sets VXLAN or VXLAN_GPE flow types.

I am trying to understand the benefit, how unifying flow type in the API
helps to the user?


Overall OK to unify "struct rte_vxlan_hdr" although it makes the struct
a little complex, perhaps we can consider extraction some nested structs
as named struct, no strong opinion.


But not sure about removing the flow item types for VXLAN-GPE, or not
adding for VXLAN-GBP.

Think about a case user adding a rule, which has a item type as VXLAN
and in the protocol header some bits are set, lets say first word, last
byte is set, how driver will know if to take it as GPE "next protocol"
or "group policy id".


The driver may decide depending on the UDP port and some distinguishing flags.
If you want to match on GBP, you should includes the GBP flag in your pattern,
no need to use a separate item.



Why not be more explicit?
It helps to driver to know more about the pattern to be able to create
proper flow rule, if there is an obvious way for driver to differentiate
these protocol extensions, and flow item type is redundant, I can
understand the proposal, but otherwise I think better to keep flow items
for extensions.


In any case we need the simple VXLAN item.
If we have GPE and GBP specialized items,
what means a match on the simple VXLAN?
Does it include packets with other extensions or exclude them?
Matching the bits in the protocol make such decision explicit.


When a rule is set in HW, HW may not care about the protocol, as long as
bits in the rule match with the packet, HW can apply the action.
But for driver to be able to set the rule properly, it needs more
explicit information.


Yes information is in the pattern to match.


Lets assume driver API get a pattern with 'RTE_FLOW_ITEM_TYPE_VXLAN'
type and "struct rte_flow_item_vxlan", at this point driver doesn't know
if it is VXLAN or any of the extensions.


Yes it knows because of the matched bits in the pattern.
If the rule specify a match on GBP flag = 1, it is GBP only.
If the rule specify a match on GBP flag = 0, it excludes GBP.
If the rule does not m

Re: [RFC V1 1/1] net: extend VXLAN header to support more extensions

2024-02-18 Thread Gavin Li





On 2/19/2024 11:44 AM, Gavin Li wrote:



On 2/9/2024 11:32 PM, Thomas Monjalon wrote:

09/02/2024 15:58, Ferruh Yigit:

On 2/9/2024 1:44 PM, Thomas Monjalon wrote:

09/02/2024 13:11, Ferruh Yigit:

On 2/9/2024 10:12 AM, Thomas Monjalon wrote:

09/02/2024 00:54, Ferruh Yigit:

On 1/30/2024 11:25 AM, Gavin Li wrote:

Currently, DPDK supports VXLAN and VXLAN-GPE with similar header
structures and we are working on adding support for VXLAN-GBP 
which is
another extension to VXLAN. More extension of VXLAN may be added 
in the

future.

VXLAN and VXLAN-GBP use the same UDP port(4789) while VXLAN-GPE 
uses a
different one, 4790. The three protocols have the same header 
length and

overall similar header structure as below.
 0   1   2   3
 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 
0 1

+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
    |R|R|R|R|I|R|R|R|
Reserved   |

+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
    |    VXLAN Network Identifier (VNI) |   
Reserved    |

+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+


    Figure 1: VXLAN Header

 0   1   2   3
 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 
0 1

+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
    |R|R|Ver|I|P|B|O|   Reserved    |Next 
Protocol  |

+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
    |    VXLAN Network Identifier (VNI) |   
Reserved    |

+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+


  Figure 2: VXLAN-GPE Header

 0   1   2   3
 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 
0 1

+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
    |G|R|R|R|I|R|R|R|R|D|R|R|A|R|R|R|    Group Policy 
ID    |

+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
    |  VXLAN Network Identifier (VNI)   |   
Reserved    |

+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+


   Figure 3: VXLAN-GBP Extension

Both VXLAN-GPE and VXLAN-GBP extended VXLAN by redefining its 
reserved
bits, which means the packets can be processed with same pattern 
and most

of the code can be reused. Instead of adding more new items by
copying/pasting code for the VXLAN extensions in the future, 
it’s better

to use existing VXLAN infrastructure and add support code in it.



Hi Gavin,

The motivation is to prevent code duplication, and the code 
mentioned is

  the driver code, right?


The motivation is mainly to provide a unified and more explicit API.



 From user perspective, I think existing approach is more explicit,
because it sets VXLAN or VXLAN_GPE flow types.

I am trying to understand the benefit, how unifying flow type in 
the API

helps to the user?

Overall OK to unify "struct rte_vxlan_hdr" although it makes the 
struct
a little complex, perhaps we can consider extraction some nested 
structs

as named struct, no strong opinion.


But not sure about removing the flow item types for VXLAN-GPE, or 
not

adding for VXLAN-GBP.

Think about a case user adding a rule, which has a item type as 
VXLAN
and in the protocol header some bits are set, lets say first 
word, last
byte is set, how driver will know if to take it as GPE "next 
protocol"

or "group policy id".


The driver may decide depending on the UDP port and some 
distinguishing flags.
If you want to match on GBP, you should includes the GBP flag in 
your pattern,

no need to use a separate item.



Why not be more explicit?
It helps to driver to know more about the pattern to be able to create
proper flow rule, if there is an obvious way for driver to 
differentiate

these protocol extensions, and flow item type is redundant, I can
understand the proposal, but otherwise I think better to keep flow 
items

for extensions.


In any case we need the simple VXLAN item.
If we have GPE and GBP specialized items,
what means a match on the simple VXLAN?
Does it include packets with other extensions or exclude them?
Matching the bits in the protocol make such decision explicit.

When a rule is set in HW, HW may not care about the protocol, as 
long as

bits in the rule match with the packet, HW can apply the action.
But for driver to be able to set the rule properly, it needs more
explicit information.


Yes information is in the pattern to match.


Lets assume driver API get a pattern with 'RTE_FLOW_ITEM_TYPE_VXLAN'
type and "struct rte_flow_item_vxlan", at this point driver doesn't 
know

if it is VXLAN or any of the extensions.


Yes it knows because of the matched bits in the pattern.
If the rule specify a match on GBP flag = 1, it i

Re: DTS testpmd and SCAPY integration

2024-02-18 Thread Patrick Robb

Hi Gregory,

I apologize for my delayed response. I have now taken a look through the
code. I do think a number of the design decisions you made for the unit
test framework can be carried over to DTS - in fact we have some tickets
currently for refactoring DTS to this end. Namely:
-Breaking out test agent setup config and test config into separate
files
-Reducing the amount of config values which need to be set by the user,
lowering the barrier for entry for running the testing framework.
-Add some level of testsuite specific YAML which can be mapped to the
testsuite its written for (just for some values, not explicit commands)

But, I figure what you are asking about here really is the design of
setting phases in YAML and writing the scapy/testpmd commands there.
Although I (and I think the DTS group broadly) agree that this system does
provide a good user experience for quickly writing new testsuites, there
are other reasons (testsuite flexibility, platform/TG agnosticism) we don't
want to provide the 100% YAML option for a testsuite. We discussed the idea
of providing two paths for writing a testsuite (YAML file only, or YAML +
python testsuite file), but honestly I don't think it's a good idea to
split our efforts (particularly early on). If we want to consider building
support for a YAML testsuite approach for simple func testsuites in the
(far) future we can discuss it, but that is an unknown. I think our efforts
now need to remain on the python testsuites.

Still, seeing your project has been a big inspiration. When we can abstract
away some boilerplate, or use YAML to override default behavior, we want to
do it. But we still plan to handle sending commands to TG/SUT from the
python testsuite files. I think you are helping us a lot with your ideas,
even if the end implementation is a little different in approach.

Note: incorporating a function(s) to load different mlnx device firmware as
a part of the testsuite is an interesting touch. Do you mind explaining why
it is made a part of the testing framework, as opposed to a pre-step for
the user to complete ahead of running the framework? Does managing loading
the firmware this way make device firmware development and testing it
against DPDK easier, or is there another reason? Thanks.

On Wed, Feb 14, 2024 at 12:27 PM Gregory Etelson 
wrote:

> Hello Patrick,
>
> Did you have time to check the Unit Test design ?
> Do you think it can be used for short functional DTS tests ?
>
> Regards,
> Gregory
>
> --
> *From:* Gregory Etelson
> *Sent:* Wednesday, January 31, 2024 09:43
> *To:* Patrick Robb 
> *Cc:* Gregory Etelson ; Jeremy Spewock <
> jspew...@iol.unh.edu>; NBU-Contact-Thomas Monjalon (EXTERNAL) <
> tho...@monjalon.net>; Honnappa Nagarahalli ;
> Juraj Linkeš ; Paul Szczepanek <
> paul.szczepa...@arm.com>; Yoan Picchi ; Luca
> Vizzarro ; c...@dpdk.org ; dev@dpdk.org
> ; nd ; Maayan Kashani ;
> Asaf Penso 
> *Subject:* Re: DTS testpmd and SCAPY integration
>
> Hello Patrick,
>
> > External email: Use caution opening links or attachments
> > Thank you for sharing Gregory. I did not get an opportunity to look
> through the code today, but I did run
> > through the presentation. A few points I noted:
> > 1. The presentation shows an example testpmd testcase for creating a
> flow rule, and then shows a
> > validation step in which standard out is compared against the expected
> string ("flow rule x created") and
> > we can conclude whether we are able to create flow rules. Are you also
> sending packets according to the
> > flow rules and validating that what is sent/received corresponds to the
> expected behavior of the flow
> > rules? When I look at the old DTS framework, and an example flow rules
> testsuite
> > (https://doc.dpdk.org/dts/test_plans/rte_flow_test_plan.html) which we
> want feature parity with, I think
> > that validation for this testing framework needs to primarily rely on
> comparing packets sent and packets
> > received.
>
> The unit test infrastructure validates flow rule creation and
> a result produced by that flow.
> Flow result is triggered by a packet.
> However, flow result validation does not always can be done by testing a
> packet.
> Unit test implements 2 flow validation methods.
>
> The first validation method tests testpmd output triggered by a test
> packet.
>
> Example: use the MODIFY_FIELD action to copy packet VLAN ID to flow TAG
> item.
> Flow tag is internal flow resource. It must be validated in DPDK
> application.
>
> Test creates 2 flow rules:
>
> Rule 1: use MODIFY_FILED to copy packet VLAN ID to flow TAG item
> pattern eth / vlan / end \
> actions modify_field op set dst_type tag ... src_type vlan_id ... / end
>
> Rule 2: validate the TAG item:
> pattern tag data is 0x31 ... / end actions mark id 0xaaa / rss / end
>
> The test sends a packet with VLAN ID 0x31: / Dot1Q(vlan=0x31) /
> The test matches tespmd output triggered by the packet for
> `FDIR matched ID=0xaaa`.
>

RE: [PATCH v5 3/3] net/mlx5/hws: add compare ESP sequence number support

2024-02-18 Thread Michael Baum

Hi,

> -Original Message-
> From: Suanming Mou 
> Sent: Monday, 19 February 2024 5:00
> To: Michael Baum ; dev@dpdk.org
> Cc: Matan Azrad ; Dariusz Sosnowski
> ; Raslan Darawsheh ; Slava
> Ovsiienko ; Ori Kam 
> Subject: RE: [PATCH v5 3/3] net/mlx5/hws: add compare ESP sequence
> number support
> 
> Hi,
> 
> > -Original Message-
> > From: Michael Baum 
> > Sent: Wednesday, February 14, 2024 3:30 PM
> > To: dev@dpdk.org
> > Cc: Matan Azrad ; Dariusz Sosnowski
> > ; Raslan Darawsheh ; Slava
> > Ovsiienko ; Ori Kam ;
> > Suanming Mou 
> > Subject: [PATCH v5 3/3] net/mlx5/hws: add compare ESP sequence number
> > support
> >
> > Add support for compare item with "RTE_FLOW_FIELD_ESP_SEQ_NUM"
> field.

The basic support of other fields is inserted in this release without 
specifying the supported fields.
I kept the behavior with this field.

> 
> Small comment, please don't forget to add the new supported comparison
> field to rel_notes.
> 
> >
> > Signed-off-by: Michael Baum 
> Acked-by: Suanming Mou 
> 
> > ---
> >  doc/guides/nics/mlx5.rst  |  1 +
> >  drivers/net/mlx5/hws/mlx5dr_definer.c | 22 --
> >  drivers/net/mlx5/mlx5_flow_hw.c   |  3 +++
> >  3 files changed, 24 insertions(+), 2 deletions(-)
> >
> > diff --git a/doc/guides/nics/mlx5.rst b/doc/guides/nics/mlx5.rst index
> > 43ef8a99dc..b793f1ef58 100644
> > --- a/doc/guides/nics/mlx5.rst
> > +++ b/doc/guides/nics/mlx5.rst
> > @@ -823,6 +823,7 @@ Limitations
> >- Only single item is supported per pattern template.
> >- Only 32-bit comparison is supported or 16-bits for random field.
> >- Only supported for ``RTE_FLOW_FIELD_META``,
> > ``RTE_FLOW_FIELD_TAG``,
> > +``RTE_FLOW_FIELD_ESP_SEQ_NUM``,
> >  ``RTE_FLOW_FIELD_RANDOM`` and ``RTE_FLOW_FIELD_VALUE``.
> >- The field type ``RTE_FLOW_FIELD_VALUE`` must be the base (``b``) field.
> >- The field type ``RTE_FLOW_FIELD_RANDOM`` can only be compared
> > with diff --git a/drivers/net/mlx5/hws/mlx5dr_definer.c
> > b/drivers/net/mlx5/hws/mlx5dr_definer.c
> > index 2d86175ca2..b29d7451e7 100644
> > --- a/drivers/net/mlx5/hws/mlx5dr_definer.c
> > +++ b/drivers/net/mlx5/hws/mlx5dr_definer.c
> > @@ -396,10 +396,20 @@ mlx5dr_definer_compare_base_value_set(const
> void
> > *item_spec,
> >
> > value = (const uint32_t *)&b->value[0];
> >
> > -   if (a->field == RTE_FLOW_FIELD_RANDOM)
> > +   switch (a->field) {
> > +   case RTE_FLOW_FIELD_RANDOM:
> > *base = htobe32(*value << 16);
> > -   else
> > +   break;
> > +   case RTE_FLOW_FIELD_TAG:
> > +   case RTE_FLOW_FIELD_META:
> > *base = htobe32(*value);
> > +   break;
> > +   case RTE_FLOW_FIELD_ESP_SEQ_NUM:
> > +   *base = *value;
> > +   break;
> > +   default:
> > +   break;
> > +   }
> >
> > MLX5_SET(ste_match_4dw_range_ctrl_dw, ctrl, base0, 1);  } @@ -
> 2887,6
> > +2897,14 @@ mlx5dr_definer_conv_item_compare_field(const struct
> > rte_flow_field_data *f,
> > fc->compare_idx = dw_offset;
> > DR_CALC_SET_HDR(fc, random_number, random_number);
> > break;
> > +   case RTE_FLOW_FIELD_ESP_SEQ_NUM:
> > +   fc = &cd-
> > >fc[MLX5DR_DEFINER_FNAME_ESP_SEQUENCE_NUMBER];
> > +   fc->item_idx = item_idx;
> > +   fc->tag_set = &mlx5dr_definer_compare_set;
> > +   fc->tag_mask_set = &mlx5dr_definer_ones_set;
> > +   fc->compare_idx = dw_offset;
> > +   DR_CALC_SET_HDR(fc, ipsec, sequence_number);
> > +   break;
> > default:
> > DR_LOG(ERR, "%u field is not supported", f->field);
> > goto err_notsup;
> > diff --git a/drivers/net/mlx5/mlx5_flow_hw.c
> > b/drivers/net/mlx5/mlx5_flow_hw.c index b5741f0817..4d6fb489b2
> 100644
> > --- a/drivers/net/mlx5/mlx5_flow_hw.c
> > +++ b/drivers/net/mlx5/mlx5_flow_hw.c
> > @@ -6725,6 +6725,7 @@ flow_hw_item_compare_field_validate(enum
> > rte_flow_field_id arg_field,
> > switch (arg_field) {
> > case RTE_FLOW_FIELD_TAG:
> > case RTE_FLOW_FIELD_META:
> > +   case RTE_FLOW_FIELD_ESP_SEQ_NUM:
> > break;
> > case RTE_FLOW_FIELD_RANDOM:
> > if (base_field == RTE_FLOW_FIELD_VALUE) @@ -6743,6
> +6744,7 @@
> > flow_hw_item_compare_field_validate(enum rte_flow_field_id arg_field,
> > case RTE_FLOW_FIELD_TAG:
> > case RTE_FLOW_FIELD_META:
> > case RTE_FLOW_FIELD_VALUE:
> > +   case RTE_FLOW_FIELD_ESP_SEQ_NUM:
> > break;
> > default:
> > return rte_flow_error_set(error, ENOTSUP, @@ -6759,6
> +6761,7 @@
> > flow_hw_item_compare_width_supported(enum rte_flow_field_id field)
> > switch (field) {
> > case RTE_FLOW_FIELD_TAG:
> > case RTE_FLOW_FIELD_META:
> > +   case RTE_FLOW_FIELD_ESP_SEQ_NUM:
> > return 32;
> > case RTE_FLOW_FIELD_RANDOM:
> > return 16;
> > --
> > 2.25.1

RE: [PATCH v5 3/3] net/mlx5/hws: add compare ESP sequence number support

2024-02-18 Thread Suanming Mou



> -Original Message-
> From: Michael Baum 
> Sent: Monday, February 19, 2024 3:22 PM
> To: Suanming Mou ; dev@dpdk.org
> Cc: Matan Azrad ; Dariusz Sosnowski
> ; Raslan Darawsheh ; Slava
> Ovsiienko ; Ori Kam 
> Subject: RE: [PATCH v5 3/3] net/mlx5/hws: add compare ESP sequence number
> support
> 
> Hi,
> 
> > -Original Message-
> > From: Suanming Mou 
> > Sent: Monday, 19 February 2024 5:00
> > To: Michael Baum ; dev@dpdk.org
> > Cc: Matan Azrad ; Dariusz Sosnowski
> > ; Raslan Darawsheh ; Slava
> > Ovsiienko ; Ori Kam 
> > Subject: RE: [PATCH v5 3/3] net/mlx5/hws: add compare ESP sequence
> > number support
> >
> > Hi,
> >
> > > -Original Message-
> > > From: Michael Baum 
> > > Sent: Wednesday, February 14, 2024 3:30 PM
> > > To: dev@dpdk.org
> > > Cc: Matan Azrad ; Dariusz Sosnowski
> > > ; Raslan Darawsheh ;
> > > Slava Ovsiienko ; Ori Kam
> > > ; Suanming Mou 
> > > Subject: [PATCH v5 3/3] net/mlx5/hws: add compare ESP sequence
> > > number support
> > >
> > > Add support for compare item with "RTE_FLOW_FIELD_ESP_SEQ_NUM"
> > field.
> 
> The basic support of other fields is inserted in this release without 
> specifying the
> supported fields.
> I kept the behavior with this field.

OK, I see. Thanks.

> 
> >
> > Small comment, please don't forget to add the new supported comparison
> > field to rel_notes.
> >
> > >
> > > Signed-off-by: Michael Baum 
> > Acked-by: Suanming Mou 
> >
> > > ---
> > >  doc/guides/nics/mlx5.rst  |  1 +
> > >  drivers/net/mlx5/hws/mlx5dr_definer.c | 22 --
> > >  drivers/net/mlx5/mlx5_flow_hw.c   |  3 +++
> > >  3 files changed, 24 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/doc/guides/nics/mlx5.rst b/doc/guides/nics/mlx5.rst
> > > index
> > > 43ef8a99dc..b793f1ef58 100644
> > > --- a/doc/guides/nics/mlx5.rst
> > > +++ b/doc/guides/nics/mlx5.rst
> > > @@ -823,6 +823,7 @@ Limitations
> > >- Only single item is supported per pattern template.
> > >- Only 32-bit comparison is supported or 16-bits for random field.
> > >- Only supported for ``RTE_FLOW_FIELD_META``,
> > > ``RTE_FLOW_FIELD_TAG``,
> > > +``RTE_FLOW_FIELD_ESP_SEQ_NUM``,
> > >  ``RTE_FLOW_FIELD_RANDOM`` and ``RTE_FLOW_FIELD_VALUE``.
> > >- The field type ``RTE_FLOW_FIELD_VALUE`` must be the base (``b``) 
> > > field.
> > >- The field type ``RTE_FLOW_FIELD_RANDOM`` can only be compared
> > > with diff --git a/drivers/net/mlx5/hws/mlx5dr_definer.c
> > > b/drivers/net/mlx5/hws/mlx5dr_definer.c
> > > index 2d86175ca2..b29d7451e7 100644
> > > --- a/drivers/net/mlx5/hws/mlx5dr_definer.c
> > > +++ b/drivers/net/mlx5/hws/mlx5dr_definer.c
> > > @@ -396,10 +396,20 @@ mlx5dr_definer_compare_base_value_set(const
> > void
> > > *item_spec,
> > >
> > >   value = (const uint32_t *)&b->value[0];
> > >
> > > - if (a->field == RTE_FLOW_FIELD_RANDOM)
> > > + switch (a->field) {
> > > + case RTE_FLOW_FIELD_RANDOM:
> > >   *base = htobe32(*value << 16);
> > > - else
> > > + break;
> > > + case RTE_FLOW_FIELD_TAG:
> > > + case RTE_FLOW_FIELD_META:
> > >   *base = htobe32(*value);
> > > + break;
> > > + case RTE_FLOW_FIELD_ESP_SEQ_NUM:
> > > + *base = *value;
> > > + break;
> > > + default:
> > > + break;
> > > + }
> > >
> > >   MLX5_SET(ste_match_4dw_range_ctrl_dw, ctrl, base0, 1);  } @@ -
> > 2887,6
> > > +2897,14 @@ mlx5dr_definer_conv_item_compare_field(const struct
> > > rte_flow_field_data *f,
> > >   fc->compare_idx = dw_offset;
> > >   DR_CALC_SET_HDR(fc, random_number, random_number);
> > >   break;
> > > + case RTE_FLOW_FIELD_ESP_SEQ_NUM:
> > > + fc = &cd-
> > > >fc[MLX5DR_DEFINER_FNAME_ESP_SEQUENCE_NUMBER];
> > > + fc->item_idx = item_idx;
> > > + fc->tag_set = &mlx5dr_definer_compare_set;
> > > + fc->tag_mask_set = &mlx5dr_definer_ones_set;
> > > + fc->compare_idx = dw_offset;
> > > + DR_CALC_SET_HDR(fc, ipsec, sequence_number);
> > > + break;
> > >   default:
> > >   DR_LOG(ERR, "%u field is not supported", f->field);
> > >   goto err_notsup;
> > > diff --git a/drivers/net/mlx5/mlx5_flow_hw.c
> > > b/drivers/net/mlx5/mlx5_flow_hw.c index b5741f0817..4d6fb489b2
> > 100644
> > > --- a/drivers/net/mlx5/mlx5_flow_hw.c
> > > +++ b/drivers/net/mlx5/mlx5_flow_hw.c
> > > @@ -6725,6 +6725,7 @@ flow_hw_item_compare_field_validate(enum
> > > rte_flow_field_id arg_field,
> > >   switch (arg_field) {
> > >   case RTE_FLOW_FIELD_TAG:
> > >   case RTE_FLOW_FIELD_META:
> > > + case RTE_FLOW_FIELD_ESP_SEQ_NUM:
> > >   break;
> > >   case RTE_FLOW_FIELD_RANDOM:
> > >   if (base_field == RTE_FLOW_FIELD_VALUE) @@ -6743,6
> > +6744,7 @@
> > > flow_hw_item_compare_field_validate(enum rte_flow_field_id arg_field,
> > >   case RTE_FLOW_FIELD_TAG:
> > >   case RTE_FLOW_FIELD_META:
> > >   case RTE_FLOW_FIELD_VALUE:
> > > + case RTE_FLOW_FIELD_ESP_SEQ_NUM:
> > >   break;
> > >   d

Re: [RFC 1/5] eal: add static per-lcore memory allocation facility

2024-02-18 Thread Mattias Rönnblom

On 2024-02-09 14:04, Morten Brørup wrote:

From: Mattias Rönnblom [mailto:hof...@lysator.liu.se]
Sent: Friday, 9 February 2024 12.46

On 2024-02-09 09:25, Morten Brørup wrote:

From: Mattias Rönnblom [mailto:mattias.ronnb...@ericsson.com]
Sent: Thursday, 8 February 2024 19.17

Introduce DPDK per-lcore id variables, or lcore variables for short.

An lcore variable has one value for every current and future lcore
id-equipped thread.

The primary  use case is for statically allocating
small chunks of often-used data, which is related logically, but

where

there are performance benefits to reap from having updates being

local

to an lcore.

Lcore variables are similar to thread-local storage (TLS, e.g., C11
_Thread_local), but decoupling the values' life time with that of

the

threads.

Lcore variables are also similar in terms of functionality provided

by

FreeBSD kernel's DPCPU_*() family of macros and the associated
build-time machinery. DPCPU uses linker scripts, which effectively
prevents the reuse of its, otherwise seemingly viable, approach.

The currently-prevailing way to solve the same problem as lcore
variables is to keep a module's per-lcore data as RTE_MAX_LCORE-

sized

array of cache-aligned, RTE_CACHE_GUARDed structs. The benefit of
lcore variables over this approach is that data related to the same
lcore now is close (spatially, in memory), rather than data used by
the same module, which in turn avoid excessive use of padding,
polluting caches with unused data.

Signed-off-by: Mattias Rönnblom 
---

This looks very promising. :-)

Here's a bunch of comments, questions and suggestions.

* Question: Performance.
What is the cost of accessing an lcore variable vs a variable in TLS?
I suppose the relative cost diminishes if the variable is a larger

struct, compared to a simple uint64_t.

In case all the relevant data is available in a cache close to the
core,
both options carry quite low overhead.

Accessing a lcore variable will always require a TLS lookup, in the
form
of retrieving the lcore_id of the current thread. In that sense, there
will likely be a number of extra instructions required to do the lcore
variable address lookup (i.e., doing the load from rte_lcore_var table
based on the lcore_id you just looked up, and adding the variable's
offset).

A TLS lookup will incur an extra overhead of less than a clock cycle,
compared to accessing a non-TLS static variable, in case static linking
is used. For shared objects, TLS is much more expensive (something
often
visible in dynamically linked DPDK app flame graphs, in the form of the
__tls_addr symbol). Then you need to add ~3 cc/access. This on a micro
benchmark running on a x86_64 Raptor Lake P-core.

(To visialize the difference between shared object and not, one can use
Compiler Explorer and -fPIC versus -fPIE.)

Things get more complicated if you access the same variable in the same
section code, since then it can be left on the stack/in a register by
the compiler, especially if LTO is used. In other words, if you do
rte_lcore_id() several times in a row, only the first one will cost you
anything. This happens fairly often in DPDK, with rte_lcore_id().

Finally, if you do something like

diff --git a/lib/eal/common/rte_random.c b/lib/eal/common/rte_random.c
index af9fffd81b..a65c30d27e 100644
--- a/lib/eal/common/rte_random.c
+++ b/lib/eal/common/rte_random.c
@@ -125,14 +125,7 @@ __rte_rand_lfsr258(struct rte_rand_state *state)
   static __rte_always_inline
   struct rte_rand_state *__rte_rand_get_state(void)
   {
-   unsigned int idx;
-
-   idx = rte_lcore_id();
-
-   if (unlikely(idx == LCORE_ID_ANY))
-   return &unregistered_rand_state;
-
-   return RTE_LCORE_VAR_PTR(rand_state);
+   return &unregistered_rand_state;
   }

   uint64_t

...and re-run the rand_perf_autotest, at least I see no difference at
all (in a statically linked build). Both results in rte_rand() using
~11
cc/call. What that suggests is that TLS overhead is very small, and
that
any extra instructions required by lcore variables doesn't add much, if
anything at all, at least in this particular case.

Excellent. Thank you for a thorough and detailed answer, Mattias.

Some of my suggestions below might also affect performance.

* Advantage: Provides direct access to worker thread variables.
With the current alternative (thread-local storage), the main thread

cannot access the TLS variables of the worker threads,

unless worker threads publish global access pointers.
Lcore variables of any lcore thread can be direcctly accessed by any

thread, which simplifies code.

* Advantage: Roadmap towards hugemem.
It would be nice if the lcore variable memory was allocated in

hugemem, to reduce TLB misses.

The current alternative (thread-local storage) is also not using

hugement, so not a degradation.

I agree, but the thing is it's hard to figure out how much memory is
required for these kind of variables, given how DPDK is

54 matches

Mail list logo