Re: [EXT] Re: [PATCH 1/2] app/test: add support for optional dependencies
On Thu, Sep 28, 2023 at 3:06 PM Akhil Goyal wrote: > > > Some tests make optionally use a component but don't require it for > > > > s/make // > > > > > building. If we include the dependency in the per-file lists, then tests > > > may be unnecessarily omitted, as the dependency is not required. > > > On the other hand, removing the optional dependency from the list can > > > cause build failures, as the test case may include the optional code, > > > but then fail to build as the build system won't have added the necessary > > > paths for header inclusion, or the necessary libraries for linking. > > > > > > Resolve this by explicitly providing a list of optional dependencies. > > > Any items in this list will be added to the dependency list if > > > available, but otherwise won't be involved in enable/disable of specific > > > tests. > > > > > > Signed-off-by: Bruce Richardson > > Reviewed-by: David Marchand > Tested-by: Akhil Goyal Series applied, thanks for the quick fix Bruce. -- David Marchand
Re: [PATCH v6 1/6] eal: provide rte stdatomics optional atomics API
On Thu, Sep 28, 2023 at 10:06 AM Thomas Monjalon wrote:
>
> 22/08/2023 23:00, Tyler Retzlaff:
> > --- a/lib/eal/include/generic/rte_rwlock.h
> > +++ b/lib/eal/include/generic/rte_rwlock.h
> > @@ -32,6 +32,7 @@
> > #include
> > #include
> > #include
> > +#include
>
> I'm not sure about adding the include in patch 1 if it is not used here.
Yes, this is something I had already fixed locally.
>
> > --- /dev/null
> > +++ b/lib/eal/include/rte_stdatomic.h
> > @@ -0,0 +1,198 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(c) 2023 Microsoft Corporation
> > + */
> > +
> > +#ifndef _RTE_STDATOMIC_H_
> > +#define _RTE_STDATOMIC_H_
> > +
> > +#include
> > +
> > +#ifdef __cplusplus
> > +extern "C" {
> > +#endif
> > +
> > +#ifdef RTE_ENABLE_STDATOMIC
> > +#ifdef __STDC_NO_ATOMICS__
> > +#error enable_stdatomics=true but atomics not supported by toolchain
> > +#endif
> > +
> > +#include
> > +
> > +/* RTE_ATOMIC(type) is provided for use as a type specifier
> > + * permitting designation of an rte atomic type.
> > + */
> > +#define RTE_ATOMIC(type) _Atomic(type)
> > +
> > +/* __rte_atomic is provided for type qualification permitting
> > + * designation of an rte atomic qualified type-name.
>
> Sorry I don't understand this comment.
The difference between atomic qualifier and atomic specifier and the
need for exposing those two notions are not obvious to me.
One clue I have is with one use later in the series:
+rte_mcslock_lock(RTE_ATOMIC(rte_mcslock_t *) *msl, rte_mcslock_t *me)
...
+prev = rte_atomic_exchange_explicit(msl, me, rte_memory_order_acq_rel);
So at least RTE_ATOMIC() seems necessary.
>
> > + */
> > +#define __rte_atomic _Atomic
> > +
> > +/* The memory order is an enumerated type in C11. */
> > +typedef memory_order rte_memory_order;
> > +
> > +#define rte_memory_order_relaxed memory_order_relaxed
> > +#ifdef __ATOMIC_RELAXED
> > +static_assert(rte_memory_order_relaxed == __ATOMIC_RELAXED,
> > + "rte_memory_order_relaxed == __ATOMIC_RELAXED");
>
> Not sure about using static_assert or RTE_BUILD_BUG_ON
Do you mean you want no check at all in a public facing header?
Or is it that we have RTE_BUILD_BUG_ON and we should keep using it
instead of static_assert?
I remember some problems with RTE_BUILD_BUG_ON where the compiler
would silently drop the whole expression and reported no problem as it
could not evaluate the expression.
At least, with static_assert (iirc, it is new to C11) the compiler
complains with a clear "error: expression in static assertion is not
constant".
We could fix RTE_BUILD_BUG_ON, but I guess the fix would be equivalent
to map it to static_assert(!condition).
Using language standard constructs seems a better choice.
--
David Marchand
Re: [PATCH v6 1/6] eal: provide rte stdatomics optional atomics API
On Fri, Sep 29, 2023 at 10:54 AM Morten Brørup wrote: > In my opinion, our move to C11 thus makes RTE_BUILD_BUG_ON obsolete. That's my thought too. > > We should mark RTE_BUILD_BUG_ON as deprecated, and disallow RTE_BUILD_BUG_ON > in new code. Perhaps checkpatches could catch this? For a clear deprecation of a part of DPDK API, I don't see a need to add something in checkpatch. Putting a RTE_DEPRECATED in RTE_BUILD_BUG_ON directly triggers a build warning (caught by CI since we run with Werror). diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h index 771c70f2c8..40542629c1 100644 --- a/lib/eal/include/rte_common.h +++ b/lib/eal/include/rte_common.h @@ -495,7 +495,7 @@ rte_is_aligned(const void * const __rte_restrict ptr, const unsigned int align) /** * Triggers an error at compilation time if the condition is true. */ -#define RTE_BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)])) +#define RTE_BUILD_BUG_ON(condition) RTE_DEPRECATED(RTE_BUILD_BUG_ON) ((void)sizeof(char[1 - 2*!!(condition)])) /*** Cache line related macros / $ ninja -C build-mini ... [18/333] Compiling C object lib/librte_eal.a.p/eal_common_eal_common_trace.c.o ../lib/eal/common/eal_common_trace.c: In function ‘eal_trace_init’: ../lib/eal/common/eal_common_trace.c:44:20: warning: "RTE_BUILD_BUG_ON" is deprecated 44 | RTE_BUILD_BUG_ON((offsetof(struct __rte_trace_header, mem) % 8) != 0); |^~~~ [38/333] Compiling C object lib/librte_eal.a.p/eal_common_malloc_heap.c.o ../lib/eal/common/malloc_heap.c: In function ‘malloc_heap_destroy’: ../lib/eal/common/malloc_heap.c:1398:20: warning: "RTE_BUILD_BUG_ON" is deprecated 1398 | RTE_BUILD_BUG_ON(offsetof(struct malloc_heap, lock) != 0); |^~~~ [50/333] Compiling C object lib/librte_eal.a.p/eal_unix_rte_thread.c.o ../lib/eal/unix/rte_thread.c: In function ‘rte_thread_self’: ../lib/eal/unix/rte_thread.c:239:20: warning: "RTE_BUILD_BUG_ON" is deprecated 239 | RTE_BUILD_BUG_ON(sizeof(pthread_t) > sizeof(uintptr_t)); | ^~~~ -- David Marchand
Re: [PATCH v6 1/6] eal: provide rte stdatomics optional atomics API
On Fri, Sep 29, 2023 at 11:26 AM Bruce Richardson wrote: > > On Fri, Sep 29, 2023 at 11:02:38AM +0200, David Marchand wrote: > > On Fri, Sep 29, 2023 at 10:54 AM Morten Brørup > > wrote: > > > In my opinion, our move to C11 thus makes RTE_BUILD_BUG_ON obsolete. > > > > That's my thought too. > > > > > > > > We should mark RTE_BUILD_BUG_ON as deprecated, and disallow > > > RTE_BUILD_BUG_ON in new code. Perhaps checkpatches could catch this? > > > > For a clear deprecation of a part of DPDK API, I don't see a need to > > add something in checkpatch. > > Putting a RTE_DEPRECATED in RTE_BUILD_BUG_ON directly triggers a build > > warning (caught by CI since we run with Werror). > > > > Would it not be sufficient to just make it an alias for the C11 static > assertions? It's not like its a lot of code to maintain, and if app users > have it in their code I'm not sure we get massive benefit from forcing them > to edit their code. I'd rather see it kept as a one-line macro purely from > a backward compatibility viewpoint. We can replace internal usages, though > - which can be checked by checkpatch. No, there is no massive benefit, just trying to reduce our ever growing API surface. Note, this macro should have been kept internal but it was introduced at a time such matter was not considered... -- David Marchand
Re: [PATCH v6 1/6] eal: provide rte stdatomics optional atomics API
On Fri, Sep 29, 2023 at 12:26 PM Thomas Monjalon wrote: > > 29/09/2023 11:34, David Marchand: > > On Fri, Sep 29, 2023 at 11:26 AM Bruce Richardson > > wrote: > > > > > > On Fri, Sep 29, 2023 at 11:02:38AM +0200, David Marchand wrote: > > > > On Fri, Sep 29, 2023 at 10:54 AM Morten Brørup > > > > wrote: > > > > > In my opinion, our move to C11 thus makes RTE_BUILD_BUG_ON obsolete. > > > > > > > > That's my thought too. > > > > > > > > > > > > > > We should mark RTE_BUILD_BUG_ON as deprecated, and disallow > > > > > RTE_BUILD_BUG_ON in new code. Perhaps checkpatches could catch this? > > > > > > > > For a clear deprecation of a part of DPDK API, I don't see a need to > > > > add something in checkpatch. > > > > Putting a RTE_DEPRECATED in RTE_BUILD_BUG_ON directly triggers a build > > > > warning (caught by CI since we run with Werror). > > > > > > > > > > Would it not be sufficient to just make it an alias for the C11 static > > > assertions? It's not like its a lot of code to maintain, and if app users > > > have it in their code I'm not sure we get massive benefit from forcing > > > them > > > to edit their code. I'd rather see it kept as a one-line macro purely from > > > a backward compatibility viewpoint. We can replace internal usages, though > > > - which can be checked by checkpatch. > > > > No, there is no massive benefit, just trying to reduce our ever > > growing API surface. > > > > Note, this macro should have been kept internal but it was introduced > > at a time such matter was not considered... > > I agree with all. > Now taking techboard hat, we agreed to avoid breaking API if possible. > So we should keep RTE_BUILD_BUG_ON forever even if not used. > Internally we can replace its usages. So back to the original topic, I get that static_assert is ok for this patch. -- David Marchand
Re: [PATCH v6 0/6] rte atomics API for optional stdatomic
Hello, On Tue, Aug 22, 2023 at 11:00 PM Tyler Retzlaff wrote: > > This series introduces API additions prefixed in the rte namespace that allow > the optional use of stdatomics.h from C11 using enable_stdatomics=true for > targets where enable_stdatomics=false no functional change is intended. > > Be aware this does not contain all changes to use stdatomics across the DPDK > tree it only introduces the minimum to allow the option to be used which is > a pre-requisite for a clean CI (probably using clang) that can be run > with enable_stdatomics=true enabled. > > It is planned that subsequent series will be introduced per lib/driver as > appropriate to further enable stdatomics use when enable_stdatomics=true. > > Notes: > > * Additional libraries beyond EAL make visible atomics use across the > API/ABI surface they will be converted in the subsequent series. > > * The eal: add rte atomic qualifier with casts patch needs some discussion > as to whether or not the legacy rte_atomic APIs should be converted to > work with enable_stdatomic=true right now some implementation dependent > casts are used to prevent cascading / having to convert too much in > the intial series. > > * Windows will obviously need complete conversion of libraries including > atomics that are not crossing API/ABI boundaries. those conversions will > introduced in separate series as new along side the existing msvc series. > > Please keep in mind we would like to prioritize the review / acceptance of > this patch since it needs to be completed in the 23.11 merge window. > > Thank you all for the discussion that lead to the formation of this series. I did a number of updates on this v6: - moved rte_stdatomic.h from patch 1 to later patches where needed, - added a RN entry, - tried to consistently/adjusted indent, - fixed mentions of stdatomic*s* to simple atomic, like in the build option name, - removed unneded comments (Thomas review on patch 1), Series applied, thanks Tyler. Two things are missing: - add doxygen tags in the new API (this can be fixed later in this release, can you look at it?), - add compilation tests for enable_stdatomic (I'll send a patch soon for devtools and GHA), -- David Marchand
[PATCH] ci: test stdatomic API
Add some compilation tests with C11 atomics enabled.
The headers check can't be enabled (as gcc and clang don't provide
stdatomic before C++23).
Signed-off-by: David Marchand
---
.ci/linux-build.sh| 6 +-
.github/workflows/build.yml | 8
devtools/test-meson-builds.sh | 3 +++
3 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh
index e0b62bac90..b09df07b55 100755
--- a/.ci/linux-build.sh
+++ b/.ci/linux-build.sh
@@ -92,7 +92,11 @@ fi
OPTS="$OPTS -Dplatform=generic"
OPTS="$OPTS -Ddefault_library=$DEF_LIB"
OPTS="$OPTS -Dbuildtype=$buildtype"
-OPTS="$OPTS -Dcheck_includes=true"
+if [ "$STDATOMIC" = "true" ]; then
+ OPTS="$OPTS -Denable_stdatomic=true"
+else
+ OPTS="$OPTS -Dcheck_includes=true"
+fi
if [ "$MINI" = "true" ]; then
OPTS="$OPTS -Denable_drivers=net/null"
OPTS="$OPTS -Ddisable_libs=*"
diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
index 7a2ac0ceee..14328622fb 100644
--- a/.github/workflows/build.yml
+++ b/.github/workflows/build.yml
@@ -30,6 +30,7 @@ jobs:
REF_GIT_TAG: none
RISCV64: ${{ matrix.config.cross == 'riscv64' }}
RUN_TESTS: ${{ contains(matrix.config.checks, 'tests') }}
+ STDATOMIC: ${{ contains(matrix.config.checks, 'stdatomic') }}
strategy:
fail-fast: false
@@ -38,6 +39,12 @@ jobs:
- os: ubuntu-20.04
compiler: gcc
mini: mini
+ - os: ubuntu-20.04
+compiler: gcc
+checks: stdatomic
+ - os: ubuntu-20.04
+compiler: clang
+checks: stdatomic
- os: ubuntu-20.04
compiler: gcc
checks: debug+doc+examples+tests
@@ -241,6 +248,7 @@ jobs:
> ~/env
echo CC=ccache ${{ matrix.config.compiler }} >> ~/env
echo DEF_LIB=${{ matrix.config.library }} >> ~/env
+echo STDATOMIC=false >> ~/env
- name: Load the cached image
run: |
docker load -i ~/.image/${{ matrix.config.image }}.tar
diff --git a/devtools/test-meson-builds.sh b/devtools/test-meson-builds.sh
index c41659d28b..ca32e3d5a5 100755
--- a/devtools/test-meson-builds.sh
+++ b/devtools/test-meson-builds.sh
@@ -239,6 +239,9 @@ done
build build-mini cc skipABI $use_shared -Ddisable_libs=* \
-Denable_drivers=net/null
+build build-gcc-shared-stdatomic gcc skipABI -Denable_stdatomic=true
$use_shared
+build build-clang-shared-stdatomic clang skipABI -Denable_stdatomic=true
$use_shared
+
# test compilation with minimal x86 instruction set
# Set the install path for libraries to "lib" explicitly to prevent problems
# with pkg-config prefixes if installed in "lib/x86_64-linux-gnu" later.
--
2.41.0
Re: [PATCH 0/2] Add eventdev tests to test suites
On Thu, Sep 28, 2023 at 5:14 PM Bruce Richardson wrote: > > The eventdev library includes a selftest API which can be used by > drivers for testing. Add the relevant automated self-test commands > into meson test suites as appropriate. > > Bruce Richardson (2): > event/sw: add self tests to fast tests > event/*: add driver selftests to driver-tests suite > > app/test/test_eventdev.c | 10 +- > drivers/event/sw/sw_evdev_selftest.c | 2 +- > 2 files changed, 6 insertions(+), 6 deletions(-) > On the principle, the series lgtm. Acked-by: David Marchand -- David Marchand
Re: [PATCH] eal/linux: prevent deadlocks on rte init and cleanup
Hello Jonathan, On Thu, Jul 20, 2023 at 7:19 AM Jonathan Erb wrote: > > Resolves a deadlock that can occur when multiple secondary > processes are starting and stopping. A deadlock can occur because > eal_memalloc_init() is taking a second unnecessary read lock. > If another DPDK process that is terminating enters rte_eal_memory_detach() > and acquires a write lock wait state before the starting process can > acquire it's second read lock then no process will be able to proceed. > > Cc: sta...@dpdk.org > > Signed-off-by: Jonathan Erb Thanks for the report and fix and sorry for the late reply. Artemy came with a similar report and a more complete fix. Could you confirm it works for you? https://patchwork.dpdk.org/project/dpdk/list/?series=29463&state=%2A&archive=both Thanks, -- David Marchand
Re: [PATCH] gpu: add support for rtx 6000 variant
Hello, On Fri, Apr 29, 2022 at 6:53 PM Cliff Burdick wrote: > > Added second GPU PCI device ID for RTX 6000 > > Signed-off-by: Cliff Burdick > --- > drivers/gpu/cuda/cuda.c| 6 +- > drivers/gpu/cuda/devices.h | 3 ++- > 2 files changed, 7 insertions(+), 2 deletions(-) Is this patch still wanted? It seems it fell through the cracks. Cc: maintainers. -- David Marchand
Re: [PATCH] gpu/cuda: Add missing stdlib include
On Tue, Sep 26, 2023 at 8:24 PM Aaron Conole wrote:
>
>
> From: John Romein
>
> getenv needs stdlib.h to be included.
>
> Bugzilla ID: 1133
>
> Fixes: 24c77594e08f ("gpu/cuda: map GPU memory with GDRCopy")
> Signed-off-by: John Romein
Thanks for the patch, it seems to be a duplicate of the prior patch
sent by Levend:
https://patchwork.dpdk.org/project/dpdk/patch/20230803162512.41396-1-levendsa...@gmail.com/
--
David Marchand
Re: [PATCH] gpu/cuda: fix getenv related build error
On Thu, Aug 3, 2023 at 6:25 PM Levend Sayar wrote:
>
> If gdrapi.h is available, meson sets DRIVERS_GPU_CUDA_GDRCOPY_H as 1.
> This causes gdrcopy.c build to give an error;
> because compiler can not find signature of getenv.
> stdlib.h is included for the definition of getenv function.
>
There was a bug report for this issue:
Bugzilla ID: 1133
> Fixes: ca12f5e8a7db ("gpu/cuda: mark unused GDRCopy functions parameters")
It is probably worth backporting:
Cc: sta...@dpdk.org
>
> Signed-off-by: Levend Sayar
Elena, this is a quick one, review please.
--
David Marchand
Re: [PATCH v3] bus/cdx: provide driver flag for optional resource mapping
On Tue, Jul 11, 2023 at 7:52 AM Abhijit Gangurde
wrote:
> @@ -383,10 +384,12 @@ cdx_probe_one_driver(struct rte_cdx_driver *dr,
> CDX_BUS_DEBUG(" probe device %s using driver: %s", dev_name,
> dr->driver.name);
>
> - ret = cdx_vfio_map_resource(dev);
> - if (ret != 0) {
> - CDX_BUS_ERR("CDX map device failed: %d", ret);
> - goto error_map_device;
> + if (dr->drv_flags & RTE_CDX_DRV_NEED_MAPPING) {
> + ret = cdx_vfio_map_resource(dev);
> + if (ret != 0) {
> + CDX_BUS_ERR("CDX map device failed: %d", ret);
> + goto error_map_device;
> + }
> }
>
> /* call the driver probe() function */
> diff --git a/drivers/bus/cdx/rte_bus_cdx.h b/drivers/bus/cdx/rte_bus_cdx.h
> new file mode 100644
> index 00..4ca12f90c4
> --- /dev/null
> +++ b/drivers/bus/cdx/rte_bus_cdx.h
> @@ -0,0 +1,52 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright (C) 2023, Advanced Micro Devices, Inc.
> + */
> +
> +#ifndef RTE_BUS_CDX_H
> +#define RTE_BUS_CDX_H
> +
> +/**
> + * @file
> + * CDX device & driver interface
> + */
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +/* Forward declarations */
> +struct rte_cdx_device;
> +
> +/**
> + * Map the CDX device resources in user space virtual memory address.
> + *
> + * Note that driver should not call this function when flag
> + * RTE_CDX_DRV_NEED_MAPPING is set, as EAL will do that for
> + * you when it's on.
Why should we export this function in the application ABI, if it is
only used by drivers?
> + *
> + * @param dev
> + * A pointer to a rte_cdx_device structure describing the device
> + * to use.
> + *
> + * @return
> + * 0 on success, negative on error and positive if no driver
> + * is found for the device.
> + */
> +__rte_experimental
> +int rte_cdx_map_device(struct rte_cdx_device *dev);
>
--
David Marchand
Re: [PATCH v2 0/2] update rsu implementation
Hello, On Fri, Jun 10, 2022 at 4:17 AM Wei Huang wrote: > > The first patch introduce PMCI driver to provide interface to access > PMCI functions which include flash controller. > The second patch update RSU (Remote System Update) implementation > to adapt with PMCI controller. Is this series still relevant? If so, it needs some rebasing. Thanks. -- David Marchand
Re: [PATCH] hash: fix SSE comparison
On Wed, Sep 6, 2023 at 4:31 AM Jieqiang Wang wrote:
>
> __mm_cmpeq_epi16 returns 0x if the corresponding 16-bit elements are
> equal. In original SSE2 implementation for function compare_signatures,
> it utilizes _mm_movemask_epi8 to create mask from the MSB of each 8-bit
> element, while we should only care about the MSB of lower 8-bit in each
> 16-bit element.
> For example, if the comparison result is all equal, SSE2 path returns
> 0x while NEON and default scalar path return 0x.
> Although this bug is not causing any negative effects since the caller
> function solely examines the trailing zeros of each match mask, we
> recommend this fix to ensure consistency with NEON and default scalar
> code behaviors.
>
> Fixes: c7d93df552c2 ("hash: use partial-key hashing")
> Cc: yipeng1.w...@intel.com
> Cc: sta...@dpdk.org
>
> Signed-off-by: Feifei Wang
> Signed-off-by: Jieqiang Wang
> Reviewed-by: Ruifeng Wang
A review from this library maintainers please?
--
David Marchand
Re: [PATCH 1/1] hash: add SVE support for bulk key lookup
On Thu, Aug 17, 2023 at 11:24 PM Harjot Singh wrote: > > From: Harjot Singh > > - Implemented Vector Length Agnostic SVE code for comparing signatures > in bulk lookup. > - Added Defines in code for SVE code support. > - New Optimised SVE code is 1-2 CPU cycle slower than NEON for N2 > processor. > > Performance Numbers from hash_perf_autotest : > > Elements in Primary or Secondary Location > > Results (in CPU cycles/operation) > --- > Operations without data > > Without pre-computed hash values > > Keysize Add/Lookup/Lookup_bulk > Neon SVE > 4 93/71/26 93/71/27 > 8 93/70/26 93/70/27 > 9 94/74/27 94/74/28 > 13 100/80/31100/79/32 > 16 100/78/30100/78/31 > 32 109/110/38 108/110/39 > > With pre-computed hash values > > Keysize Add/Lookup/Lookup_bulk > Neon SVE > 4 83/58/27 83/58/29 > 8 83/57/27 83/57/28 > 9 83/60/28 83/60/29 > 13 84/60/28 83/60/29 > 16 83/58/27 83/58/29 > 32 84/68/31 84/68/32 > > Signed-off-by: Harjot Singh > Reviewed-by: Nathan Brown > Reviewed-by: Feifei Wang > Reviewed-by: Jieqiang Wang > Reviewed-by: Honnappa Nagarahalli Thanks for the patch, please update the release notes. -- David Marchand
Re: [PATCH v9] hash: add XOR32 hash function
On Tue, Jul 11, 2023 at 12:00 AM Bili Dong wrote: > > An XOR32 hash is needed in the Software Switch (SWX) Pipeline for its > use case in P4. We implement it in this patch so it could be easily > registered in the pipeline later. > > Signed-off-by: Bili Dong Review, please. -- David Marchand
Re: [PATCH] lib/hash: new feature adding existing key
On Mon, Mar 13, 2023 at 8:36 AM Abdullah Ömer Yamaç wrote: > > In some use cases inserting data with the same key shouldn't be > overwritten. We use a new flag in this patch to disable overwriting > data for the same key. > > Signed-off-by: Abdullah Ömer Yamaç If this patch is still relevant, please rebase it and send a new revision. Don't forget to copy the library maintainers. -- David Marchand
Re: [PATCH] dumpcap: fix mbuf pool ring type
On Fri, Aug 4, 2023 at 6:16 PM Stephen Hemminger
wrote:
>
> The ring used to store mbufs needs to be multiple producer,
> multiple consumer because multiple queues might on multiple
> cores might be allocating and same time (consume) and in
> case of ring full, the mbufs will be returned (multiple producer).
I think I get the idea, but can you rephrase please?
>
> Bugzilla ID: 1271
> Fixes: cb2440fd77af ("dumpcap: fix mbuf pool ring type")
This Fixes: tag looks wrong.
> Signed-off-by: Stephen Hemminger
> ---
> app/dumpcap/main.c | 7 +++
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/app/dumpcap/main.c b/app/dumpcap/main.c
> index 64294bbfb3e6..991174e95022 100644
> --- a/app/dumpcap/main.c
> +++ b/app/dumpcap/main.c
> @@ -691,10 +691,9 @@ static struct rte_mempool *create_mempool(void)
> data_size = mbuf_size;
> }
>
> - mp = rte_pktmbuf_pool_create_by_ops(pool_name, num_mbufs,
> - MBUF_POOL_CACHE_SIZE, 0,
> - data_size,
> - rte_socket_id(), "ring_mp_sc");
> + mp = rte_pktmbuf_pool_create(pool_name, num_mbufs,
> +MBUF_POOL_CACHE_SIZE, 0,
> +data_size, rte_socket_id());
Switching to rte_pktmbuf_pool_create() still leaves the user with the
possibility to shoot himself in the foot (I was thinking of setting
some --mbuf-pool-ops-name EAL option).
This application has explicit requirements in terms of concurrent
access (and I don't think the mempool library exposes per driver
capabilities in that regard).
The application was enforcing the use of mempool/ring so far.
I think it is safer to go with an explicit
rte_pktmbuf_pool_create_by_ops(... "ring_mp_mc").
WDYT?
> if (mp == NULL)
> rte_exit(EXIT_FAILURE,
> "Mempool (%s) creation failed: %s\n", pool_name,
> --
> 2.39.2
>
Thanks.
--
David Marchand
Re: [PATCH 4/4] pcapng: move timestamp calculation into pdump
Hello Stephen,
On Thu, Sep 21, 2023 at 6:24 AM Stephen Hemminger
wrote:
>
> The computation of timestamp is more easily done in pdump
> than pcapng. The initialization is easier and makes the pcapng
> library have no global state.
>
> It also makes it easier to add HW timestamp support later.
>
> Simplify the computation of nanoseconds from TSC to a two
> step process which avoids numeric overflow issues. The previous
> code was not thread safe as well.
>
Bugzilla ID: 1291 ?
This patch (and patch 3) updates some pcapng API, is it worth a RN update?
> Fixes: c882eb544842 ("pcapng: fix timestamp wrapping in output files")
Is it worth backporting?
I would say no, as some API update was needed to fix the issue.
But on the other hand, this is an experimental API, so I prefer to ask.
> Signed-off-by: Stephen Hemminger
--
David Marchand
Re: [PATCH v1 1/1] eal/random: fix random state initialization for non-eal threads
Hello guys,
On Mon, Aug 28, 2023 at 2:07 PM Anatoly Burakov
wrote:
>
> Currently, the rte_rand() state is initialized with seed, and each
> rand state is initialized up until RTE_MAX_LCORE'th rand state. However,
> rand state also has one extra rand state reserved for non-EAL threads,
> which is not initialized. Fix it by initializing this extra state.
>
> Fixes: 3f002f069612 ("eal: replace libc-based random generation with LFSR")
> Cc: mattias.ronnb...@ericsson.com
> Cc: sta...@dpdk.org
>
> Signed-off-by: Anatoly Burakov
We have two series for fixing related issues.
Stephen alternative patch 1 handles Anatoly fix here.
https://patchwork.dpdk.org/project/dpdk/list/?series=29449&state=%2A&archive=both
I see Anatoly was acked by Mattias and Morten, though Stephen
(RTE_DIM) fix is more elegant.
How do you guys want me to proceed?
Thanks.
--
David Marchand
Re: [PATCH] bus/platform: fix use after free
On Thu, Aug 24, 2023 at 12:52 PM Tomasz Duszynski
wrote:
>
> Remove device from the list before doing actual cleanup to avoid use
> after free.
>
> Bugzilla ID: 1276
> Fixes: 17c839f74da3 ("bus: add platform bus")
>
> Signed-off-by: Tomasz Duszynski
Applied, thanks.
--
David Marchand
Re: [PATCH] rawdev: fix device naming
On Mon, Jul 24, 2023 at 3:37 PM Tomasz Duszynski wrote:
>
> Use proper naming when dealing with a raw device.
>
> Fixes: c88b3f2558ed ("rawdev: introduce raw device library")
> Cc: sta...@dpdk.org
>
> Signed-off-by: Tomasz Duszynski
Acked-by: Hemant Agrawal
Applied, thanks.
--
David Marchand
Re: [PATCH v4] app/test: add support for skipping tests
On Mon, Sep 25, 2023 at 6:27 PM Aaron Conole wrote: > Bruce Richardson writes: > > > When called from automated tools, like meson test, it is often useful to > > skip tests in a test suite, without having to alter the test build. To > > do so, we add support for DPDK_TEST_SKIP environment variable, where one > > can provide a comma-separated list of tests. When the test binary is > > called to run one of the tests on the list via either cmdline parameter > > or environment variable (as done with meson test), the test will not > > actually be run, but will be reported skipped. > > > > Example run: > > $ DPDK_TEST_SKIP=dump_devargs,dump_ring meson test --suite=debug-tests > > ... > > 1/9 DPDK:debug-tests / dump_devargs SKIP1.11s > > 2/9 DPDK:debug-tests / dump_log_types OK 1.06s > > 3/9 DPDK:debug-tests / dump_malloc_heapsOK 1.11s > > 4/9 DPDK:debug-tests / dump_malloc_statsOK 1.07s > > 5/9 DPDK:debug-tests / dump_mempool OK 1.11s > > 6/9 DPDK:debug-tests / dump_memzone OK 1.06s > > 7/9 DPDK:debug-tests / dump_physmem OK 1.13s > > 8/9 DPDK:debug-tests / dump_ringSKIP1.04s > > 9/9 DPDK:debug-tests / dump_struct_sizesOK 1.10s > > > > Ok: 7 > > Expected Fail: 0 > > Fail: 0 > > Unexpected Pass:0 > > Skipped: 2 > > Timeout:0 > > > > Signed-off-by: Bruce Richardson > > Acked-by: Thomas Monjalon > Acked-by: Aaron Conole Applied, thanks. -- David Marchand
Re: [PATCH 0/2] add checks for tests not in a suite
On Wed, Sep 27, 2023 at 10:27 AM Bruce Richardson wrote: > > On Wed, Sep 27, 2023 at 08:30:05AM +0200, David Marchand wrote: > > On Tue, Sep 26, 2023 at 5:01 PM Aaron Conole wrote: > > > David Marchand writes: > > > > On Tue, Sep 19, 2023 at 10:36 AM Bruce Richardson > > > > wrote: > > > >> > > To help ensure that we don't have "orphaned" tests not in any test > > > >> > > suites we can add the following checks: > > > >> > > > > > >> > > * In developer-mode builds, emit a warning for each test defined > > > >> > > using > > > >> > > REGISTER_TEST_COMMAND > > > >> > > * In checkpatches, add a check to prevent the addition of new tests > > > >> > > using the REGISTER_TEST_COMMAND macro > > > >> > > > > > >> > > Bruce Richardson (2): > > > >> > > app/test: emit warning for tests not in a test suite > > > >> > > devtools: check for tests added without a test suite > > > >> > > > > > >> > > app/test/suites/meson.build | 13 - > > > >> > > buildtools/get-test-suites.py | 12 +--- > > > >> > > devtools/checkpatches.sh | 8 > > > >> > > 3 files changed, 29 insertions(+), 4 deletions(-) > > > >> > > > > >> > The "non_suite_tests" testsuite returned by > > > >> > buildtools/get-test-suites.py is a bit strange, as it is not a > > > >> > testsuite from meson pov. > > > >> > > > >> Yeah, it is a bit strange, and I'm open to new ideas on other > > > >> solutions. I > > > >> did it that way to avoid having yet another script to scan the files - > > > >> I > > > >> figured it was faster (in terms of runtime, not dev time) to do the > > > > > > > > I had figured it was "faster dev time" that won :-). > > > > I am fine with it, I don't expect more complications in this area in > > > > the future. > > > > > > > > > > > >> scanning when the files are already being opened and processed by this > > > >> one. > > > >> > > > >> Of course, if we can get the un-suitened [:-)] test cases down to > > > >> zero, we > > > >> can theoretically drop this check in future, and just use the > > > >> checkpatch > > > >> one. > > > > > > > > Well, that's still a question that nobody seems to comment on. > > > > > > > > What should we do with tests that don't enter one of those testsuites, > > > > and are not invoked by the CI? > > > > > > > > Though we may be removing some level of coverage, I am for > > > > cleaning/unused dead code. > > > > > > I guess it does require actually looking at these tests and classifying > > > them to either put them into the proper suites. As of now, we aren't > > > really removing coverage if they aren't being run - but are any > > > maintainers or developers actually running them? > > > > Could we go a step further than Bruce runtime warning (which is at the > > meson level and does not impact running the test)? > > Perhaps have those orphaned tests fail unless their test names are > > provided in a env variable like > > DPDK_TRUST_ME_I_WILL_SUBMIT_A_PATCH_FOR_THIS_TEST (naming is hard > > ;-))? > > > > With a systematic failure, there is less chance that > > developers/maintainers miss the situation. > > If those developers/maintainers simply waive the warning with the env > > variable and don't send a patch, well.. too bad. > > > > After a release or two, if we don't hear from anyone, we can start > > removing the unused one. > > > I think that seems a littel severe at this point. I'll put this idea in the fridge for now. Let's proceed with your series first (and some doc followups). -- David Marchand
Re: [PATCH 0/2] add checks for tests not in a suite
On Fri, Sep 15, 2023 at 1:52 PM Bruce Richardson wrote: > > To help ensure that we don't have "orphaned" tests not in any test > suites we can add the following checks: > > * In developer-mode builds, emit a warning for each test defined using > REGISTER_TEST_COMMAND > * In checkpatches, add a check to prevent the addition of new tests > using the REGISTER_TEST_COMMAND macro > > Bruce Richardson (2): > app/test: emit warning for tests not in a test suite > devtools: check for tests added without a test suite > > app/test/suites/meson.build | 13 - > buildtools/get-test-suites.py | 12 +--- > devtools/checkpatches.sh | 8 > 3 files changed, 29 insertions(+), 4 deletions(-) Series applied. Thanks Bruce. -- David Marchand
Re: [PATCH v4] bus/pci: fix legacy device IO port map in secondary process
On Wed, Aug 30, 2023 at 7:07 AM Wenwu Ma wrote:
>
> When doing IO port mapping for legacy device in secondary process, the
> region information is missing, so, we need to refill it.
>
> Fixes: 4b741542ecde ("bus/pci: avoid depending on private kernel value")
> Cc: sta...@dpdk.org
>
> Signed-off-by: Wenwu Ma
>
> ---
> drivers/bus/pci/linux/pci_vfio.c | 43 ++--
> 1 file changed, 41 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/bus/pci/linux/pci_vfio.c
> b/drivers/bus/pci/linux/pci_vfio.c
> index e634de8322..5ef26c98d1 100644
> --- a/drivers/bus/pci/linux/pci_vfio.c
> +++ b/drivers/bus/pci/linux/pci_vfio.c
> @@ -1314,6 +1314,27 @@ pci_vfio_ioport_map(struct rte_pci_device *dev, int
> bar,
> return -1;
> }
>
> + if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
> + struct vfio_device_info device_info = { .argsz =
> sizeof(device_info) };
> + char pci_addr[PATH_MAX];
> + int vfio_dev_fd;
> + struct rte_pci_addr *loc = &dev->addr;
> + int ret;
> + /* store PCI address string */
> + snprintf(pci_addr, sizeof(pci_addr), PCI_PRI_FMT,
> + loc->domain, loc->bus, loc->devid,
> loc->function);
> +
> + ret = rte_vfio_setup_device(rte_pci_get_sysfs_path(),
> pci_addr,
> + &vfio_dev_fd, &device_info);
>From a pci bus API pov, nothing prevents a driver from mixing memory
mapped with vfio and ioport resources (iow, calls to
rte_pci_map_resource() and rte_pci_ioport_map()).
IOW, it may not be the case with the net/virtio driver but, in theory,
rte_pci_ioport_map()/pci_vfio_ioport_map() may be called after a
rte_pci_map_resource() call.
In a similar manner, from the API pov,
rte_pci_ioport_map()/pci_vfio_ioport_map() may be called for multiple
bars.
In summary, nothing in this patch checks that vfio has been configured
already and I think we need a refcount to handle those situations.
Nipun, Chenbo, WDYT?
> + if (ret)
> + return -1;
ret value is not used, so there is no need for this variable.
if (rte_vfio_setup_device(rte_pci_get_sysfs_path(), pci_addr,
&vfio_dev_fd, &device_info) != 0)
return -1;
> +
> + ret = pci_vfio_fill_regions(dev, vfio_dev_fd, &device_info);
> + if (ret)
> + return -1;
Same here, ret is not needed.
> +
> + }
> +
> if (pci_vfio_get_region(dev, bar, &size, &offset) != 0) {
> RTE_LOG(ERR, EAL, "Cannot get offset of region %d.\n", bar);
> return -1;
> @@ -1361,8 +1382,26 @@ pci_vfio_ioport_write(struct rte_pci_ioport *p,
> int
> pci_vfio_ioport_unmap(struct rte_pci_ioport *p)
> {
> - RTE_SET_USED(p);
> - return -1;
> + char pci_addr[PATH_MAX] = {0};
> + struct rte_pci_addr *loc = &p->dev->addr;
> + int ret, vfio_dev_fd;
> +
> + /* store PCI address string */
> + snprintf(pci_addr, sizeof(pci_addr), PCI_PRI_FMT,
> + loc->domain, loc->bus, loc->devid, loc->function);
> +
> + vfio_dev_fd = rte_intr_dev_fd_get(p->dev->intr_handle);
> + if (vfio_dev_fd < 0)
> + return -1;
This check is odd and does not seem related.
> +
> + ret = rte_vfio_release_device(rte_pci_get_sysfs_path(), pci_addr,
> + vfio_dev_fd);
> + if (ret < 0) {
> + RTE_LOG(ERR, EAL, "Cannot release VFIO device\n");
> + return ret;
> + }
--
David Marchand
Re: [dpdk-dev] [PATCH] common/cnxk: fix direct rte symbol usage
Hello Jerin,
On Tue, Oct 3, 2023 at 8:40 PM wrote:
>
> From: Jerin Jacob
>
> The common code is shared between different driver environments,
> introduce missing plt_ abstractions of missing rte_ symbols and
> use plt symbols to avoid changing roc_* files.
>
> Also update the thread name for outbound soft expiry thread
> in a7ba40b2b1bf7.
>
> Fixes: 3d4e27fd7ff0 ("use abstracted bit count functions")
> Fixes: a7ba40b2b1bf ("drivers: convert to internal control threads")
> Fixes: c88d3638c7fc ("common/cnxk: support REE")
> Cc: sta...@dpdk.org
>
> Signed-off-by: Jerin Jacob
- Could we add something in checkpatch for this driver?
I was aware of this s/rte_/plt_/ peculiarity but still missed it...
- If you want this backported in LTS, I suggest splitting the fix
against c88d3638c7fc ("common/cnxk: support REE") and the rest than
only affects current release.
- One comment:
> diff --git a/drivers/common/cnxk/roc_nix_inl_dev.c
> b/drivers/common/cnxk/roc_nix_inl_dev.c
> index 6aa191410b..614d0858e5 100644
> --- a/drivers/common/cnxk/roc_nix_inl_dev.c
> +++ b/drivers/common/cnxk/roc_nix_inl_dev.c
> @@ -826,7 +826,7 @@ nix_inl_outb_poll_thread_setup(struct nix_inl_dev
> *inl_dev)
> soft_exp_consumer_cnt = 0;
> soft_exp_poll_thread_exit = false;
> rc = plt_thread_create_control(&inl_dev->soft_exp_poll_thread,
> - "outb-poll", nix_inl_outb_poll_thread, inl_dev);
> + "outb-soft-exp-poll", nix_inl_outb_poll_thread,
> inl_dev);
Such a thread name is too long.
This is reverting Thomas change.
Is this intentional?
--
David Marchand
Re: [dpdk-dev] [PATCH] common/cnxk: fix direct rte symbol usage
On Wed, Oct 4, 2023 at 8:43 AM Jerin Jacob wrote:
> > > diff --git a/drivers/common/cnxk/roc_nix_inl_dev.c
> > > b/drivers/common/cnxk/roc_nix_inl_dev.c
> > > index 6aa191410b..614d0858e5 100644
> > > --- a/drivers/common/cnxk/roc_nix_inl_dev.c
> > > +++ b/drivers/common/cnxk/roc_nix_inl_dev.c
> > > @@ -826,7 +826,7 @@ nix_inl_outb_poll_thread_setup(struct nix_inl_dev
> > > *inl_dev)
> > > soft_exp_consumer_cnt = 0;
> > > soft_exp_poll_thread_exit = false;
> > > rc = plt_thread_create_control(&inl_dev->soft_exp_poll_thread,
> > > - "outb-poll", nix_inl_outb_poll_thread, inl_dev);
> > > + "outb-soft-exp-poll", nix_inl_outb_poll_thread,
> > > inl_dev);
> >
> > Such a thread name is too long.
> > This is reverting Thomas change.
> >
> > Is this intentional?
>
> Yes, as mentioned in git commit log. Are 19 characters OK, right? If
The commitlog was ambiguous.
> not, I will reduce it, "outb-poll" too generic.
The thread name max length in pthread API is 16 bytes (including the
trailing \0).
Besides, looking again at this driver, I suspect Thomas missed it
because of the plt_ prefix, when doing ce703c47de95 ("eal: force
prefix for internal threads").
Converting to the internal API would restrict the name down to 11
bytes (including \0).
--
David Marchand
Re: [PATCH v2 1/2] random: initialize the random state for non-EAL threads
On Mon, Oct 2, 2023 at 2:28 PM Mattias Rönnblom wrote:
>
> On 2023-09-07 17:24, Stephen Hemminger wrote:
> > The per-lcore PRNG was not initializing the rand_state of all
> > the lcores. Any usage of rte_random by a non-EAL lcore would
>
> "/../ by an unregistered non-EAL thread /../"
>
> > use rand_states[RTE_MAX_LCORE] which was never initialized.
> >
> > Fix by using RTE_DIM() which will get all lcores.
> >
> > Fixes: 3f002f069612 ("eal: replace libc-based random generation with LFSR")
> > Cc: mattias.ronnb...@ericsson.com
> > Acked-by: Morten Brørup
> > Signed-off-by: Stephen Hemminger
> > ---
> > lib/eal/common/rte_random.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/lib/eal/common/rte_random.c b/lib/eal/common/rte_random.c
> > index 53636331a27b..812e5b4757b5 100644
> > --- a/lib/eal/common/rte_random.c
> > +++ b/lib/eal/common/rte_random.c
> > @@ -84,7 +84,7 @@ rte_srand(uint64_t seed)
> > unsigned int lcore_id;
> >
> > /* add lcore_id to seed to avoid having the same sequence */
> > - for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++)
> > + for (lcore_id = 0; lcore_id < RTE_DIM(rand_states); lcore_id++)
> > __rte_srand_lfsr258(seed + lcore_id, &rand_states[lcore_id]);
> > }
> >
>
> With the above-mentioned commit message rewording:
>
> Acked-by: Mattias Rönnblom
>
Applied this patch.
The second patch is marked as rejected in pw, in favor of a followup doc patch.
Thanks.
--
David Marchand
Re: [PATCH v2] net/axgbe: use CPUID to identify cpu
Hello Selwin,
On Tue, Oct 3, 2023 at 1:40 PM Selwin Sebastian
wrote:
>
> Using root complex to identify cpu will not work for vm passthrough.
> CPUID is used to get family and modelid to identify cpu
>
> Fixes: b0db927b5eba ("net/axgbe: use PCI root complex device to distinguish
> device")
> Cc: sta...@dpdk.org
>
> Signed-off-by: Selwin Sebastian
> ---
> drivers/net/axgbe/axgbe_ethdev.c | 106 ++-
> 1 file changed, 63 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/net/axgbe/axgbe_ethdev.c
> b/drivers/net/axgbe/axgbe_ethdev.c
> index 48714eebe6..4fdb0ae168 100644
> --- a/drivers/net/axgbe/axgbe_ethdev.c
> +++ b/drivers/net/axgbe/axgbe_ethdev.c
> @@ -12,6 +12,12 @@
>
> #include "eal_filesystem.h"
>
> +#ifdef RTE_ARCH_X86
> +#include
> +#else
> +#define __cpuid (n, a, b, c, d)
With a space in this macro definition, the precompiler will think that
it must replace the __cpuid token as literally (n, a, b, c, d).
--
David Marchand
Re: [PATCH] bus/dpaa: fix outside array bounds error with GCC v13
On Fri, Jul 21, 2023 at 7:28 AM Gagandeep Singh wrote:
>
> when RTE_ENABLE_ASSERT is enable, DPAA driver is doing
> wrong NULL check on frame queue which allows the code
> to have access to NULL address.
> GCC v13 is giving array bounds error if code is
> accessing any memory region less than 4KB.
> This patch fixes this issue by adding proper NULL checks
> on frame queue.
>
> Bugzilla ID: 1233
> Fixes: c47ff048b99a ("bus/dpaa: add QMAN driver core routines")
> Cc: sta...@dpdk.org
>
> Signed-off-by: Gagandeep Singh
> Acked-by: Hemant Agrawal
> Acked-by: Jerin Jacob
Applied, thanks.
--
David Marchand
Re: [PATCH v3] bus/cdx: provide driver flag for optional resource mapping
On Wed, Oct 4, 2023 at 12:06 PM Gangurde, Abhijit wrote: > > > +/** > > > + * Map the CDX device resources in user space virtual memory address. > > > + * > > > + * Note that driver should not call this function when flag > > > + * RTE_CDX_DRV_NEED_MAPPING is set, as EAL will do that for > > > + * you when it's on. > > > > Why should we export this function in the application ABI, if it is > > only used by drivers? > > This can be called from an application as well if this flag is not set hence, > we need to export this function. What kind of applications / in which usecase, one would need to map the device resources? Except a driver? -- David Marchand
Re: [PATCH] mem: allow using ASan in multi-process mode
On Wed, Oct 4, 2023 at 4:23 PM Artur Paszkiewicz wrote: > > Multi-process applications operate on shared hugepage memory but each > process has its own ASan shadow region which is not synchronized with > the other processes. This causes issues when different processes try to > use the same memory because they have their own view of which addresses > are valid. > > Fix it by mapping the shadow regions for memseg lists as shared memory. > The primary process is responsible for creating and removing the shared > memory objects. > > Disable ASan instrumentation for triggering the page fault in > alloc_seg() because if the segment is already allocated by another > process and is marked as free in the shadow, accessing this address will > cause an ASan error. > > Signed-off-by: Artur Paszkiewicz Interesting patch. I have a few questions: - did you test with --in-memory mode? with --no-huge? - I did not look at the patch, but I wonder if there is a risk some "local" ASan region (for the process heap, for example) can overlap with some "shared" ASan region (for shared DPDK hugepages). - with this work, would unit tests (that were marked failing with ASan) be ok now? See REGISTER_FAST_TEST macro in app/test. Thanks for working on this topic. -- David Marchand
Re: [PATCH v5 1/3] lib: introduce dispatcher library
ev2] are dequeued
> + * on a particular port, all pertaining to the same flow. The match
> + * callback for registration A returns true for ev0 and ev2, and the
> + * matching function for registration B for ev1. In that scenario, the
> + * dispatcher may choose to deliver first [ev0, ev2] using A's deliver
> + * function, and then [ev1] to B - or vice versa.
> + *
> + * rte_dispatcher_register() may be called by any thread
> + * (including unregistered non-EAL threads), but not while the event
> + * dispatcher is running on any service lcore.
> + *
> + * @param dispatcher
> + * The dispatcher instance.
> + *
> + * @param match_fun
> + * The match callback function.
> + *
> + * @param match_cb_data
> + * A pointer to some application-specific opaque data (or NULL),
> + * which is supplied back to the application when match_fun is
> + * called.
> + *
> + * @param process_fun
> + * The process callback function.
> + *
> + * @param process_cb_data
> + * A pointer to some application-specific opaque data (or NULL),
> + * which is supplied back to the application when process_fun is
> + * called.
> + *
> + * @return
> + * - >= 0: The identifier for this registration.
> + * - -ENOMEM: Unable to allocate sufficient resources.
> + */
> +__rte_experimental
> +int
> +rte_dispatcher_register(struct rte_dispatcher *dispatcher,
> + rte_dispatcher_match_t match_fun, void *match_cb_data,
> + rte_dispatcher_process_t process_fun,
> + void *process_cb_data);
> +
> +/**
> + * Unregister an event handler.
> + *
> + * This function may be called by any thread (including unregistered
> + * non-EAL threads), but not while the dispatcher is running on
> + * any service lcore.
> + *
> + * @param dispatcher
> + * The dispatcher instance.
> + *
> + * @param handler_id
> + * The handler registration id returned by the original
> + * rte_dispatcher_register() call.
> + *
> + * @return
> + * - 0: Success
> + * - -EINVAL: The @c handler_id parameter was invalid.
> + */
> +__rte_experimental
> +int
> +rte_dispatcher_unregister(struct rte_dispatcher *dispatcher, int handler_id);
> +
> +/**
> + * Register a finalize callback function.
> + *
> + * An application may optionally install one or more finalize
> + * callbacks.
> + *
> + * All finalize callbacks are invoked by the dispatcher when a
> + * complete batch of events (retrieve using rte_event_dequeue_burst())
> + * have been delivered to the application (or have been dropped).
> + *
> + * The finalize callback is not tied to any particular handler.
> + *
> + * The finalize callback provides an opportunity for the application
> + * to do per-batch processing. One case where this may be useful is if
> + * an event output buffer is used, and is shared among several
> + * handlers. In such a case, proper output buffer flushing may be
> + * assured using a finalize callback.
> + *
> + * rte_dispatcher_finalize_register() may be called by any thread
> + * (including unregistered non-EAL threads), but not while the
> + * dispatcher is running on any service lcore.
> + *
> + * @param dispatcher
> + * The dispatcher instance.
> + *
> + * @param finalize_fun
> + * The function called after completing the processing of a
> + * dequeue batch.
> + *
> + * @param finalize_data
> + * A pointer to some application-specific opaque data (or NULL),
> + * which is supplied back to the application when @c finalize_fun is
> + * called.
> + *
> + * @return
> + * - >= 0: The identifier for this registration.
> + * - -ENOMEM: Unable to allocate sufficient resources.
> + */
> +__rte_experimental
> +int
> +rte_dispatcher_finalize_register(struct rte_dispatcher *dispatcher,
> +rte_dispatcher_finalize_t finalize_fun,
> + void *finalize_data);
> +
> +/**
> + * Unregister a finalize callback.
> + *
> + * This function may be called by any thread (including unregistered
> + * non-EAL threads), but not while the dispatcher is running on
> + * any service lcore.
> + *
> + * @param dispatcher
> + * The dispatcher instance.
> + *
> + * @param reg_id
> + * The finalize registration id returned by the original
> + * rte_dispatcher_finalize_register() call.
> + *
> + * @return
> + * - 0: Success
> + * - -EINVAL: The @c reg_id parameter was invalid.
> + */
> +__rte_experimental
> +int
> +rte_dispatcher_finalize_unregister(struct rte_dispatcher *dispatcher,
> + int reg_id);
> +
> +/**
> + * Start a dispatcher instance.
> + *
> + * Enables the dispatcher service.
> + *
> + * The underlying event device must have been started prior to calling
> + * rte_dispatcher_start().
> + *
> + * For the dispatcher to actually perform work (i.e., dispatch
> + * events), its service must have been mapped to one or more service
> + * lcores, and its service run state set to '1'. A dispatcher's
> + * service is retrieved using rte_dispatcher_service_id_get().
> + *
> + * Each service lcore to which the dispatcher is mapped should
> + * have at least one event port configured. Such configuration is
> + * performed by calling rte_dispatcher_bind_port_to_lcore(), prior to
> + * starting the dispatcher.
> + *
> + * @param dispatcher
> + * The dispatcher instance.
> + *
> + * @return
> + * - 0: Success
> + * - <0: Error code on failure
> + */
> +__rte_experimental
> +int
> +rte_dispatcher_start(struct rte_dispatcher *dispatcher);
> +
> +/**
> + * Stop a running dispatcher instance.
> + *
> + * Disables the dispatcher service.
> + *
> + * @param dispatcher
> + * The dispatcher instance.
> + *
> + * @return
> + * - 0: Success
> + * - -EINVAL: Invalid @c id.
> + */
> +__rte_experimental
> +int
> +rte_dispatcher_stop(struct rte_dispatcher *dispatcher);
> +
> +/**
> + * Retrieve statistics for a dispatcher instance.
> + *
> + * This function is MT safe and may be called by any thread
> + * (including unregistered non-EAL threads).
> + *
> + * @param dispatcher
> + * The dispatcher instance.
> + * @param[out] stats
> + * A pointer to a structure to fill with statistics.
> + */
> +__rte_experimental
> +void
> +rte_dispatcher_stats_get(const struct rte_dispatcher *dispatcher,
> +struct rte_dispatcher_stats *stats);
> +
> +/**
> + * Reset statistics for a dispatcher instance.
> + *
> + * This function may be called by any thread (including unregistered
> + * non-EAL threads), but may not produce the correct result if the
> + * dispatcher is running on any service lcore.
> + *
> + * @param dispatcher
> + * The dispatcher instance.
> + */
> +__rte_experimental
> +void
> +rte_dispatcher_stats_reset(struct rte_dispatcher *dispatcher);
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif /* __RTE_DISPATCHER__ */
> diff --git a/lib/dispatcher/version.map b/lib/dispatcher/version.map
> new file mode 100644
> index 00..8f9ad96522
> --- /dev/null
> +++ b/lib/dispatcher/version.map
> @@ -0,0 +1,20 @@
> +EXPERIMENTAL {
> + global:
> +
> + # added in 23.11
> + rte_dispatcher_create;
> + rte_dispatcher_free;
> + rte_dispatcher_service_id_get;
> + rte_dispatcher_bind_port_to_lcore;
> + rte_dispatcher_unbind_port_from_lcore;
> + rte_dispatcher_register;
> + rte_dispatcher_unregister;
> + rte_dispatcher_finalize_register;
> + rte_dispatcher_finalize_unregister;
> + rte_dispatcher_start;
> + rte_dispatcher_stop;
> + rte_dispatcher_stats_get;
> + rte_dispatcher_stats_reset;
Sort alphabetically please.
> +
> + local: *;
> +};
> diff --git a/lib/meson.build b/lib/meson.build
> index 099b0ed18a..3093b338d2 100644
> --- a/lib/meson.build
> +++ b/lib/meson.build
> @@ -35,6 +35,7 @@ libraries = [
> 'distributor',
> 'efd',
> 'eventdev',
> +'dispatcher', # dispatcher depends on eventdev
> 'gpudev',
> 'gro',
> 'gso',
> @@ -81,6 +82,7 @@ optional_libs = [
> 'cfgfile',
> 'compressdev',
> 'cryptodev',
> +'dispatcher',
> 'distributor',
> 'dmadev',
> 'efd',
> --
> 2.34.1
>
--
David Marchand
Re: [PATCH v5 2/3] test: add dispatcher test suite
} while (stats.ev_drop_count == 0 && stats.ev_dispatch_count == 0);
> +
> + rc = test_app_stop(test_app);
> + RETURN_ON_ERROR(rc);
> +
> + TEST_ASSERT_EQUAL(stats.ev_drop_count, 1, "Drop count is not one");
> + TEST_ASSERT_EQUAL(stats.ev_dispatch_count, 0,
> + "Dispatch count is not zero");
> + TEST_ASSERT(stats.poll_count > 0, "Poll count is zero");
> +
> + return TEST_SUCCESS;
> +}
> +
> +#define MORE_THAN_MAX_HANDLERS 1000
> +#define MIN_HANDLERS 32
> +
> +static int
> +test_many_handler_registrations(void)
> +{
> + int rc;
> + int num_regs = 0;
> + int reg_ids[MORE_THAN_MAX_HANDLERS];
> + int reg_id;
> + int i;
> +
> + rc = test_app_unregister_callbacks(test_app);
> +
> + RETURN_ON_ERROR(rc);
> +
> + for (i = 0; i < MORE_THAN_MAX_HANDLERS; i++) {
> + reg_id = rte_dispatcher_register(test_app->dispatcher,
> +never_match, NULL,
> +test_app_never_process,
> NULL);
> + if (reg_id < 0)
> + break;
> +
> + reg_ids[num_regs++] = reg_id;
> + }
> +
> + TEST_ASSERT_EQUAL(reg_id, -ENOMEM, "Incorrect return code. Expected "
> + "%d but was %d", -ENOMEM, reg_id);
> + TEST_ASSERT(num_regs >= MIN_HANDLERS, "Registration failed already "
> + "after %d handler registrations.", num_regs);
> +
> + for (i = 0; i < num_regs; i++) {
> + rc = rte_dispatcher_unregister(test_app->dispatcher,
> + reg_ids[i]);
> + TEST_ASSERT_SUCCESS(rc, "Unable to unregister handler %d",
> + reg_ids[i]);
> + }
> +
> + return TEST_SUCCESS;
> +}
> +
> +static void
> +dummy_finalize(uint8_t event_dev_id __rte_unused,
> + uint8_t event_port_id __rte_unused,
> + void *cb_data __rte_unused)
> +{
> +}
> +
> +#define MORE_THAN_MAX_FINALIZERS 1000
> +#define MIN_FINALIZERS 16
> +
> +static int
> +test_many_finalize_registrations(void)
> +{
> + int rc;
> + int num_regs = 0;
> + int reg_ids[MORE_THAN_MAX_FINALIZERS];
> + int reg_id;
> + int i;
> +
> + rc = test_app_unregister_callbacks(test_app);
> +
> + RETURN_ON_ERROR(rc);
> +
> + for (i = 0; i < MORE_THAN_MAX_FINALIZERS; i++) {
> + reg_id = rte_dispatcher_finalize_register(
> + test_app->dispatcher, dummy_finalize, NULL
> + );
> +
> + if (reg_id < 0)
> + break;
> +
> + reg_ids[num_regs++] = reg_id;
> + }
> +
> + TEST_ASSERT_EQUAL(reg_id, -ENOMEM, "Incorrect return code. Expected "
> + "%d but was %d", -ENOMEM, reg_id);
> + TEST_ASSERT(num_regs >= MIN_FINALIZERS, "Finalize registration failed
> "
> + "already after %d registrations.", num_regs);
> +
> + for (i = 0; i < num_regs; i++) {
> + rc = rte_dispatcher_finalize_unregister(
> + test_app->dispatcher, reg_ids[i]
> + );
> + TEST_ASSERT_SUCCESS(rc, "Unable to unregister finalizer %d",
> + reg_ids[i]);
> + }
> +
> + return TEST_SUCCESS;
> +}
> +
> +static struct unit_test_suite test_suite = {
> + .suite_name = "Event dispatcher test suite",
> + .unit_test_cases = {
> + TEST_CASE_ST(test_setup, test_teardown, test_basic),
> + TEST_CASE_ST(test_setup, test_teardown, test_drop),
> + TEST_CASE_ST(test_setup, test_teardown,
> +test_many_handler_registrations),
> + TEST_CASE_ST(test_setup, test_teardown,
> +test_many_finalize_registrations),
> + TEST_CASES_END()
> + }
> +};
> +
> +static int
> +test_dispatcher(void)
> +{
> + return unit_test_suite_runner(&test_suite);
> +}
> +
> +REGISTER_TEST_COMMAND(dispatcher_autotest, test_dispatcher);
We have new macros (see REGISTER_FAST_TEST for example) so a test is
associated to an existing testsuite.
I think this test should be part of the fast-test testsuite, wdyt?
--
David Marchand
Re: [PATCH v5 3/3] doc: add dispatcher programming guide
e_burst()`` call is made (assuming implicit release
> +is employed).
> +
> +The following is an example with an application-defined event output
> +buffer (the ``event_buffer``):
> +
> +.. code-block:: c
> +
> +static void
> +finalize_batch(uint8_t event_dev_id, uint8_t event_port_id,
> + void *cb_data)
> +{
> +struct event_buffer *buffer = cb_data;
> +unsigned lcore_id = rte_lcore_id();
> +struct event_buffer_lcore *lcore_buffer =
> +&buffer->lcore_buffer[lcore_id];
> +
> +event_buffer_lcore_flush(lcore_buffer);
> +}
> +
> +/* In the module's initialization code */
> +rte_dispatcher_finalize_register(dispatcher, finalize_batch,
> + shared_event_buffer);
> +
> +The dispatcher does not track any relationship between a handler and a
> +finalize callback, and all finalize callbacks will be called, if (and
> +only if) at least one event was dequeued from the event device.
> +
> +Finalize callback registration and unregistration cannot safely be
> +done while the dispatcher's service function is running on any lcore.
> +
> +Service
> +---
> +
> +The dispatcher is a DPDK service, and is managed in a manner similar
> +to other DPDK services (e.g., an Event Timer Adapter).
> +
> +Below is an example of how to configure a particular lcore to serve as
> +a service lcore, and to map an already-configured dispatcher
> +(identified by ``DISPATCHER_ID``) to that lcore.
> +
> +.. code-block:: c
> +
> +static void
> +launch_dispatcher_core(struct rte_dispatcher *dispatcher,
> + unsigned lcore_id)
> +{
> +uint32_t service_id;
> +
> +rte_service_lcore_add(lcore_id);
> +
> +rte_dispatcher_service_id_get(dispatcher, &service_id);
> +
> +rte_service_map_lcore_set(service_id, lcore_id, 1);
> +
> +rte_service_lcore_start(lcore_id);
> +
> +rte_service_runstate_set(service_id, 1);
> +}
> +
> +As the final step, the dispatcher must be started.
> +
> +.. code-block:: c
> +
> +rte_dispatcher_start(dispatcher);
> +
> +
> +Multi Service Dispatcher Lcores
> +^^^
> +
> +In an Eventdev application, most (or all) compute-intensive and
> +performance-sensitive processing is done in an event-driven manner,
> +where CPU cycles spent on application domain logic is the direct
> +result of items of work (i.e., ``rte_event`` events) dequeued from an
> +event device.
> +
> +In the light of this, it makes sense to have the dispatcher service be
> +the only DPDK service on all lcores used for packet processing — at
> +least in principle.
> +
> +However, there is nothing in DPDK that prevents colocating other
> +services with the dispatcher service on the same lcore.
> +
> +Tasks that prior to the introduction of the dispatcher into the
> +application was performed on the lcore, even though no events were
> +received, are prime targets for being converted into such auxiliary
> +services, running on the dispatcher core set.
> +
> +An example of such a task would be the management of a per-lcore timer
> +wheel (i.e., calling ``rte_timer_manage()``).
> +
> +For applications employing :doc:`Read-Copy-Update (RCU) ` (or
> +similar technique), may opt for having quiescent state (e.g., calling
> +``rte_rcu_qsbr_quiescent()``) signaling factored out into a separate
> +service, to assure resource reclaimination occurs even in though some
> +lcores currently do not process any events.
> +
> +If more services than the dispatcher service is mapped to a service
> +lcore, it's important that the other service are well-behaved and
> +don't interfere with event processing to the extent the system's
> +throughput and/or latency requirements are at risk of not being met.
> +
> +In particular, to avoid jitter, they should have an small upper bound
> +for the maximum amount of time spent in a single service function
> +call.
> +
> +An example of scenario with a more CPU-heavy colocated service is a
> +low-lcore count deployment, where the event device lacks the
> +``RTE_EVENT_ETH_RX_ADAPTER_CAP_INTERNAL_PORT`` capability (and thus
> +require software to feed incoming packets into the event device). In
> +this case, the best performance may be achieved if the Event Ethernet
> +RX and/or TX Adapters are mapped to lcores also used by for event
> +dispatching, since otherwise the adapter lcores would have a lot of
> +idle CPU cycles.
> +
> +.. rubric:: Footnotes
> +
> +.. [#Mapping]
> + Event routing may reasonably be done based on other ``rte_event``
> + fields (or even event user data). Indeed, that's the very reason to
> + have match callback functions, instead of a simple queue
> + id-to-handler mapping scheme. Queue id-based routing serves well in
> + a simple example.
> +
> +.. [#Port-MT-Safety]
> + This property (which is a feature, not a bug) is inherited from the
> + core Eventdev APIs.
> diff --git a/doc/guides/prog_guide/index.rst b/doc/guides/prog_guide/index.rst
> index 52a6d9e7aa..ab05bd6074 100644
> --- a/doc/guides/prog_guide/index.rst
> +++ b/doc/guides/prog_guide/index.rst
> @@ -60,6 +60,7 @@ Programmer's Guide
> event_ethernet_tx_adapter
> event_timer_adapter
> event_crypto_adapter
> +dispatcher_lib
> qos_framework
> power_man
> packet_classif_access_ctrl
> --
> 2.34.1
>
--
David Marchand
Re: [PATCH] config/x86: config support for AMD EPYC processors
On Mon, Sep 25, 2023 at 5:11 PM Sivaprasad Tummala wrote: > > From: Sivaprasad Tummala > > By default, max lcores are limited to 128 for x86 platforms. > On AMD EPYC processors, this limit needs to be increased to > leverage all the cores. > > The patch adjusts the limit specifically for native compilation > on AMD EPYC CPUs. > > Signed-off-by: Sivaprasad Tummala This patch is a revamp of http://inbox.dpdk.org/dev/by5pr12mb3681c3fc6676bc03f0b42ccc96...@by5pr12mb3681.namprd12.prod.outlook.com/ for which a discussion at techboard is supposed to have taken place. But I didn't find a trace of it. One option that had been discussed in the previous thread was to increase the max number of cores for x86. I am unclear if this option has been properly evaluated/debatted. Can the topic be brought again at techboard? Thanks. -- David Marchand
Re: [PATCH] gpu: add support for rtx 6000 variant
On Thu, Oct 5, 2023 at 2:50 PM Elena Agostini wrote: > > This patch makes sense to me and actually we should update the > drivers/gpu/cuda/devices.h with all the new GPU devices ID > > I don’t have an RTX 6000 GPU in my setup so I trust Cliff on the correctness > of the ID Elena, please work with Cliff on a v2 with a release notes update and ping me or Thomas once the patch is reviewed and ready for merge. Thanks. -- David Marchand
Re: [PATCH v9] hash: add XOR32 hash function
Hello, On Tue, Jul 11, 2023 at 12:00 AM Bili Dong wrote: > > An XOR32 hash is needed in the Software Switch (SWX) Pipeline for its > use case in P4. We implement it in this patch so it could be easily > registered in the pipeline later. > > Signed-off-by: Bili Dong > .mailmap | 1 + > app/test/test_hash_functions.c | 47 +++--- > lib/hash/rte_hash_xor.h| 113 + > 3 files changed, 151 insertions(+), 10 deletions(-) > create mode 100644 lib/hash/rte_hash_xor.h > I was about to merge this patch but there are some issues that should have been caught by the library maintainer... It seems the intention is to have a public API (meaning external DPDK apps) consumption. Then the header must be exported via 'headers' in lib/hash/meson.build. Besides, such API addition must be announced in the release notes. And this API should be documented either in some doc/guides/ or at least with doxygen (meaning that rte_hash_xor.h must be referenced in doc/api/doxy-api-index.md). Thanks. -- David Marchand
Re: [PATCH] gpu/cuda: Add missing stdlib include
Hello,
On Thu, Oct 5, 2023 at 2:44 PM Elena Agostini wrote:
>
> Ack
Please, don't top post.
>
>
>
> Thanks
>
> EA
>
>
>
> From: David Marchand
> Date: Friday, 29 September 2023 at 16:58
> To: Aaron Conole , John Romein
> Cc: dev@dpdk.org , Elena Agostini ,
> levendsa...@gmail.com
> Subject: Re: [PATCH] gpu/cuda: Add missing stdlib include
>
> External email: Use caution opening links or attachments
>
>
> On Tue, Sep 26, 2023 at 8:24 PM Aaron Conole wrote:
> >
> >
> > From: John Romein
> >
> > getenv needs stdlib.h to be included.
> >
> > Bugzilla ID: 1133
> >
> > Fixes: 24c77594e08f ("gpu/cuda: map GPU memory with GDRCopy")
> > Signed-off-by: John Romein
>
> Thanks for the patch, it seems to be a duplicate of the prior patch
> sent by Levend:
> https://patchwork.dpdk.org/project/dpdk/patch/20230803162512.41396-1-levendsa...@gmail.com/
As I mentionned, this is a duplicate and you just acked the other patch.
Please pay attention.
--
David Marchand
Re: [PATCH v5 3/3] power: amd power monitor support
On Wed, Aug 16, 2023 at 9:00 PM Sivaprasad Tummala wrote: > > mwaitx allows EPYC processors to enter a implementation dependent > power/performance optimized state (C1 state) for a specific period > or until a store to the monitored address range. > > Signed-off-by: Sivaprasad Tummala > Acked-by: Anatoly Burakov This series won't apply cleanly (following MSVC related series merge) and it breaks x86 32bits compilation. http://mails.dpdk.org/archives/test-report/2023-August/442809.html -- David Marchand
Re: [PATCH v2 2/2] eal: remove NUMFLAGS enumeration
On Fri, Aug 11, 2023 at 8:08 AM Sivaprasad Tummala wrote: > > This patch removes RTE_CPUFLAG_NUMFLAGS to allow new CPU > features without breaking ABI each time. > > Signed-off-by: Sivaprasad Tummala I relooked at the API and I see one case (that I had missed in my previous reply) where an ABI issue may be present so I updated the commitlog in this regard. And then I merged this patch, with a few modifications. - removed the deprecation notice and updated RN, - removed leftover "Last item" comments in arch cpuflag headers, I hope everyone is happy with this. Thanks. -- David Marchand
Re: [PATCH v2] power: support amd-pstate cpufreq driver
On Tue, Sep 26, 2023 at 10:01 PM Sivaprasad Tummala wrote: > > amd-pstate introduces a new CPU frequency control mechanism for AMD > EPYC processors using the ACPI Collaborative Performance Power Control > feature for a finer grained frequency management. > > Patch to add support for amd-pstate driver. > > Signed-off-by: Sivaprasad Tummala Review please. -- David Marchand
Re: [PATCH v5 1/3] lib: introduce dispatcher library
Hello Mattias,
On Thu, Oct 5, 2023 at 12:09 PM Mattias Rönnblom wrote:
> >> +
> >> +deps += ['eventdev']
> >> diff --git a/lib/dispatcher/rte_dispatcher.c
> >> b/lib/dispatcher/rte_dispatcher.c
> >> new file mode 100644
> >> index 00..0e69db2b9b
> >> --- /dev/null
> >> +++ b/lib/dispatcher/rte_dispatcher.c
> >> @@ -0,0 +1,708 @@
> >> +/* SPDX-License-Identifier: BSD-3-Clause
> >> + * Copyright(c) 2023 Ericsson AB
> >> + */
> >> +
> >> +#include
> >> +#include
> >> +
> >> +#include
> >> +#include
> >> +#include
> >> +#include
> >> +#include
> >> +
> >> +#include "eventdev_pmd.h"
> >> +
> >> +#include
> >> +
> >> +#define EVD_MAX_PORTS_PER_LCORE 4
> >> +#define EVD_MAX_HANDLERS 32
> >> +#define EVD_MAX_FINALIZERS 16
> >> +#define EVD_AVG_PRIO_INTERVAL 2000
> >> +#define EVD_SERVICE_NAME "dispatcher"
> >> +
> >> +struct rte_dispatcher_lcore_port {
> >> + uint8_t port_id;
> >> + uint16_t batch_size;
> >> + uint64_t timeout;
> >> +};
> >> +
> >> +struct rte_dispatcher_handler {
> >> + int id;
> >> + rte_dispatcher_match_t match_fun;
> >> + void *match_data;
> >> + rte_dispatcher_process_t process_fun;
> >> + void *process_data;
> >> +};
> >> +
> >> +struct rte_dispatcher_finalizer {
> >> + int id;
> >> + rte_dispatcher_finalize_t finalize_fun;
> >> + void *finalize_data;
> >> +};
> >> +
> >> +struct rte_dispatcher_lcore {
> >> + uint8_t num_ports;
> >> + uint16_t num_handlers;
> >> + int32_t prio_count;
> >> + struct rte_dispatcher_lcore_port ports[EVD_MAX_PORTS_PER_LCORE];
> >> + struct rte_dispatcher_handler handlers[EVD_MAX_HANDLERS];
> >> + struct rte_dispatcher_stats stats;
> >> +} __rte_cache_aligned;
> >> +
> >> +struct rte_dispatcher {
> >> + uint8_t event_dev_id;
> >> + int socket_id;
> >> + uint32_t service_id;
> >> + struct rte_dispatcher_lcore lcores[RTE_MAX_LCORE];
> >> + uint16_t num_finalizers;
> >> + struct rte_dispatcher_finalizer finalizers[EVD_MAX_FINALIZERS];
> >> +};
> >> +
> >> +static int
> >> +evd_lookup_handler_idx(struct rte_dispatcher_lcore *lcore,
> >> + const struct rte_event *event)
> >
> > Wrt DPDK coding tyle, indent is a single tab.
> > Adding an extra tab is recommended when continuing control statements
> > like if()/for()/..
> >
>
> Sure, but I don't understand why you mention this.
I wanted to remind the DPDK coding style which I try to more closely
enforce for new code.
indent is off in this file (especially for function prototypes with
multiple tabs used).
>
> > On the other hand, max accepted length for a line is 100 columns.
> >
> > Wdyt of a single line for this specific case?
>
>
> Are you asking why the evd_lookup_handler_idx() function prototype is
> not a single line?
>
> It would make it long, that's why. Even if 100 wide lines are allowed,
> it doesn't means the author is forced to use such long lines?
I find it more readable.
If you want to stick to 80 columns, please comply with a single tab for indent.
[snip]
> >> +static int
> >> +evd_set_service_runstate(struct rte_dispatcher *dispatcher, int state)
> >> +{
> >> + int rc;
> >> +
> >> + rc = rte_service_component_runstate_set(dispatcher->service_id,
> >> + state);
> >> +
> >> + if (rc != 0) {
> >> + RTE_EDEV_LOG_ERR("Unexpected error %d occurred while
> >> setting "
> >> +"service component run state to %d\n", rc,
> >> +state);
> >> + RTE_ASSERT(0);
> >
> > Why not propagating the error to callers?
> >
> >
>
> The root cause would be a programming error, hence an assertion is more
> appropriate way to deal with the situation.
Without building RTE_ENABLE_ASSERT (disabled by default), the code
later in this function will still be executed.
[snip]
> >> +typedef void
> >> +(*rte_dispatcher_finalize_t)(uint8_t event_dev_id, uint8_t event_port_id,
> >> + void *cb_data);
> >> +
> >> +/**
> >> + * Dispatcher statistics
> >> + */
> >> +struct rte_dispatcher_stats {
> >> + uint64_t poll_count;
> >> + /**< Number of event dequeue calls made toward the event device. */
> >
> > We had a number of issues with doxygen post annotations.
> > Prefer the prefixed ones.
> >
>
> OK. More readable, too. I just used the postfix syntax since it seemed
> the only one used in DPDK.
Historically yes, but we started cleaning headers for readability
(like in ethdev) and after catching a few errors with postfix
comments.
--
David Marchand
Re: [PATCH v5 2/3] test: add dispatcher test suite
On Thu, Oct 5, 2023 at 1:26 PM Mattias Rönnblom wrote:
[snip]
> >> +#define RETURN_ON_ERROR(rc) \
> >> + do {\
> >> + if (rc != TEST_SUCCESS) \
> >> + return rc; \
> >> + } while (0)
> >
> > TEST_ASSERT?
> > This gives context about which part of a test failed.
> >
>
> This macro is used in a situation where the failure has occured and has
> been reported already.
>
> Maybe it would be better to replace the macro instationation with just
> the if+return statements.
>
> RETURN_ON_ERROR(rc);
>
> ->
>
> if (rc != TEST_SUCCESS)
> return rc;
Yes, this macro does not add much, you can remove it.
[snip]
> >> + for (i = 0; i < NUM_SERVICE_CORES; i++)
> >> + if (app->service_lcores[i] == lcore_id)
> >> + return i;
> >
> > This construct is hard to read and prone to error if the code is updated
> > later.
> >
> > for () {
> >if ()
> > return i;
> > }
> >
> >
>
> I wouldn't consider that an improvement (rather the opposite).
Well, I disagree, but it is not enforced in the coding style so I won't insist.
[snip]
> >> +static struct unit_test_suite test_suite = {
> >> + .suite_name = "Event dispatcher test suite",
> >> + .unit_test_cases = {
> >> + TEST_CASE_ST(test_setup, test_teardown, test_basic),
> >> + TEST_CASE_ST(test_setup, test_teardown, test_drop),
> >> + TEST_CASE_ST(test_setup, test_teardown,
> >> +test_many_handler_registrations),
> >> + TEST_CASE_ST(test_setup, test_teardown,
> >> +test_many_finalize_registrations),
> >> + TEST_CASES_END()
> >> + }
> >> +};
> >> +
> >> +static int
> >> +test_dispatcher(void)
> >> +{
> >> + return unit_test_suite_runner(&test_suite);
> >> +}
> >> +
> >> +REGISTER_TEST_COMMAND(dispatcher_autotest, test_dispatcher);
> >
> > We have new macros (see REGISTER_FAST_TEST for example) so a test is
> > associated to an existing testsuite.
> > I think this test should be part of the fast-test testsuite, wdyt?
> >
> >
>
> It needs setup and teardown methods, so I assume a generic test suite
> woulnd't do.
>
> The dispatcher tests do have fairly short run times, so in that sense
> they should qualify.
So please use REGISTER_FAST_TEST().
Thanks.
--
David Marchand
Re: [PATCH v2 1/2] power: refactor uncore power management interfaces
On Wed, Aug 16, 2023 at 12:10 PM Sivaprasad Tummala
wrote:
>
> currently the uncore power management implementation is vendor specific.
> Added new vendor agnostic uncore power interface similar to rte_power
> and rename specific implementations ("rte_power_intel_uncore") to
> "power_intel_uncore" along with functions.
>
> Signed-off-by: Sivaprasad Tummala
Review please.
--
David Marchand
Re: [PATCH 4/4] pcapng: move timestamp calculation into pdump
On Wed, Oct 4, 2023 at 7:13 PM Stephen Hemminger
wrote:
>
> On Mon, 2 Oct 2023 10:15:25 +0200
> David Marchand wrote:
>
> > >
> >
> > Bugzilla ID: 1291 ?
> >
> > This patch (and patch 3) updates some pcapng API, is it worth a RN update?
> >
> > > Fixes: c882eb544842 ("pcapng: fix timestamp wrapping in output files")
> >
> > Is it worth backporting?
> > I would say no, as some API update was needed to fix the issue.
> > But on the other hand, this is an experimental API, so I prefer to ask.
> >
> >
> > > Signed-off-by: Stephen Hemminger
>
> Good question.
> Is experimental API allowed to change in a stable release?
I don't think this is cleary described in our ABI policy.
An experimental API may be changed at any time, but nothing is said
wrt backports.
Breaking an API is always a pain, and for a LTS release it would
probably be badly accepted by users.
Cc: Kevin for his opinion.
We may need a clarification on this topic in the doc.
--
David Marchand
Re: [PATCH] random: clarify PRNG MT safety guarantees
On Wed, Oct 4, 2023 at 1:00 PM Mattias Rönnblom
wrote:
>
> Clarify MT safety guarantees for unregistered non-EAL threads calling
> PRNG functions in rte_random.h.
>
> Clarify that rte_srand() is not MT safe in regards to calls to
> rte_rand_max() and rte_drand().
>
> Suggested-by: Stephen Hemminger
> Signed-off-by: Mattias Rönnblom
Should it be backported along 3a4e21301c7a ("random: initialize state
for unregistered non-EAL threads") ?
--
David Marchand
Re: [PATCH] build: deprecate enable_kmods option
On Thu, Aug 10, 2023 at 3:29 PM Bruce Richardson wrote: > > With the removal of the kni kernel driver, there are no longer any > Linux kernel modules in our repository, leaving only modules for FreeBSD > present. Since: > > * BSD has no issues with out-of-tree modules and > * There are no in-tree equivalents for those modules in BSD > > there is no point in building for BSD without those modules. > > Therefore, we can remove the enable_kmods option, always building the > kmods for BSD. > > We can also remove the infrastructure for Linux kmods too, since use of > out-of-tree modules for Linux is not something the DPDK project wants to > pursue in future. > > Signed-off-by: Bruce Richardson > Acked-by: Stephen Hemminger > Acked-by: Morten Brørup > Acked-by: Thomas Monjalon Acked-by: David Marchand Applied, thanks Bruce. -- David Marchand
Re: [PATCH] gpu/cuda: fix getenv related build error
On Thu, Aug 3, 2023 at 6:25 PM Levend Sayar wrote:
>
> If gdrapi.h is available, meson sets DRIVERS_GPU_CUDA_GDRCOPY_H as 1.
> This causes gdrcopy.c build to give an error;
> because compiler can not find signature of getenv.
> stdlib.h is included for the definition of getenv function.
>
> Fixes: ca12f5e8a7db ("gpu/cuda: mark unused GDRCopy functions parameters")
>
> Signed-off-by: Levend Sayar
Applied, thanks Levend.
--
David Marchand
Re: [PATCH v4 2/2] eal: annotate rte_memseg_list_walk()
On Fri, Sep 8, 2023 at 3:18 PM Artemy Kovalyov wrote: > > Implementing a lock annotation for rte_memseg_list_walk() to > proactively identify bugs similar to memory_hotplug_lock deadlock during > initialization during compile time. > > Signed-off-by: Artemy Kovalyov Series applied, thanks Artemy. -- David Marchand
Re: [dpdk-dev] [PATCH v6] crypto/ccp: move device from vdev to PCI
Hello Sunil,
On Mon, Jul 26, 2021 at 11:09 AM wrote:
>
> From: Amaranath Somalapuram
>
> drop all the code duplicating the PCI bus driver
> developed for enable IOMMU in vdev.
>
> Signed-off-by: Amaranath Somalapuram
Looking back in history, this patch converted crypto/ccp to a PCI
driver, but the associated doc is still about vdev.
https://doc.dpdk.org/guides/cryptodevs/ccp.html#initialization
And I also noticed some remnants, like:
drivers/crypto/ccp/meson.build:deps += 'bus_vdev'
drivers/crypto/ccp/rte_ccp_pmd.c:#include
drivers/crypto/ccp/rte_ccp_pmd.c: CCP_LOG_ERR("failed to
create cryptodev vdev");
Can you send fixes?
Thanks.
--
David Marchand
[PATCH] doc/guides: refer to generic binding devices section
Rather than copy/paste everywhere how to bind a device and create VF devices, refer to the Linux GSG section about it. Signed-off-by: David Marchand --- doc/guides/bbdevs/acc100.rst| 73 ++--- doc/guides/bbdevs/fpga_5gnr_fec.rst | 71 ++-- doc/guides/bbdevs/fpga_lte_fec.rst | 70 ++- doc/guides/bbdevs/vrb1.rst | 66 ++ doc/guides/cryptodevs/ccp.rst | 14 +- doc/guides/nics/intel_vf.rst| 57 -- 6 files changed, 25 insertions(+), 326 deletions(-) diff --git a/doc/guides/bbdevs/acc100.rst b/doc/guides/bbdevs/acc100.rst index 8a275dcdd4..f26ce58f78 100644 --- a/doc/guides/bbdevs/acc100.rst +++ b/doc/guides/bbdevs/acc100.rst @@ -94,7 +94,7 @@ Initialization -- When the device first powers up, its PCI Physical Functions (PF) can be listed through these -commands for ACC100 and ACC101 respectively: +commands for ACC100 (device id ``0d5c``) and ACC101 (device id ``57c4``) respectively: .. code-block:: console @@ -102,76 +102,11 @@ commands for ACC100 and ACC101 respectively: sudo lspci -vd8086:57c4 The physical and virtual functions are compatible with Linux UIO drivers: -``vfio`` and ``igb_uio``. However, in order to work the 5G/4G +``vfio_pci`` and ``igb_uio``. However, in order to work the 5G/4G FEC device first needs to be bound to one of these linux drivers through DPDK. - -Bind PF UIO driver(s) -~ - -Install the DPDK igb_uio driver, bind it with the PF PCI device ID and use -``lspci`` to confirm the PF device is under use by ``igb_uio`` DPDK UIO driver. - -The igb_uio driver may be bound to the PF PCI device using one of two methods for ACC100 -(for ACC101 the device id ``57c4`` should be used in lieu of ``0d5c``): - - -1. PCI functions (physical or virtual, depending on the use case) can be bound to -the UIO driver by repeating this command for every function. - -.. code-block:: console - - cd - insmod ./build/kmod/igb_uio.ko - echo "8086 0d5c" > /sys/bus/pci/drivers/igb_uio/new_id - lspci -vd8086:0d5c - - -2. Another way to bind PF with DPDK UIO driver is by using the ``dpdk-devbind.py`` tool - -.. code-block:: console - - cd - ./usertools/dpdk-devbind.py -b igb_uio :06:00.0 - -where the PCI device ID (example: :06:00.0) is obtained using lspci -vd8086:0d5c - - -In a similar way the 5G/4G FEC PF may be bound with vfio-pci as any PCIe device. - - -Enable Virtual Functions - - -Now, it should be visible in the printouts that PCI PF is under igb_uio control -"``Kernel driver in use: igb_uio``" - -To show the number of available VFs on the device, read ``sriov_totalvfs`` file.. - -.. code-block:: console - - cat /sys/bus/pci/devices/\:\:./sriov_totalvfs - - where \:\:. is the PCI device ID - - -To enable VFs via igb_uio, echo the number of virtual functions intended to -enable to ``max_vfs`` file.. - -.. code-block:: console - - echo > /sys/bus/pci/devices/\:\:./max_vfs - - -Afterwards, all VFs must be bound to appropriate UIO drivers as required, same -way it was done with the physical function previously. - -Enabling SR-IOV via vfio driver is pretty much the same, except that the file -name is different: - -.. code-block:: console - - echo > /sys/bus/pci/devices/\:\:./sriov_numvfs +For more details on how to bind the PF device and create VF devices, see +:ref:`linux_gsg_binding_kernel`. Configure the VFs through PF diff --git a/doc/guides/bbdevs/fpga_5gnr_fec.rst b/doc/guides/bbdevs/fpga_5gnr_fec.rst index 9d71585e9e..d126c9c3e4 100644 --- a/doc/guides/bbdevs/fpga_5gnr_fec.rst +++ b/doc/guides/bbdevs/fpga_5gnr_fec.rst @@ -72,76 +72,11 @@ When the device first powers up, its PCI Physical Functions (PF) can be listed t sudo lspci -vd8086:0d8f The physical and virtual functions are compatible with Linux UIO drivers: -``vfio`` and ``igb_uio``. However, in order to work the FPGA 5GNR FEC device firstly needs +``vfio_pci`` and ``igb_uio``. However, in order to work the FPGA 5GNR FEC device firstly needs to be bound to one of these linux drivers through DPDK. - -Bind PF UIO driver(s) -~ - -Install the DPDK igb_uio driver, bind it with the PF PCI device ID and use -``lspci`` to confirm the PF device is under use by ``igb_uio`` DPDK UIO driver. - -The igb_uio driver may be bound to the PF PCI device using one of two methods: - - -1. PCI functions (physical or virtual, depending on the use case) can be bound to -the UIO driver by repeating this command for every function. - -.. code-block:: console - - insmod igb_uio.ko - echo "8086 0d8f" > /sys/bus/pci/drivers/igb_uio/new_id - lspci -vd8086:0d8f - - -2. Another way to bind PF with DPDK UIO driver is by using the ``dpdk-devbind.py`` tool - -.. code-block:: console - - cd - ./usertools/dpdk-devbind.py -b igb_u
Re: [PATCH v3] vhost: add IRQ suppression
On Fri, Sep 29, 2023 at 12:38 PM Maxime Coquelin wrote: > > Guest notifications offloading, which has been introduced > in v23.07, aims at offloading syscalls out of the datapath. > > This patch optimizes the offloading by not offloading the > guest notification for a given virtqueue if one is already > being offloaded by the application. > > With a single VDUSE device, we can already see few > notifications being suppressed when doing throughput > testing with Iperf3. We can expect to see much more being > suppressed when the offloading thread is under pressure. > > Signed-off-by: Maxime Coquelin This looks like a good idea. Reviewed-by: David Marchand -- David Marchand
Re: [PATCH v6 1/3] eal: add x86 cpuid support for monitorx
On Mon, Oct 9, 2023 at 6:24 PM Patrick Robb wrote: > > Recheck-request: iol-unit-amd64-testing > > Failed for service_autotest on windows. I'm doing a retest to see if it's > reproducible. Thanks for checking. This unit test has never been entirely reliable... I see the rerun was fine. I'll go ahead with this series. -- David Marchand
Re: [PATCH v6 3/3] power: amd power monitor support
Hello Siva,
On Mon, Oct 9, 2023 at 4:06 PM Sivaprasad Tummala
wrote:
>
> mwaitx allows EPYC processors to enter a implementation dependent
> power/performance optimized state (C1 state) for a specific period
> or until a store to the monitored address range.
>
> Signed-off-by: Sivaprasad Tummala
> Acked-by: Anatoly Burakov
> ---
Please put some changelog to make life easier for reviewers (and me).
I diffed with the previous version to check what had been changed and
I see this:
static void amd_mwaitx(const uint64_t timeout)
...
- "c"(2), /* enable timer */
- "b"(timeout));
+ "c"(0)); /* no time-out */
I will take this series as is, but please confirm why this change was needed.
> lib/eal/x86/rte_power_intrinsics.c | 108 ++---
> 1 file changed, 84 insertions(+), 24 deletions(-)
>
> diff --git a/lib/eal/x86/rte_power_intrinsics.c
> b/lib/eal/x86/rte_power_intrinsics.c
> index 664cde01e9..0d2953f570 100644
> --- a/lib/eal/x86/rte_power_intrinsics.c
> +++ b/lib/eal/x86/rte_power_intrinsics.c
> @@ -17,6 +17,78 @@ static struct power_wait_status {
> volatile void *monitor_addr; /**< NULL if not currently sleeping */
> } __rte_cache_aligned wait_status[RTE_MAX_LCORE];
>
> +/**
> + * This functions uses UMONITOR/UMWAIT instructions and will enter C0.2
> state.
Fixed while applying, function*
> + * For more information about usage of these instructions, please refer to
> + * Intel(R) 64 and IA-32 Architectures Software Developer's Manual.
> + */
> +static void intel_umonitor(volatile void *addr)
> +{
> +#if defined(RTE_TOOLCHAIN_MSVC) || defined(__WAITPKG__)
> + /* cast away "volatile" when using the intrinsic */
> + _umonitor((void *)(uintptr_t)addr);
> +#else
> + /*
> +* we're using raw byte codes for compiler versions which
> +* don't support this instruction natively.
> +*/
> + asm volatile(".byte 0xf3, 0x0f, 0xae, 0xf7;"
> + :
> + : "D"(addr));
> +#endif
> +}
> +
> +static void intel_umwait(const uint64_t timeout)
> +{
> + const uint32_t tsc_l = (uint32_t)timeout;
> + const uint32_t tsc_h = (uint32_t)(timeout >> 32);
> +#if defined(RTE_TOOLCHAIN_MSVC) || defined(__WAITPKG__)
> + _umwait(tsc_l, tsc_h);
> +#else
> + asm volatile(".byte 0xf2, 0x0f, 0xae, 0xf7;"
> + : /* ignore rflags */
> + : "D"(0), /* enter C0.2 */
> + "a"(tsc_l), "d"(tsc_h));
> +#endif
> +}
> +
> +/**
> + * This functions uses MONITORX/MWAITX instructions and will enter C1 state.
> + * For more information about usage of these instructions, please refer to
> + * AMD64 Architecture Programmer’s Manual.
> + */
> +static void amd_monitorx(volatile void *addr)
> +{
> +#if defined(__MWAITX__)
This part probably breaks build with MSVC.
I could not determine whether MSVC supports this intrinsic.
I'll rely on Tyler to fix it later.
Series applied.
--
David Marchand
Re: [PATCH v3] power: support amd-pstate cpufreq driver
On Mon, Oct 9, 2023 at 4:55 PM Sivaprasad Tummala wrote: > > amd-pstate introduces a new CPU frequency control mechanism for AMD > EPYC processors using the ACPI Collaborative Performance Power Control > feature for a finer grained frequency management. > > Patch to add support for amd-pstate driver. > > Signed-off-by: Sivaprasad Tummala > Acked-by: Anatoly Burakov Applied, thanks. -- David Marchand
Re: [PATCH] random: clarify PRNG MT safety guarantees
On Wed, Oct 4, 2023 at 2:06 PM Morten Brørup wrote: > > > From: Mattias Rönnblom [mailto:mattias.ronnb...@ericsson.com] > > Sent: Wednesday, 4 October 2023 12.55 > random: clarify PRNG MT safety guarantees > > > > Clarify MT safety guarantees for unregistered non-EAL threads calling > > PRNG functions in rte_random.h. > > > > Clarify that rte_srand() is not MT safe in regards to calls to > > rte_rand_max() and rte_drand(). > > > > Suggested-by: Stephen Hemminger > > Signed-off-by: Mattias Rönnblom > Acked-by: Morten Brørup Applied, thanks Mattias. -- David Marchand
Re: [PATCH v3] hash: fix SSE comparison
On Mon, Oct 9, 2023 at 4:33 PM Bruce Richardson
wrote:
>
> On Sat, Oct 07, 2023 at 03:36:34PM +0800, Jieqiang Wang wrote:
> > __mm_cmpeq_epi16 returns 0x if the corresponding 16-bit elements are
> > equal. In original SSE2 implementation for function compare_signatures,
> > it utilizes _mm_movemask_epi8 to create mask from the MSB of each 8-bit
> > element, while we should only care about the MSB of lower 8-bit in each
> > 16-bit element.
> > For example, if the comparison result is all equal, SSE2 path returns
> > 0x while NEON and default scalar path return 0x.
> > Although this bug is not causing any negative effects since the caller
> > function solely examines the trailing zeros of each match mask, we
> > recommend this fix to ensure consistency with NEON and default scalar
> > code behaviors.
> >
> > Fixes: c7d93df552c2 ("hash: use partial-key hashing")
> > Cc: sta...@dpdk.org
> >
> > v2:
> > 1. Utilize scalar mask instead of vector mask to save extra loads (Bruce)
> >
> > v3:
> > 1. Fix coding style warnings
Changelog and other notes on a patch should not be in the commitlog
itself, but should go after ---.
https://doc.dpdk.org/guides/contributing/patches.html#creating-patches
> >
> > Signed-off-by: Feifei Wang
> > Signed-off-by: Jieqiang Wang
> > Reviewed-by: Ruifeng Wang
> Acked-by: Bruce Richardson
Applied, thanks Jieqiang and Bruce.
--
David Marchand
Re: [PATCH v6 2/3] test: add dispatcher test suite
On Mon, Oct 9, 2023 at 8:22 PM Mattias Rönnblom
wrote:
> +static int
> +test_dispatcher(void)
> +{
> + return unit_test_suite_runner(&test_suite);
> +}
> +
> +REGISTER_FAST_TEST(dispatcher_autotest, false, true, test_dispatcher);
Since this test expects some lcores, wdyt of adding:
@@ -1044,6 +1044,12 @@ static struct unit_test_suite test_suite = {
static int
test_dispatcher(void)
{
+ if (rte_lcore_count() < NUM_SERVICE_CORES + 1) {
+ printf("Not enough cores for dispatcher_autotest,
expecting at least %u\n",
+ NUM_SERVICE_CORES + 1);
+ return TEST_SKIPPED;
+ }
+
return unit_test_suite_runner(&test_suite);
}
This should avoid the failures we get with some CI env.
(additionnally, I tested this on my laptop and the test runs fine)
--
David Marchand
Re: [PATCH v6 3/3] doc: add dispatcher programming guide
On Mon, Oct 9, 2023 at 8:23 PM Mattias Rönnblom wrote: [snip] > +A module may use more than one event handler, for convenience or to > +further decouple sub-modules. However, the dispatcher may impose an > +upper limit of the number handlers. In addition, installing a large > +number of handlers increase dispatcher overhead, although this does > +not nessarily translate to a system-level performance degradation. See necessarily* [snip] > +Event Clustering > + > + > +The dispatcher maintains the order of events destined for the same > +handler. > + > +*Order* here refers to the order in which the events were delivered > +from the event device to the dispatcher (i.e., in the event array > +populated by ``rte_event_dequeue_burst()``), in relation to the order > +in which the dispatcher deliveres these events to the application. > + > +The dispatcher *does not* guarantee to maintain the order of events > +delivered to *different* handlers. > + > +For example, assume that ``MODULE_A_QUEUE_ID`` expands to the value 0, > +and ``MODULE_B_STAGE_0_QUEUE_ID`` expands to the value 1. Then > +consider a scenario where the following events are dequeued from the > +event device (qid is short for event queue id). > + > +.. code-block:: Surprisingly, Ubuntu in GHA sphinx complains about this code-block directive while generating on my Fedora runs fine... FAILED: doc/guides/html /usr/bin/python3 ../buildtools/call-sphinx-build.py /usr/bin/sphinx-build 23.11.0-rc0 /home/runner/work/dpdk/dpdk/doc/guides /home/runner/work/dpdk/dpdk/build/doc/guides -a -W Warning, treated as error: /home/runner/work/dpdk/dpdk/doc/guides/prog_guide/dispatcher_lib.rst:253:Error in "code-block" directive: 1 argument(s) required, 0 supplied. .. code-block:: [e0: qid=1], [e1: qid=1], [e2: qid=0], [e3: qid=1] Looking at https://www.sphinx-doc.org/en/master/usage/restructuredtext/directives.html#directive-code-block, I suspect there is probably a difference in the default settings of sphinx in those Ubuntu containers. This is pseudo-code / close to C, so we could probably mark this block as "C", but "none" works fine too. WDYT? -- David Marchand
[PATCH] eventdev: fix symbol export for port maintenance
Trying to call rte_event_maintain out of the eventdev library triggers
a link failure, as the tracepoint symbol associated to this inline
helper was not exported.
Fixes: 54f17843a887 ("eventdev: add port maintenance API")
Cc: sta...@dpdk.org
Signed-off-by: David Marchand
---
Caught by the CI when testing the dispatcher library.
See for example:
https://github.com/ovsrobot/dpdk/actions/runs/6460514355/job/17538348529#step:19:5506
---
lib/eventdev/version.map | 1 +
1 file changed, 1 insertion(+)
diff --git a/lib/eventdev/version.map b/lib/eventdev/version.map
index b03c10d99f..249eb115b1 100644
--- a/lib/eventdev/version.map
+++ b/lib/eventdev/version.map
@@ -5,6 +5,7 @@ DPDK_24 {
__rte_eventdev_trace_deq_burst;
__rte_eventdev_trace_enq_burst;
__rte_eventdev_trace_eth_tx_adapter_enqueue;
+ __rte_eventdev_trace_maintain;
__rte_eventdev_trace_timer_arm_burst;
__rte_eventdev_trace_timer_arm_tmo_tick_burst;
__rte_eventdev_trace_timer_cancel_burst;
--
2.41.0
Re: [PATCH v6 2/3] test: add dispatcher test suite
On Mon, Oct 9, 2023 at 8:22 PM Mattias Rönnblom wrote: > > Add unit tests for the dispatcher. Fyi, this patch is the first external user of rte_event_maintain and it revealed an issue: http://mails.dpdk.org/archives/test-report/2023-October/475671.html I sent a fix, can you have a look? https://patchwork.dpdk.org/project/dpdk/patch/20231010140029.66159-1-david.march...@redhat.com/ Thanks. -- David Marchand
Re: [PATCH] eventdev: fix symbol export for port maintenance
On Wed, Oct 11, 2023 at 8:47 AM Mattias Rönnblom wrote:
>
> On 2023-10-10 16:00, David Marchand wrote:
> > Trying to call rte_event_maintain out of the eventdev library triggers
> > a link failure, as the tracepoint symbol associated to this inline
> > helper was not exported.
> >
> > Fixes: 54f17843a887 ("eventdev: add port maintenance API")
> > Cc: sta...@dpdk.org
> >
> > Signed-off-by: David Marchand
> > ---
> > Caught by the CI when testing the dispatcher library.
> > See for example:
> > https://github.com/ovsrobot/dpdk/actions/runs/6460514355/job/17538348529#step:19:5506
> >
> > ---
> > lib/eventdev/version.map | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/lib/eventdev/version.map b/lib/eventdev/version.map
> > index b03c10d99f..249eb115b1 100644
> > --- a/lib/eventdev/version.map
> > +++ b/lib/eventdev/version.map
> > @@ -5,6 +5,7 @@ DPDK_24 {
> > __rte_eventdev_trace_deq_burst;
> > __rte_eventdev_trace_enq_burst;
> > __rte_eventdev_trace_eth_tx_adapter_enqueue;
> > + __rte_eventdev_trace_maintain;
> > __rte_eventdev_trace_timer_arm_burst;
> > __rte_eventdev_trace_timer_arm_tmo_tick_burst;
> > __rte_eventdev_trace_timer_cancel_burst;
>
> I can't say I know why it's needed, but the change seems consistent with
> other Eventdev trace points.
The trace point framework in DPDK relies on a per trace point global
variable (whose name is __):
#define __RTE_TRACE_POINT(_mode, _tp, _args, ...) \
extern rte_trace_point_t __##_tp; \
static __rte_always_inline void \
_tp _args \
{ \
__rte_trace_point_emit_header_##_mode(&__##_tp); \
__VA_ARGS__ \
}
When tracepoints are called from within a shared library code, and
because all symbols of a group of objects are visible, the tracepoint
symbols are resolved by the linker.
But when this tracepoint is called via an inline helper from some code
out of the shared library, this symbol must be exported in the shared
library map or it won't be visible to "external" users.
--
David Marchand
Re: [PATCH] eventdev: fix symbol export for port maintenance
Hello Jerin,
On Wed, Oct 11, 2023 at 8:51 AM Jerin Jacob wrote:
>
> On Tue, Oct 10, 2023 at 7:30 PM David Marchand
> wrote:
> >
> > Trying to call rte_event_maintain out of the eventdev library triggers
> > a link failure, as the tracepoint symbol associated to this inline
> > helper was not exported.
> >
> > Fixes: 54f17843a887 ("eventdev: add port maintenance API")
> > Cc: sta...@dpdk.org
> >
> > Signed-off-by: David Marchand
>
> Acked-by: Jerin Jacob
>
> David, If it is stopping dispatcher library integration, please take
> this patch through main tree, if not, I will add through event tree
> for rc2.
I was going to suggest merging directly in main :-).
I will delegate it to me in patchwork.
Thanks.
--
David Marchand
Re: [PATCH] eventdev: fix symbol export for port maintenance
Jerin,
On Wed, Oct 11, 2023 at 9:03 AM David Marchand
wrote:
>
> On Wed, Oct 11, 2023 at 8:47 AM Mattias Rönnblom
> wrote:
> >
> > On 2023-10-10 16:00, David Marchand wrote:
> > > Trying to call rte_event_maintain out of the eventdev library triggers
> > > a link failure, as the tracepoint symbol associated to this inline
> > > helper was not exported.
> > >
> > > Fixes: 54f17843a887 ("eventdev: add port maintenance API")
> > > Cc: sta...@dpdk.org
> > >
> > > Signed-off-by: David Marchand
> > > ---
> > > Caught by the CI when testing the dispatcher library.
> > > See for example:
> > > https://github.com/ovsrobot/dpdk/actions/runs/6460514355/job/17538348529#step:19:5506
> > >
> > > ---
> > > lib/eventdev/version.map | 1 +
> > > 1 file changed, 1 insertion(+)
> > >
> > > diff --git a/lib/eventdev/version.map b/lib/eventdev/version.map
> > > index b03c10d99f..249eb115b1 100644
> > > --- a/lib/eventdev/version.map
> > > +++ b/lib/eventdev/version.map
> > > @@ -5,6 +5,7 @@ DPDK_24 {
> > > __rte_eventdev_trace_deq_burst;
> > > __rte_eventdev_trace_enq_burst;
> > > __rte_eventdev_trace_eth_tx_adapter_enqueue;
> > > + __rte_eventdev_trace_maintain;
> > > __rte_eventdev_trace_timer_arm_burst;
> > > __rte_eventdev_trace_timer_arm_tmo_tick_burst;
> > > __rte_eventdev_trace_timer_cancel_burst;
> >
> > I can't say I know why it's needed, but the change seems consistent with
> > other Eventdev trace points.
>
> The trace point framework in DPDK relies on a per trace point global
> variable (whose name is __):
>
> #define __RTE_TRACE_POINT(_mode, _tp, _args, ...) \
> extern rte_trace_point_t __##_tp; \
> static __rte_always_inline void \
> _tp _args \
> { \
> __rte_trace_point_emit_header_##_mode(&__##_tp); \
> __VA_ARGS__ \
> }
>
> When tracepoints are called from within a shared library code, and
> because all symbols of a group of objects are visible, the tracepoint
> symbols are resolved by the linker.
> But when this tracepoint is called via an inline helper from some code
> out of the shared library, this symbol must be exported in the shared
> library map or it won't be visible to "external" users.
Could we describe / mention this in the trace point library doc?
Or maybe I read too quickly and there is already something but it was
not obvious to me.
--
David Marchand
Re: [PATCH v6 2/3] test: add dispatcher test suite
On Wed, Oct 11, 2023 at 8:28 AM Mattias Rönnblom wrote:
>
> On 2023-10-10 13:56, David Marchand wrote:
> > On Mon, Oct 9, 2023 at 8:22 PM Mattias Rönnblom
> > wrote:
> >> +static int
> >> +test_dispatcher(void)
> >> +{
> >> + return unit_test_suite_runner(&test_suite);
> >> +}
> >> +
> >> +REGISTER_FAST_TEST(dispatcher_autotest, false, true, test_dispatcher);
> >
> > Since this test expects some lcores, wdyt of adding:
> >
> > @@ -1044,6 +1044,12 @@ static struct unit_test_suite test_suite = {
> > static int
> > test_dispatcher(void)
> > {
> > + if (rte_lcore_count() < NUM_SERVICE_CORES + 1) {
> > + printf("Not enough cores for dispatcher_autotest,
> > expecting at least %u\n",
> > + NUM_SERVICE_CORES + 1);
> > + return TEST_SKIPPED;
> > + }
> > +
> > return unit_test_suite_runner(&test_suite);
> > }
> >
> > This should avoid the failures we get with some CI env.
> > (additionnally, I tested this on my laptop and the test runs fine)
> >
> >
>
> Indeed, this is a much better way than to fail the test case.
>
> I'm thinking this is best done in test_setup(), since it's related to
> the setup. In case other test cases are added that required a different
> setup, there may be no minimum lcore requirement.
This is what I had tried as a first attempt but as I hit some crashes
in the teardown step, I went with the easiest fix.
>
> You will get multiple (four, for the moment) print-outs though, in case
> you run with fewer than 4 lcores.
>
> I'll also make sure I skip (and not fail) the tests in case the DSW
> event device is not included in the build.
>
Yep, it is better like this.
Thanks for v7, I'll have a look today.
--
David Marchand
Re: [PATCH] doc: remove confusing command to send patch
On Tue, Oct 10, 2023 at 6:26 PM Thomas Monjalon wrote: > > In the contributor guide, it was said that no need to Cc maintainers > for new additions, probably for new directories not having a maintainer. > There is no harm, and it is a good habit, to always Cc maintainers. > > Remove this case as it can mislead to not Cc maintainers when needed. > > Signed-off-by: Thomas Monjalon I agree Cc: maintainers should be the default / recommended way of sending patches. Just to convince myself, adding some meson skeleton for a "plop" library, adding an entry in the release notes and hooking in lib/meson.build: $ git show --stat doc/guides/rel_notes/release_23_11.rst | 4 lib/meson.build| 1 + lib/plop/meson.build | 2 ++ $ ./devtools/get-maintainer.sh 0001-new-awesome-library.patch In this case, it translates to an empty To: list if you follow the example command line: git send-email --to-cmd ./devtools/get-maintainer.sh --cc dev@dpdk.org 000*.patch We could add a default list of recipients if no maintainer is found by the script. And the next question is who should be in that list.. -- David Marchand
Re: [PATCH] common/qat: enable gen4 c devices
Hello Ciara, On Mon, Aug 21, 2023 at 11:37 AM Power, Ciara wrote: > > > > > > +-+-+-+-+--+---+---++--- > > -+--+++ > > > | Yes | No | No | 4 | 401xxx | IDZ/ N/A | qat_401xxx| > > > 4xxx | > > 4942 | 2| 4943 | 16 | > > > > > > +-+-+-+-+--+---+---+-- > > > --++--+++ > > > + | Yes | Yes | Yes | 4 | 402xxx | linux/6.4+| qat_402xxx| > > > 4xxx | > > 4944 | 2| 4945 | 16 | > > > + > > > +-+-+-+-+--+---+---++-- > > --+--+++ > > > + | Yes | No | No | 4 | 402xxx | IDZ/ N/A | qat_402xxx| > > > 4xxx | > > 4944 | 2| 4945 | 16 | > > > + > > > + +-+-+-+-+--+---+---+ > > > + ++--+++ > > > > Is there such a kernel module named qat_402xxx upstream? > > I can only find qat_4xxx. > > > Good catch, you're right, there is no kernel module 402xxx. > These devices fall under the original 4xxx driver. > Will update here, and send a fix for the 401xxx entry later. I noticed this patch for 402xxx pulled in the main branch. Don't forget to send the fix on 401xxx entry please. -- David Marchand
Re: [PATCH] eventdev: fix symbol export for port maintenance
On Wed, Oct 11, 2023 at 8:51 AM Jerin Jacob wrote:
> On Tue, Oct 10, 2023 at 7:30 PM David Marchand
> wrote:
> >
> > Trying to call rte_event_maintain out of the eventdev library triggers
> > a link failure, as the tracepoint symbol associated to this inline
> > helper was not exported.
> >
> > Fixes: 54f17843a887 ("eventdev: add port maintenance API")
> > Cc: sta...@dpdk.org
> >
> > Signed-off-by: David Marchand
> Acked-by: Jerin Jacob
Applied thanks.
--
David Marchand
Re: [PATCH] doc: sort build and EAL features in the release notes
On Wed, Oct 11, 2023 at 9:54 AM Thomas Monjalon wrote: > > When adding build and EAL features in 23.11, > the format and sorting order was unusual. Indeed, my bad. > This change is making these features similar as others. > > Signed-off-by: Thomas Monjalon Acked-by: David Marchand Applied, thanks. -- David Marchand
Re: [PATCH v5 1/3] lib: introduce dispatcher library
On Mon, Oct 9, 2023 at 6:50 PM Mattias Rönnblom wrote:
[snip]
> >>>> +static int
> >>>> +evd_set_service_runstate(struct rte_dispatcher *dispatcher, int state)
> >>>> +{
> >>>> + int rc;
> >>>> +
> >>>> + rc = rte_service_component_runstate_set(dispatcher->service_id,
> >>>> + state);
> >>>> +
> >>>> + if (rc != 0) {
> >>>> + RTE_EDEV_LOG_ERR("Unexpected error %d occurred while
> >>>> setting "
> >>>> +"service component run state to %d\n",
> >>>> rc,
> >>>> +state);
> >>>> + RTE_ASSERT(0);
> >>>
> >>> Why not propagating the error to callers?
> >>>
> >>>
> >>
> >> The root cause would be a programming error, hence an assertion is more
> >> appropriate way to deal with the situation.
> >
> > Without building RTE_ENABLE_ASSERT (disabled by default), the code
> > later in this function will still be executed.
> >
>
> If RTE_ASSERT() is not the way to assure a consistent internal library
> state, what is? RTE_VERIFY()?
The usual way in DPDK is to use RTE_VERIFY or rte_panic with the error message.
There is also libc assert().
RTE_ASSERT is more of a debug macro since it is under a build option.
But by making the library "panic" on some assertion, I have followup comments:
- what is the point of returning an int for rte_dispatcher_start() /
rte_dispatcher_stop()?
- rte_dispatcher_start() and rte_dispatcher_stop() (doxygen)
documentation needs updating, as they can't return anything but 0.
--
David Marchand
Re: [PATCH] ethdev: clarify device queue state after start and stop
"Wrong stopped Rx queue(%u) state(%d)\n",
> + queue_id, rx_qinfo.queue_state);
> + }
> +
> + /* Stopped TxQ */
> + for (uint16_t queue_id = 0; queue_id < dev_info.nb_tx_queues;
> queue_id++) {
> + ret = rte_eth_tx_queue_info_get(port_id, queue_id,
> &tx_qinfo);
> + if (ret == -ENOTSUP)
> + continue;
> +
> + TEST_ASSERT(ret == 0,
> + "Port(%u), queue(%u) failed to get TxQ
> info.\n",
> + port_id, queue_id);
> +
> + TEST_ASSERT(tx_qinfo.queue_state ==
> RTE_ETH_QUEUE_STATE_STOPPED,
> + "Wrong stopped Tx queue(%u) state(%d)\n",
> + queue_id, tx_qinfo.queue_state);
> + }
> + }
> +
> + return TEST_SUCCESS;
> +}
> +
> +static struct unit_test_suite ethdev_api_testsuite = {
> + .suite_name = "ethdev API tests",
> + .setup = NULL,
> + .teardown = NULL,
> + .unit_test_cases = {
> + TEST_CASE(ethdev_api_queue_status),
> + /* TODO: Add deferred_start queue status test */
> + TEST_CASES_END() /**< NULL terminate unit test array */
> + }
> +};
> +
> +static int
> +test_ethdev_api(void)
> +{
> + rte_log_set_global_level(RTE_LOG_DEBUG);
> + rte_log_set_level(RTE_LOGTYPE_EAL, RTE_LOG_DEBUG);
> +
> + return unit_test_suite_runner(ðdev_api_testsuite);
> +}
> +
> +REGISTER_TEST_COMMAND(ethdev_api, test_ethdev_api);
REGISTER_FAST_TEST or REGISTER_DRIVER_TEST.
> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
> index 8542257721c9..6441fe049b06 100644
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -2812,6 +2812,9 @@ int rte_eth_dev_tx_queue_stop(uint16_t port_id,
> uint16_t tx_queue_id);
> * Device RTE_ETH_DEV_NOLIVE_MAC_ADDR flag causes MAC address to be set
> before
> * PMD port start callback function is invoked.
> *
> + * All device queues (except form deferred start queues) status should be
> + * `RTE_ETH_QUEUE_STATE_STARTED` after start.
> + *
> * On success, all basic functions exported by the Ethernet API (link status,
> * receive/transmit, and so on) can be invoked.
> *
> @@ -2828,6 +2831,8 @@ int rte_eth_dev_start(uint16_t port_id);
> * Stop an Ethernet device. The device can be restarted with a call to
> * rte_eth_dev_start()
> *
> + * All device queues status should be `RTE_ETH_QUEUE_STATE_STOPPED` after
> stop.
> + *
> * @param port_id
> * The port identifier of the Ethernet device.
> * @return
> --
> 2.34.1
>
The rest lgtm.
--
David Marchand
Re: [PATCH] bus/vmbus: add support allow/block scan mode
On Thu, Jun 9, 2022 at 10:46 AM Xiaoming Jiang wrote: > > bus/vmbus: add support allow/block scan mode > > Signed-off-by: Xiaoming Jiang Acked-by: Long Li Acked-by: Huisong Li Applied, thanks Xiaoming. -- David Marchand
Re: [PATCH 0/2] simplify building x86 code with AVX2 support
Hello Bruce, On Thu, Jul 27, 2023 at 11:31 AM Bruce Richardson wrote: > > Inside our optimized vector drivers (and libs), there were always build > time checks for various levels of instruction set support, most > notably AVX2 and AVX-512 on x86 systems. One of the checks done in > each case was verifying that the compiler used was able to generate > AVX code appropriately. However, since this was originally done, > our minimum compiler support has been updated and so we no longer > need to check this - all supported compilers can generate AVX2. > This allows us to simplify the build logic for the x86 vector code. > > In future, we can do similarly for AVX-512. > > Bruce Richardson (2): > build/x86: remove conditional checks for AVX2 support > build: remove unnecessary AVX2 compiler flag > > drivers/net/bnxt/bnxt_ethdev.c | 8 > drivers/net/bnxt/bnxt_rxr.h | 2 +- > drivers/net/bnxt/bnxt_txr.h | 2 +- > drivers/net/bnxt/meson.build| 26 + > drivers/net/enic/meson.build| 10 +++--- > drivers/net/i40e/i40e_rxtx.c| 14 - > drivers/net/i40e/meson.build| 22 ++--- > drivers/net/iavf/iavf_rxtx_vec_common.h | 2 +- > drivers/net/iavf/meson.build| 22 ++--- > drivers/net/ice/meson.build | 21 ++-- > lib/acl/meson.build | 23 +- > lib/acl/rte_acl.c | 10 +++--- > 12 files changed, 49 insertions(+), 113 deletions(-) Sorry, this series had fallen through the cracks... though I intended to apply with other build update series. It still applies fine, I ran my checks and I see nothing wrong with the series. Applied, thanks for the cleanup. -- David Marchand
Re: [PATCH v8 0/3] Add dispatcher library
On Thu, Oct 12, 2023 at 10:55 AM Mattias Rönnblom wrote: > > The purpose of the dispatcher library is to decouple different parts > of an eventdev-based application (e.g., processing pipeline stages), > sharing the same underlying event device. > > The dispatcher replaces the conditional logic (often, a switch > statement) that typically follows an event device dequeue operation, > where events are dispatched to different parts of the application > based on event meta data, such as the queue id or scheduling type. > > The concept is similar to a UNIX file descriptor event loop library. > Instead of tying callback functions to fds as for example libevent > does, the dispatcher relies on application-supplied matching callback > functions to decide where to deliver events. > > A dispatcher is configured to dequeue events from a specific event > device, and ties into the service core framework, to do its (and the > application's) work. > > The dispatcher provides a convenient way for an eventdev-based > application to use service cores for application-level processing, and > thus for sharing those cores with other DPDK services. > > Although the dispatcher adds some overhead, experience suggests that > the net effect on the application (both synthetic benchmarks and more > real-world applications) may well be positive. This is primarily due > to clustering (see programming guide) reducing cache misses. > > Benchmarking indicates that the overhead is ~10 cc/event (on a > large core), with a handful of often-used handlers. > > The dispatcher does not support run-time reconfiguration. > > The use of the dispatcher library is optional, and an eventdev-based > application may still opt to access the event device using direct > eventdev API calls, or by some other means. > > Mattias Rönnblom (3): > lib: introduce dispatcher library > test: add dispatcher test suite > doc: add dispatcher programming guide > > MAINTAINERS |6 + > app/test/meson.build |1 + > app/test/test_dispatcher.c | 1056 ++ > doc/api/doxy-api-index.md|1 + > doc/api/doxy-api.conf.in |1 + > doc/guides/prog_guide/dispatcher_lib.rst | 433 + > doc/guides/prog_guide/index.rst |1 + > doc/guides/rel_notes/release_23_11.rst |5 + > lib/dispatcher/meson.build | 13 + > lib/dispatcher/rte_dispatcher.c | 694 ++ > lib/dispatcher/rte_dispatcher.h | 458 ++ > lib/dispatcher/version.map | 20 + > lib/meson.build |2 + > 13 files changed, 2691 insertions(+) > create mode 100644 app/test/test_dispatcher.c > create mode 100644 doc/guides/prog_guide/dispatcher_lib.rst > create mode 100644 lib/dispatcher/meson.build > create mode 100644 lib/dispatcher/rte_dispatcher.c > create mode 100644 lib/dispatcher/rte_dispatcher.h > create mode 100644 lib/dispatcher/version.map > Thanks for this latest revision, it lgtm. I fixed a few grammar issues in the documentation and used simple .. note:: blocks to be consistent with the rest of our docs. Applied, thanks Mattias. -- David Marchand
Re: [PATCH v3 0/5] document and simplify use of cmdline
On Wed, Oct 11, 2023 at 3:34 PM Bruce Richardson wrote: > > The DPDK commandline library is widely used by apps and examples within > DPDK, but it is not documented in our programmers guide and it requires > a lot of boilerplate code definitions in order to used. We can improve > this situation by creating a simple python script to automatically > generate the boilerplate from a list of commands. > > This patchset contains a new documentation chapter on cmdline library, > going through step-by-step how to add commands and create the necessary > token lists and parse contexts. > > Following that initial doc patch, the set then contains a > boilerplate-generating script, as well as a set of three patches showing > its use, by converting three examples to use the script instead of > having the hard-coded boilerplate. Once the script is used, adding a new > command becomes as simple as adding the desired command to the .list > file, and then writing the required function which will be called for > that command. No other boilerplate coding is necessary. > > Script obviously does not cover the full range of capabilities of the > commandline lib, but does cover the most used parts. The code-saving to > each of the examples by auto-generating the boilerplate is significant, > and probably more examples with commandlines can be converted over in > future. > > The "cmdline" example itself, is not converted over, as it should > probably remain as a simple example of direct library use without the > script. > > V3: > * Added lots of documentation > * Added support for help text for each command > * Cleaned up script a little so it passes pycodestyle and most flake8 > checks, when line-length is set to max 100. > * Removed RFC tag, as I consider this patchset stable enough for > consideration in a release. > > V2-RFC: > * Add support for IP addresses in commands > * Move to buildtools directory and make installable > * Convert 3 examples to use script, and eliminate their boilerplate > > Bruce Richardson (5): > doc/prog_guide: new chapter on cmdline library > buildtools: script to generate cmdline boilerplate > examples/simple_mp: auto-generate cmdline boilerplate > examples/hotplug_mp: auto-generate cmdline boilerplate > examples/bond: auto-generate cmdline boilerplate > > app/test/commands.c | 2 + > buildtools/dpdk-cmdline-gen.py| 167 +++ > buildtools/meson.build| 7 + > doc/guides/prog_guide/cmdline.rst | 466 ++ > doc/guides/prog_guide/index.rst | 1 + > examples/bond/Makefile| 12 +- > examples/bond/commands.list | 6 + > examples/bond/main.c | 161 +- > examples/bond/main.h | 10 - > examples/bond/meson.build | 8 + > examples/multi_process/hotplug_mp/Makefile| 12 +- > examples/multi_process/hotplug_mp/commands.c | 147 +- > examples/multi_process/hotplug_mp/commands.h | 10 - > .../multi_process/hotplug_mp/commands.list| 5 + > examples/multi_process/hotplug_mp/meson.build | 9 + > examples/multi_process/simple_mp/Makefile | 12 +- > examples/multi_process/simple_mp/meson.build | 9 + > .../multi_process/simple_mp/mp_commands.c | 106 +--- > .../multi_process/simple_mp/mp_commands.h | 14 - > .../multi_process/simple_mp/mp_commands.list | 3 + > 20 files changed, 745 insertions(+), 422 deletions(-) Interesting series. So if we remove the doc addition, this patch is removing loc, so +1 from me :-). There is a problem though with externally building the examples. Maybe the dpdk-cmdline-gen.py has not been exported in the install process. > create mode 100755 buildtools/dpdk-cmdline-gen.py > create mode 100644 doc/guides/prog_guide/cmdline.rst > create mode 100644 examples/bond/commands.list > delete mode 100644 examples/bond/main.h > delete mode 100644 examples/multi_process/hotplug_mp/commands.h > create mode 100644 examples/multi_process/hotplug_mp/commands.list > delete mode 100644 examples/multi_process/simple_mp/mp_commands.h > create mode 100644 examples/multi_process/simple_mp/mp_commands.list > > -- > 2.39.2 > -- David Marchand
Re: [PATCH v2 0/3] example/l3fwd: relax l3fwd rx RSS/Offload if needed
Hello, On Fri, Oct 13, 2023 at 6:28 AM Trevor Tao wrote: > > This series tries to relax l3fwd rx RSS/Offload mode requirement if they > are not supported by underlying hw or virtual devices, there is an > option named relax_rx_mode added to enable this option. > > Trevor Tao (3): > examples/l3fwd: relax RSS requirement with option > examples/l3fwd: relax the Offload requirement > doc: add a relax rx mode requirement option > > doc/guides/rel_notes/release_23_11.rst | 251 +--- > doc/guides/sample_app_ug/l3_forward.rst | 4 +- > examples/l3fwd/l3fwd.h | 12 +- > examples/l3fwd/l3fwd_em.h | 2 +- > examples/l3fwd/l3fwd_lpm.h | 2 +- > examples/l3fwd/main.c | 30 +++ > 6 files changed, 272 insertions(+), 29 deletions(-) Thanks for the patches. One comment on the form though. I see 4 batches of "v2 patches" in patchwork. https://patchwork.dpdk.org/project/dpdk/list/?submitter=3044&state=* I marked all but the last batch as superseded. If there is something wrong with this, please register to patchwork with your mail address, you will then be able to update the states yourself. It is hard to follow what work has been done between those different iterations of patches. For future submissions, don't forget to version your patches with a new revision (iirc, we should be at version 7 if I count the initial standalone v1 patch). In the cover letter, please provide a changelog of the differences between revisions. If you think it is better, you may put a more detailed changelog in each patch commitlog (as annotations, after ---). Thanks. -- David Marchand
Re: [PATCH v1] doc: update QAT kernel module entry
Hello,
On Fri, Oct 13, 2023 at 1:00 PM Brian Dooley wrote:
>
> The Kernel Module entry for 2.0 devices was incorrect in the doc table.
> Updated table with Kernel Module qat_4xxx.
>
> Fixes: 83ce3b393176 ("doc: update QAT device support")
This does not look like the right Fixes: tag.
The commit that introduced first this reference to qat_401xxx is
f4eac3a09c51 ("common/qat: enable GEN4 b devices").
>
> Signed-off-by: Brian Dooley
Otherwise, lgtm.
Cc: Ciara for info.
--
David Marchand
Re: [PATCH] net/iavf: fix pkt len check
Hello Dexia, On Fri, Oct 13, 2023 at 1:42 PM Dexia Li wrote: > > App usually encap some bytes in mbuf headroom, for example, tunnel > header. When RTE_MBUF_F_TX_TCP_SEG is set, this check will drop packets. > Since the packet will be cut by hw soon, the out packet will not exceed > mtu. > > Signed-off-by: Dexia Li This should be fixed with series: https://patchwork.dpdk.org/project/dpdk/list/?series=29651&state=%2A&archive=both Applied in next-net-intel: https://git.dpdk.org/next/dpdk-next-net-intel/commit/?id=ff67c8a679c2daba282841459d8cd4131b3a85ab Those patches will make it to the main dpdk repo soon. Please double check they work for you. Thanks. -- David Marchand
Re: [PATCH v3] bus/cdx: provide driver flag for optional resource mapping
On Fri, Oct 13, 2023 at 1:52 PM Gangurde, Abhijit wrote: > > > > > > +/** > > > > > + * Map the CDX device resources in user space virtual memory address. > > > > > + * > > > > > + * Note that driver should not call this function when flag > > > > > + * RTE_CDX_DRV_NEED_MAPPING is set, as EAL will do that for > > > > > + * you when it's on. > > > > > > > > Why should we export this function in the application ABI, if it is > > > > only used by drivers? > > > > > > This can be called from an application as well if this flag is not set > > > hence, we > > need to export this function. > > > > What kind of applications / in which usecase, one would need to map > > the device resources? > > Except a driver? > > I understand that it is probably not the ideal use case, but some of the > customers > are using a single application which also registers itself as driver. > Probably such > applications need to use internal APIs instead of making these APIs external. > Will analyze it further and send another rev of this patch. External drivers should be supported with current code. DPDK must be built with enable_driver_sdk option, and the external driver code can include bus_cdx_driver.h. Possibly compiling this code with -DENABLE_INTERNAL_API cflag will be necessary. -- David Marchand
Re: [PATCH] net/iavf: fix pkt len check
On Mon, Oct 16, 2023 at 3:37 AM Dexia Li wrote: > > Thanks for your commit. > It works for me. > Thanks for confirming, I marked this patch as rejected. -- David Marchand
[PATCH] net/bonding: fix link status callback stop
If a bonding port gets released, a link status alarm callback still
referenced the ethdev port that may be reused later.
Cancel this callback when stopping the port.
Bugzilla ID: 1301
Fixes: a45b288ef21a ("bond: support link status polling")
Cc: sta...@dpdk.org
Signed-off-by: David Marchand
---
Note: this issue was made apparent now that we release the ethdev port
shared mem, see 36c46e738120 ("ethdev: cleanup shared data with ...").
---
drivers/net/bonding/rte_eth_bond_pmd.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c
b/drivers/net/bonding/rte_eth_bond_pmd.c
index 122b1187fd..b8ee8be50f 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -2159,6 +2159,10 @@ bond_ethdev_stop(struct rte_eth_dev *eth_dev)
eth_dev->data->dev_link.link_status = RTE_ETH_LINK_DOWN;
eth_dev->data->dev_started = 0;
+ if (internals->link_status_polling_enabled) {
+
rte_eal_alarm_cancel(bond_ethdev_member_link_status_change_monitor,
+ (void *)&rte_eth_devices[internals->port_id]);
+ }
internals->link_status_polling_enabled = 0;
for (i = 0; i < internals->member_count; i++) {
uint16_t member_id = internals->members[i].port_id;
--
2.41.0
Re: [PATCH 0/2] fix enable_stdatomic=true build with clang
On Mon, Oct 16, 2023 at 8:53 PM Tyler Retzlaff wrote: > > We are temporarily exposed while the CI is enabled that tests > CC=clang && -Denable_stdatomic. > > In the meantime two mistakes have been found so fix them to allow > the CI to be enabled. > > Tyler Retzlaff (2): > power: fix use of rte stdatomic > event/cnxk: remove single use of rte stdatomic Thank you for the fixes. Reviewed-by: David Marchand -- David Marchand
Re: [PATCH 0/2] fix enable_stdatomic=true build with clang
On Mon, Oct 16, 2023 at 9:07 PM David Marchand wrote: > > On Mon, Oct 16, 2023 at 8:53 PM Tyler Retzlaff > wrote: > > > > We are temporarily exposed while the CI is enabled that tests > > CC=clang && -Denable_stdatomic. > > > > In the meantime two mistakes have been found so fix them to allow > > the CI to be enabled. > > > > Tyler Retzlaff (2): > > power: fix use of rte stdatomic > > event/cnxk: remove single use of rte stdatomic > > Thank you for the fixes. > Reviewed-by: David Marchand Series applied, thanks. -- David Marchand
Re: [PATCH v4 0/7] document and simplify use of cmdline
Hi Bruce,
On Mon, Oct 16, 2023 at 4:06 PM Bruce Richardson
wrote:
>
> The DPDK commandline library is widely used by apps and examples within
> DPDK, but it is not documented in our programmers guide and it requires
> a lot of boilerplate code definitions in order to used. We can improve
> this situation by creating a simple python script to automatically
> generate the boilerplate from a list of commands.
>
> This patchset contains a new documentation chapter on cmdline library,
> going through step-by-step how to add commands and create the necessary
> token lists and parse contexts.
>
> Following that initial doc patch, the set then contains a
> boilerplate-generating script, as well as a set of four patches showing
> its use, by converting four examples to use the script instead of
> having the hard-coded boilerplate. Once the script is used, adding a new
> command becomes as simple as adding the desired command to the .list
> file, and then writing the required function which will be called for
> that command. No other boilerplate coding is necessary.
>
> Script obviously does not cover the full range of capabilities of the
> commandline lib, but does cover the most used parts. The code-saving to
> each of the examples by auto-generating the boilerplate is significant,
> and probably more examples with commandlines can be converted over in
> future.
This is not something blocking for the series, but I had a quick try
with testpmd to see where this series stands.
I hit, right off the bat, some of those limitations, but I think there
are not that difficult to deal with.
- testpmd accepts both "help" and "help " commands.
But the cmdline library does not provide a way (afair) for specifiying
this "optional" aspect.
And it can't be expressed with this series current syntax since
generated symbols would conflict if we ask for two "help" commands.
My quick hack was to introduce a way to select the prefix of the
generated symbols.
There may be a better way, like finding a better discriminant for
naming symbols...
But, on the other hand, the symbols prefix might be something a
developer wants to control (instead of currently hardcoded cmd_
prefix).
@@ -20,6 +20,12 @@ def process_command(tokens, cfile, comment):
"""Generate the structures and definitions for a single command."""
name = []
+prefix, sep, cmd_name = tokens[0].partition(':')
+if cmd_name:
+tokens[0] = cmd_name
+else:
+prefix = 'cmd'
+
if tokens[0].startswith('<'):
print('Error: each command must start with at least one
literal string', file=sys.stderr)
sys.exit(1)
(etc... I am not copying the rest of the diff)
I then used as:
cmd_brief:help # help: Show help
help section # help: Show help
- Multi choice fixed strings is something that is often used in
testpmd, like here, in the help command.
Here is my quick hack:
diff --git a/buildtools/dpdk-cmdline-gen.py b/buildtools/dpdk-cmdline-gen.py
index 3b41fb0493..e8c9e079de 100755
--- a/buildtools/dpdk-cmdline-gen.py
+++ b/buildtools/dpdk-cmdline-gen.py
@@ -35,7 +35,11 @@ def process_command(tokens, cfile, comment):
for t in tokens:
if t.startswith('<'):
t_type, t_name = t[1:].split('>')
-t_val = 'NULL'
+if len(t_type.split('(')) == 2:
+t_type, t_val = t_type.split('(')
+t_val = '"' + t_val.split(')')[0] + '"'
+else:
+t_val = 'NULL'
else:
t_type = 'STRING'
t_name = t
@@ -113,7 +117,7 @@ def process_commands(infile, hfile, cfile, ctxname):
continue
if '#' not in line:
line = line + '#' # ensure split always works, even if
no help text
-tokens, comment = line.split('#', 1)
+tokens, comment = line.rsplit('#', 1)
instances.append(process_command(tokens.strip().split(),
cfile, comment.strip()))
print(f'static __rte_used cmdline_parse_ctx_t {ctxname}[] = {{')
Which translates as:
cmd_brief:help # help: Show help
help section # help: Show help
>
> The "cmdline" example itself, is not converted over, as it should
> probably remain as a simple example of direct library use without the
> script.
+1
--
David Marchand
[PATCH v2] ci: test stdatomic API
Add some compilation tests with C11 atomics enabled.
The headers check can't be enabled (as gcc and clang don't provide
stdatomic before C++23).
Signed-off-by: David Marchand
Reviewed-by: Aaron Conole
---
Changelog from v1:
- following Thomas offlist review, tweaked coverage in
test-meson-builds.sh to reduce the number of build targets,
---
.ci/linux-build.sh| 6 +-
.github/workflows/build.yml | 8
devtools/test-meson-builds.sh | 4 +++-
3 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh
index e0b62bac90..b09df07b55 100755
--- a/.ci/linux-build.sh
+++ b/.ci/linux-build.sh
@@ -92,7 +92,11 @@ fi
OPTS="$OPTS -Dplatform=generic"
OPTS="$OPTS -Ddefault_library=$DEF_LIB"
OPTS="$OPTS -Dbuildtype=$buildtype"
-OPTS="$OPTS -Dcheck_includes=true"
+if [ "$STDATOMIC" = "true" ]; then
+ OPTS="$OPTS -Denable_stdatomic=true"
+else
+ OPTS="$OPTS -Dcheck_includes=true"
+fi
if [ "$MINI" = "true" ]; then
OPTS="$OPTS -Denable_drivers=net/null"
OPTS="$OPTS -Ddisable_libs=*"
diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
index 7a2ac0ceee..14328622fb 100644
--- a/.github/workflows/build.yml
+++ b/.github/workflows/build.yml
@@ -30,6 +30,7 @@ jobs:
REF_GIT_TAG: none
RISCV64: ${{ matrix.config.cross == 'riscv64' }}
RUN_TESTS: ${{ contains(matrix.config.checks, 'tests') }}
+ STDATOMIC: ${{ contains(matrix.config.checks, 'stdatomic') }}
strategy:
fail-fast: false
@@ -38,6 +39,12 @@ jobs:
- os: ubuntu-20.04
compiler: gcc
mini: mini
+ - os: ubuntu-20.04
+compiler: gcc
+checks: stdatomic
+ - os: ubuntu-20.04
+compiler: clang
+checks: stdatomic
- os: ubuntu-20.04
compiler: gcc
checks: debug+doc+examples+tests
@@ -241,6 +248,7 @@ jobs:
> ~/env
echo CC=ccache ${{ matrix.config.compiler }} >> ~/env
echo DEF_LIB=${{ matrix.config.library }} >> ~/env
+echo STDATOMIC=false >> ~/env
- name: Load the cached image
run: |
docker load -i ~/.image/${{ matrix.config.image }}.tar
diff --git a/devtools/test-meson-builds.sh b/devtools/test-meson-builds.sh
index 605a855999..5c07063cbd 100755
--- a/devtools/test-meson-builds.sh
+++ b/devtools/test-meson-builds.sh
@@ -227,11 +227,13 @@ for c in gcc clang ; do
for s in static shared ; do
if [ $s = shared ] ; then
abicheck=ABI
+ stdatomic=-Denable_stdatomic=true
else
abicheck=skipABI # save time and disk space
+ stdatomic=-Denable_stdatomic=false
fi
export CC="$CCACHE $c"
- build build-$c-$s $c $abicheck --default-library=$s
+ build build-$c-$s $c $abicheck $stdatomic --default-library=$s
unset CC
done
done
--
2.41.0
Re: [PATCH v2 1/2] power: refactor uncore power management interfaces
On Wed, Aug 16, 2023 at 12:10 PM Sivaprasad Tummala
wrote:
>
> currently the uncore power management implementation is vendor specific.
> Added new vendor agnostic uncore power interface similar to rte_power
> and rename specific implementations ("rte_power_intel_uncore") to
> "power_intel_uncore" along with functions.
>
> Signed-off-by: Sivaprasad Tummala
No news from the maintainers is not a good news...
But, given Anatoly was not against it, I took this series for rc1.
Applied with some fixes on doc and style, and added an entry in the RN, thanks.
--
David Marchand
[PATCH] ci: test manuals generation in GHA
Add missing package so manuals are generated as part of the docs check. Signed-off-by: David Marchand --- .github/workflows/build.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 7a2ac0ceee..e6c6267eb4 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -125,7 +125,7 @@ jobs: run: sudo apt install -y gdb jq - name: Install doc generation packages if: env.BUILD_DOCS == 'true' - run: sudo apt install -y doxygen graphviz python3-sphinx + run: sudo apt install -y doxygen graphviz man-db python3-sphinx python3-sphinx-rtd-theme - name: Run setup run: | -- 2.41.0
Re: [PATCH v2] ci: test stdatomic API
On Tue, Oct 17, 2023 at 9:15 AM David Marchand wrote: > > Add some compilation tests with C11 atomics enabled. > The headers check can't be enabled (as gcc and clang don't provide > stdatomic before C++23). > > Signed-off-by: David Marchand > Reviewed-by: Aaron Conole Applied, thanks. -- David Marchand
[PATCH] devtools: extend check on compiler builtin atomics
rte_memory_order_* should be used when calling the new stdatomic API.
Add a check on __ATOMIC_* tokens.
Signed-off-by: David Marchand
---
devtools/checkpatches.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh
index 2635923e14..7740152643 100755
--- a/devtools/checkpatches.sh
+++ b/devtools/checkpatches.sh
@@ -113,9 +113,9 @@ check_forbidden_additions() { #
# refrain from using compiler __atomic_xxx builtins
awk -v FOLDERS="lib drivers app examples" \
- -v EXPRESSIONS="__atomic_.*\\\(" \
+ -v EXPRESSIONS="__atomic_.*\\\(
__ATOMIC_(RELAXED|CONSUME|ACQUIRE|RELEASE|ACQ_REL|SEQ_CST)" \
-v RET_ON_FAIL=1 \
- -v MESSAGE='Using __atomic_xxx built-ins, prefer
rte_atomic_xxx' \
+ -v MESSAGE='Using __atomic_xxx/__ATOMIC_XXX built-ins, prefer
rte_atomic_xxx' \
-f $(dirname $(readlink -f $0))/check-forbidden-tokens.awk \
"$1" || res=1
--
2.41.0
[PATCH] event/cnxk: fix symbol map
Caught while rebasing a series that check map files.
Remove superfluous whitespace.
Current ABI number is 24.
Sort experimental symbols and annotate them with the version they are
introduced in.
Fixes: 03714a41bd26 ("event/cnxk: add event port flow context API")
Signed-off-by: David Marchand
---
drivers/event/cnxk/version.map | 12 +++-
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/event/cnxk/version.map b/drivers/event/cnxk/version.map
index 9dbf8eb59d..3dd9a8fdd1 100644
--- a/drivers/event/cnxk/version.map
+++ b/drivers/event/cnxk/version.map
@@ -1,9 +1,11 @@
- DPDK_23 {
+DPDK_24 {
local: *;
- };
+};
- EXPERIMENTAL {
+EXPERIMENTAL {
global:
- rte_pmd_cnxk_eventdev_wait_head;
+
+ # added in 23.11
rte_pmd_cnxk_eventdev_is_head;
- };
+ rte_pmd_cnxk_eventdev_wait_head;
+};
--
2.41.0
Re: [PATCH] event/dsw: fix missing device pointer
On Tue, Oct 17, 2023 at 6:04 PM Jerin Jacob wrote:
>
> On Tue, Oct 17, 2023 at 9:32 PM Bruce Richardson
> wrote:
> >
> > After calling rte_event_dev_info_get() the ".dev" field of the info
> > structure should have a pointer to the underlying device, allowing the
> > user to e.g. get the device name using using rte_dev_name(info.dev).
> >
> > The distributed software eventdev info structure did not return a
> > correct device pointer, though, instead returning NULL, which caused
> > crashes getting "rte_dev_name". Initializing the dev pointer inside the
> > "eventdev" struct in the device probe function fixes this by ensuring we
> > have a valid pointer to return in info_get calls.
> >
> > Fixes: 46a186b1f0c5 ("event/dsw: add device registration and build system")
> > Cc: mattias.ronnb...@ericsson.com
> >
> > Signed-off-by: Bruce Richardson
>
> Is this issue for all "vdev" devices? if so, Please check for
> drivers/event/skeleton too.
Should we add some eventdev wrappers for shared code like this?
Something like rte_eth_dev_pci_generic_probe() / rte_eth_vdev_allocate().
--
David Marchand
Re: [PATCH v4 0/7] document and simplify use of cmdline
'STRING'
> > t_name = t
> > @@ -113,7 +117,7 @@ def process_commands(infile, hfile, cfile, ctxname):
> > continue
> > if '#' not in line:
> > line = line + '#' # ensure split always works, even if
> > no help text
> > -tokens, comment = line.split('#', 1)
> > +tokens, comment = line.rsplit('#', 1)
> > instances.append(process_command(tokens.strip().split(),
> > cfile, comment.strip()))
> >
> > print(f'static __rte_used cmdline_parse_ctx_t {ctxname}[] = {{')
> >
> >
> > Which translates as:
> > cmd_brief:help # help: Show help
> > help section # help: Show help
> >
>
> +1
> I was actualy thinking that adding support for multi-choice fixed strings
> is something we should add. One thought that I had was that "#" is not a
> particularly good choice of separator here. While, as you show, it can be
> made to work; I think - since we are defining our own syntax here - that it
> would be both simpler for the script, and simpler for the user, to have "|"
> as the option separator. It should be familiar for everyone as an option
> separator from regexes, unlike "#" which is more familar for comments.
>
> So what about:
> help <|all|control|display|config|ports|>section
I don't like using | as it gives the false impression regexp are supported...
>
> By starting with the separator, we should avoid having to provide the
> STRING type at all.
... and as a consequence, I find <|all confusing, it is like an empty
value would be acceptable.
About skipping the token type for such lists, I had considered it, but
I thought other types took an optional list of allowed values...
Now looking at the cmdline types, it is not the case.
Maybe I mixed with some other cli framework I played with in the past...
All of this to say, ok for me to omit the type.
>
> To my previous point on not liking to have a prefix-specifier, the
> alternative to making testpmd work with the script is to tweak very
> slightly the "help ".
>
> help # show general help
> help on <|all|control|display|config|ports|>section
>
> By making the command "help on ports" rather than "help ports" we would
> avoid the need for the prefix syntax.
There are other cases where a "chain of command" returns the value of
a parameter.
And the same parameter may be set via "chain of command value".
--
David Marchand
[PATCH] bitops: mark new symbols as stable
Calling an experimental symbol from an inline helper triggers a warning
when such code is not compiled with experimental API.
This can be seen when rte_bitops.h gets (indirectly) included in OVS
builds.
On the other hand, rte_clz32, rte_clz64, rte_ctz32, rte_ctz64,
rte_popcount32, rte_popcount64 are inline helpers for abstracting common
bit counting functions. This part of the API is unlikely to change.
Mark those symbols as stable.
Fixes: 18898c4d06f9 ("eal: use abstracted bit count functions")
Signed-off-by: David Marchand
---
Copying Techboard for info, as this goes against the usual policy of
marking new API as experimental.
---
lib/eal/include/rte_bitops.h | 48
1 file changed, 48 deletions(-)
diff --git a/lib/eal/include/rte_bitops.h b/lib/eal/include/rte_bitops.h
index 6b8ae8d3ac..174d25216d 100644
--- a/lib/eal/include/rte_bitops.h
+++ b/lib/eal/include/rte_bitops.h
@@ -280,9 +280,6 @@ rte_bit_relaxed_test_and_clear64(unsigned int nr, volatile
uint64_t *addr)
#ifdef RTE_TOOLCHAIN_MSVC
/**
- * @warning
- * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
- *
* Get the count of leading 0-bits in v.
*
* @param v
@@ -290,7 +287,6 @@ rte_bit_relaxed_test_and_clear64(unsigned int nr, volatile
uint64_t *addr)
* @return
* The count of leading zero bits.
*/
-__rte_experimental
static inline unsigned int
rte_clz32(uint32_t v)
{
@@ -302,9 +298,6 @@ rte_clz32(uint32_t v)
}
/**
- * @warning
- * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
- *
* Get the count of leading 0-bits in v.
*
* @param v
@@ -312,7 +305,6 @@ rte_clz32(uint32_t v)
* @return
* The count of leading zero bits.
*/
-__rte_experimental
static inline unsigned int
rte_clz64(uint64_t v)
{
@@ -324,9 +316,6 @@ rte_clz64(uint64_t v)
}
/**
- * @warning
- * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
- *
* Get the count of trailing 0-bits in v.
*
* @param v
@@ -334,7 +323,6 @@ rte_clz64(uint64_t v)
* @return
* The count of trailing zero bits.
*/
-__rte_experimental
static inline unsigned int
rte_ctz32(uint32_t v)
{
@@ -346,9 +334,6 @@ rte_ctz32(uint32_t v)
}
/**
- * @warning
- * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
- *
* Get the count of trailing 0-bits in v.
*
* @param v
@@ -356,7 +341,6 @@ rte_ctz32(uint32_t v)
* @return
* The count of trailing zero bits.
*/
-__rte_experimental
static inline unsigned int
rte_ctz64(uint64_t v)
{
@@ -368,9 +352,6 @@ rte_ctz64(uint64_t v)
}
/**
- * @warning
- * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
- *
* Get the count of 1-bits in v.
*
* @param v
@@ -378,7 +359,6 @@ rte_ctz64(uint64_t v)
* @return
* The count of 1-bits.
*/
-__rte_experimental
static inline unsigned int
rte_popcount32(uint32_t v)
{
@@ -386,9 +366,6 @@ rte_popcount32(uint32_t v)
}
/**
- * @warning
- * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
- *
* Get the count of 1-bits in v.
*
* @param v
@@ -396,7 +373,6 @@ rte_popcount32(uint32_t v)
* @return
* The count of 1-bits.
*/
-__rte_experimental
static inline unsigned int
rte_popcount64(uint64_t v)
{
@@ -406,9 +382,6 @@ rte_popcount64(uint64_t v)
#else
/**
- * @warning
- * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
- *
* Get the count of leading 0-bits in v.
*
* @param v
@@ -416,7 +389,6 @@ rte_popcount64(uint64_t v)
* @return
* The count of leading zero bits.
*/
-__rte_experimental
static inline unsigned int
rte_clz32(uint32_t v)
{
@@ -424,9 +396,6 @@ rte_clz32(uint32_t v)
}
/**
- * @warning
- * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
- *
* Get the count of leading 0-bits in v.
*
* @param v
@@ -434,7 +403,6 @@ rte_clz32(uint32_t v)
* @return
* The count of leading zero bits.
*/
-__rte_experimental
static inline unsigned int
rte_clz64(uint64_t v)
{
@@ -442,9 +410,6 @@ rte_clz64(uint64_t v)
}
/**
- * @warning
- * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
- *
* Get the count of trailing 0-bits in v.
*
* @param v
@@ -452,7 +417,6 @@ rte_clz64(uint64_t v)
* @return
* The count of trailing zero bits.
*/
-__rte_experimental
static inline unsigned int
rte_ctz32(uint32_t v)
{
@@ -460,9 +424,6 @@ rte_ctz32(uint32_t v)
}
/**
- * @warning
- * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
- *
* Get the count of trailing 0-bits in v.
*
* @param v
@@ -470,7 +431,6 @@ rte_ctz32(uint32_t v)
* @return
* The count of trailing zero bits.
*/
-__rte_experimental
static inline unsigned int
rte_ctz64(uint64_t v)
{
@@ -478,9 +438,6 @@ rte_ctz64(uint64_t v)
}
/**
- * @warning
- * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
- *
[PATCH] ml/cnxk: don't export internal headers
driver_sdk_headers is used to expose headers that may be used by
external drivers.
Don't export ml/cnxk internal headers.
Fixes: fe83ffd9ec2e ("ml/cnxk: add skeleton")
Signed-off-by: David Marchand
---
drivers/ml/cnxk/meson.build | 7 ---
1 file changed, 7 deletions(-)
diff --git a/drivers/ml/cnxk/meson.build b/drivers/ml/cnxk/meson.build
index 94fa4283b1..5bf17d8ae3 100644
--- a/drivers/ml/cnxk/meson.build
+++ b/drivers/ml/cnxk/meson.build
@@ -7,13 +7,6 @@ if not is_linux or not dpdk_conf.get('RTE_ARCH_64')
subdir_done()
endif
-driver_sdk_headers = files(
-'cn10k_ml_dev.h',
-'cn10k_ml_ops.h',
-'cn10k_ml_model.h',
-'cn10k_ml_ocm.h',
-)
-
sources = files(
'cn10k_ml_dev.c',
'cn10k_ml_ops.c',
--
2.41.0
[PATCH] hash: fix build with GFNI
As an external header, rte_thash_x86_gfni.h should be self sufficient.
Fixes: 3d4e27fd7ff0 ("use abstracted bit count functions")
Signed-off-by: David Marchand
---
lib/hash/rte_thash_x86_gfni.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/lib/hash/rte_thash_x86_gfni.h b/lib/hash/rte_thash_x86_gfni.h
index fbec16dde0..b7c5a4ba7d 100644
--- a/lib/hash/rte_thash_x86_gfni.h
+++ b/lib/hash/rte_thash_x86_gfni.h
@@ -12,6 +12,7 @@
* using Galois Fields New Instructions.
*/
+#include
#include
#include
--
2.41.0
Re: [PATCH v4] bus/pci: fix legacy device IO port map in secondary process
On Mon, Oct 9, 2023 at 5:06 AM Ma, WenwuX wrote: > > From a pci bus API pov, nothing prevents a driver from mixing memory > > mapped with vfio and ioport resources (iow, calls to > > rte_pci_map_resource() and rte_pci_ioport_map()). > > IOW, it may not be the case with the net/virtio driver but, in theory, > > rte_pci_ioport_map()/pci_vfio_ioport_map() may be called after a > > rte_pci_map_resource() call. > > > > In a similar manner, from the API pov, > > rte_pci_ioport_map()/pci_vfio_ioport_map() may be called for multiple bars. > > > > In summary, nothing in this patch checks that vfio has been configured > > already > > and I think we need a refcount to handle those situations. > > > We call rte_vfio_setup_device just to get device info, we can call > rte_vfio_release_device as soon as pci_vfio_fill_regions is done. > This avoids reference counting operations, do you think it works? Afaics, rte_vfio_setup_device should not be called if a call to rte_pci_map_device for this device was successful (rte_pci_map_device itself calls rte_vfio_setup_device). And as a consequence, calling rte_vfio_release_device cannot be done unconditionnally neither. -- David Marchand
Re: [PATCH v1 1/2] baseband/acc: support ACC100 deRM corner case SDK
On Tue, Oct 10, 2023 at 7:55 PM Hernan Vargas wrote:
>
> Implement de-ratematch pre-processing for ACC100 SW corner cases.
> Some specific 5GUL FEC corner cases may cause unintended back pressure
> and in some cases a potential stability issue on the ACC100.
> The PMD can detect such code block configuration and issue an info
> message to the user.
>
> Signed-off-by: Hernan Vargas
> ---
> drivers/baseband/acc/meson.build | 23 ++-
> drivers/baseband/acc/rte_acc100_pmd.c | 59 +--
> 2 files changed, 77 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/baseband/acc/meson.build
> b/drivers/baseband/acc/meson.build
> index 27a654b50153..84f4fea635ef 100644
> --- a/drivers/baseband/acc/meson.build
> +++ b/drivers/baseband/acc/meson.build
> @@ -1,7 +1,28 @@
> # SPDX-License-Identifier: BSD-3-Clause
> # Copyright(c) 2020 Intel Corporation
>
> -deps += ['bus_pci']
...
> +deps += ['bbdev', 'bus_pci']
This part is likely a rebase damage.
See: b7b8de26f34d ("drivers: add dependencies for some classes")
--
David Marchand
