Re: Error in rte_eal_init() when multiple PODs over single node of K8 cluster
Hello Bruce, Thank you for your mail! I have attached the complete log that we observed. Upon your suggestion, we experimented with the --in-memory option and it worked! Since this option enables both --no-shconf and --huge-unlink. We also tried each option separately. The test ran fine with the --no-shconf option, but it failed when only the --huge-unlink option was added. Now when --no-shconf option also worked, we also need some understanding whether it will impact multi-queue performance and if there are any other costs associated with using this flag. Best Regards, Avijit Pandey Cloud SME | VoerEirAB +919598570190 From: Bruce Richardson Date: Wednesday, 27 March 2024 at 20:25 To: Avijit Pandey Cc: dev@dpdk.org Subject: Re: Error in rte_eal_init() when multiple PODs over single node of K8 cluster On Wed, Mar 27, 2024 at 12:42:55PM +, Avijit Pandey wrote: >Hello Devs, > > >I hope this email finds you well. > >I am reaching out to seek assistance regarding an issue I am facing in >DPDK within my Kubernetes cluster. > > >I have deployed a Kubernetes cluster v1.26.0, and I am currently >running network testing through DPPD-PRoX ([1]commit/02425932) using >DPDK (v22.11.0). I have deployed 3 pairs of PODs (3 server pods and 3 >client pods) on a single K8 node. The server generates and sends >traffic to the receiver pod. > > >During the automated testing, I encounter an error: "Error in >rte_eal_init()." This error occurs randomly, and I am unable to >determine the root cause. However, this issue does not occur when I use >a single pair of PODs (1 server pod and 1 client pod). The traffic is >sent and received through the sriov NICs. > > >With master core index 23, full core mask is 0x280 > >EAL command line: /opt/samplevnf/VNFs/DPPD-PROX/build/prox >-c0x280 --main-lcore=23 -n4 --allow :86:04.6 > >error Error in rte_eal_init() > > Not sure what the problem is exactly, without a better error message. Can you manage to provide the EAL output in the failure case, perhaps using --log-level flag to up the log levels a bit higher if the error is not clear from the default output. Also, in case of running multiple instances of DPDK on a single system, I'd generally recommend passing --in-memory flag to each instance to avoid issues with conflicts over hugepage files. (This will disable support for DPDK multi-process operation, so don't use the flag if that is a feature you are using.) /Bruce PS: couple of other comments on your commandline that may be of interest, since it's a little longer than it needs to be :-) - We'd generally recommend, for clarity, using "-l" flag rather than "-c" for passing core masks. In your case "-c 0x280" should be equivalent to the more comprehensible "-l 23,25". - DPDK always uses the lowest core number as the main lcore, so in the example above --main-lcore=23 should be superfluous and can be omitted - For mempool creation, -n 4 is the default in DPDK if unsupecified, so again that flag can be dropped without impact, unless something specific in the app depends on it in some other way. - If you want to shorten your allow list a little, the ":" can be dropped from the PCI address. So "--allow :86:04.6" can be "-a 86:04.6" dpdk.log Description: dpdk.log
RE: [PATCH 1/2] eal: provide macro for GCC builtin constant intrinsic
> From: Stephen Hemminger [mailto:step...@networkplumber.org]
> Sent: Monday, 1 April 2024 00.03
>
> On Wed, 20 Mar 2024 14:33:35 -0700
> Tyler Retzlaff wrote:
>
> > +#ifdef RTE_TOOLCHAIN_MSVC
> > +#define __rte_constant(e) 0
> > +#else
> > +#define __rte_constant(e) __extension__(__builtin_constant_p(e))
> > +#endif
> > +
>
>
> I did some looking around and some other project have macros
> for expressing constant expression vs constant.
>
> Implementing this with some form of sizeof math is possible.
> For example in linux/compiler.h
>
> /*
> * This returns a constant expression while determining if an argument
> is
> * a constant expression, most importantly without evaluating the
> argument.
> * Glory to Martin Uecker
> *
> * Details:
> * - sizeof() return an integer constant expression, and does not
> evaluate
> * the value of its operand; it only examines the type of its operand.
> * - The results of comparing two integer constant expressions is also
> * an integer constant expression.
> * - The first literal "8" isn't important. It could be any literal
> value.
> * - The second literal "8" is to avoid warnings about unaligned
> pointers;
> * this could otherwise just be "1".
> * - (long)(x) is used to avoid warnings about 64-bit types on 32-bit
> * architectures.
> * - The C Standard defines "null pointer constant", "(void *)0", as
> * distinct from other void pointers.
> * - If (x) is an integer constant expression, then the "* 0l" resolves
> * it into an integer constant expression of value 0. Since it is cast
> to
> * "void *", this makes the second operand a null pointer constant.
> * - If (x) is not an integer constant expression, then the second
> operand
> * resolves to a void pointer (but not a null pointer constant: the
> value
> * is not an integer constant 0).
> * - The conditional operator's third operand, "(int *)8", is an object
> * pointer (to type "int").
> * - The behavior (including the return type) of the conditional
> operator
> * ("operand1 ? operand2 : operand3") depends on the kind of
> expressions
> * given for the second and third operands. This is the central
> mechanism
> * of the macro:
> * - When one operand is a null pointer constant (i.e. when x is an
> integer
> * constant expression) and the other is an object pointer (i.e. our
> * third operand), the conditional operator returns the type of the
> * object pointer operand (i.e. "int *). Here, within the sizeof(),
> we
> * would then get:
> * sizeof(*((int *)(...)) == sizeof(int) == 4
> * - When one operand is a void pointer (i.e. when x is not an integer
> * constant expression) and the other is an object pointer (i.e. our
> * third operand), the conditional operator returns a "void *" type.
> * Here, within the sizeof(), we would then get:
> * sizeof(*((void *)(...)) == sizeof(void) == 1
> * - The equality comparison to "sizeof(int)" therefore depends on (x):
> * sizeof(int) == sizeof(int) (x) was a constant expression
> * sizeof(int) != sizeof(void)(x) was not a constant expression
> */
> #define __is_constexpr(x) \
> (sizeof(int) == sizeof(*(8 ? ((void *)((long)(x) * 0l)) : (int
> *)8)))
Nice!
If the author is willing to license it under the BSD license, we can copy it as
is.
We might want to add a couple of build time checks to verify that it does what
is expected; to catch any changes in compiler behavior.
Re: [PATCH v20 13/14] log: add support for systemd journal
On Sat, 30 Mar 2024 at 16:46, Stephen Hemminger
wrote:
>
> If DPDK application is being run as a systemd service, then
> it can use the journal protocol which allows putting more information
> in the log such as priority and other information.
>
> The use of journal protocol is automatically detected and
> handled. Rather than having a dependency on libsystemd,
> just use the protocol directly as defined in:
> https://systemd.io/JOURNAL_NATIVE_PROTOCOL/
>
> Signed-off-by: Stephen Hemminger
> ---
> doc/guides/prog_guide/log_lib.rst | 14 ++
> lib/eal/common/eal_common_options.c | 11 ++
> lib/eal/common/eal_options.h| 2 +
> lib/log/log.c | 5 +
> lib/log/log_internal.h | 3 +
> lib/log/log_journal.c | 200
> lib/log/log_private.h | 2 +
> lib/log/log_stubs.c | 11 +-
> lib/log/meson.build | 10 +-
> lib/log/version.map | 1 +
> 10 files changed, 255 insertions(+), 4 deletions(-)
> create mode 100644 lib/log/log_journal.c
This is very nice work, I like it a lot!
> +/*
> + * send structured message using journal protocol
> + * See: https://systemd.io/JOURNAL_NATIVE_PROTOCOL/
> + */
> +static int
> +journal_send(const char *buf, size_t len)
> +{
> + struct iovec iov[4];
> + unsigned int n = 0;
> + int priority = rte_log_cur_msg_loglevel() - 1;
> + char msg[] = "MESSAGE=";
> + char newline = '\n';
> + char pbuf[16]; /* "PRIORITY=N\n" */
> +
> + iov[n].iov_base = msg;
> + iov[n++].iov_len = strlen(msg);
> +
> + iov[n].iov_base = (char *)(uintptr_t)buf;
> + iov[n++].iov_len = len;
> +
> + /* if message doesn't end with newline, one will be applied. */
> + if (buf[len - 1] != '\n') {
> + iov[n].iov_base = &newline;
> + iov[n++].iov_len = 1;
> + }
> +
> + /* priority value between 0 ("emerg") and 7 ("debug") */
> + iov[n].iov_base = pbuf;
> + iov[n++].iov_len = snprintf(pbuf, sizeof(pbuf),
> + "PRIORITY=%d\n", priority);
> +
> + return writev(log_journal_fd, iov, n);
> +}
Doesn't need to be implemented immediately, but the nicest thing about
talking directly to the journal is the ability to send lots of
arbitrary and useful metadata together with a message, so would be
nice if the logging function took an optional string vector that is
appended, or some other such mechanism.
A very useful example is creating a UUID and then setting
MESSAGE_ID=uuid when logging particularly important messages that are
useful to filter by this type - for example, when an interface state
changes to up/down. That way, one can do 'journalctl MESSAGE_ID=abc..'
and get all messages about interface state changes. A project can also
ship a catalog file that adds additional context to each ID, that is
automatically parsed and displayed to users.
Re: [PATCH v1 0/2] deque: add multithread unsafe deque library
On Sun, 31 Mar 2024 20:37:27 -0500 Aditya Ambadipudi wrote: > As previously discussed in the mailing list [1] we are sending out this > patch that provides the implementation and unit test cases for the > RTE_DEQUE library. This includes functions for creating a RTE_DEQUE > object. Allocating memory to it. Deleting that object and free'ing the > memory associated with it. Enqueue/Dequeue functions. Functions for > zero-copy API. > > [1] https://mails.dpdk.org/archives/dev/2023-August/275003.html Does this build without errors with the Microsoft Visual C compiler? Want to make sure that all new code does not create more work for the Windows maintainers.
Re: The effect of inlining
On 2024-03-29 14:42, Morten Brørup wrote: +CC techboard From: Maxime Coquelin [mailto:maxime.coque...@redhat.com] Sent: Friday, 29 March 2024 14.05 Hi Stephen, On 3/29/24 03:53, Stephen Hemminger wrote: On Thu, 28 Mar 2024 17:10:42 -0700 Andrey Ignatov wrote: You don't need always inline, the compiler will do it anyway. I can remove it in v2, but it's not completely obvious to me how is it decided when to specify it explicitly and when not? I see plenty of __rte_always_inline in this file: % git grep -c '^static __rte_always_inline' lib/vhost/virtio_net.c lib/vhost/virtio_net.c:66 Cargo cult really. Cargo cult... really? Well, I just did a quick test by comparing IO forwarding with testpmd between main branch and with adding a patch that removes all the inline/noinline in lib/vhost/virtio_net.c [0]. main branch: 14.63Mpps main branch - inline/noinline: 10.24Mpps Thank you for testing this, Maxime. Very interesting! It is sometimes suggested on techboard meetings that we should convert more inline functions to non-inline for improved API/ABI stability, with the argument that the performance of inlining is negligible. I think you are mixing two different (but related) things here. 1) marking functions with the inline family of keywords/attributes 2) keeping function definitions in header files 1) does not affect the ABI, while 2) does. Neither 1) nor 2) affects the API (i.e., source-level compatibility). 2) *allows* for function inlining even in non-LTO builds, but doesn't force it. If you don't believe 2) makes a difference performance-wise, it follows that you also don't believe LTO makes much of a difference. Both have the same effect: allowing the compiler to reason over a larger chunk of your program. Allowing the compiler to inline small, often-called functions is crucial for performance, in my experience. If the target symbol tend to be in a shared object, the difference is even larger. It's also quite common that you see no effect of LTO (other than a reduction of code footprint). As LTO becomes more practical to use, 2) loses much of its appeal. If PGO ever becomes practical to use, maybe 1) will as well. I think this test proves that the sum of many small (negligible) performance differences it not negligible! Andrey, thanks for the patch, I'll have a look at it next week. Maxime [0]: https://pastebin.com/72P2npZ0
[PATCH 0/2] mempool: patches
Tried building mempool with MSVC. In addition to the __rte_constant macro Tyler had already done. This fixes a couple of warnings, and cleans up the code. Stephen Hemminger (2): mempool: replace GCC pragma with cast mempool: fix unused warning with MSVC lib/mempool/rte_mempool.c | 14 ++ 1 file changed, 2 insertions(+), 12 deletions(-) -- 2.43.0
[PATCH 1/2] mempool: replace GCC pragma with cast
Building mempool with MSVC generates a warning
because of this pragma (same with clang when debug is enabled).
The issue the pragma was working around can be better solved
by using an additional cast.
Fixes: af75078fece3 ("first public release")
Signed-off-by: Stephen Hemminger
---
lib/mempool/rte_mempool.c | 10 +-
1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/lib/mempool/rte_mempool.c b/lib/mempool/rte_mempool.c
index 12390a2c81..734e8a2feb 100644
--- a/lib/mempool/rte_mempool.c
+++ b/lib/mempool/rte_mempool.c
@@ -1056,10 +1056,6 @@ rte_mempool_dump_cache(FILE *f, const struct rte_mempool
*mp)
return count;
}
-#ifndef __INTEL_COMPILER
-#pragma GCC diagnostic ignored "-Wcast-qual"
-#endif
-
/* check and update cookies or panic (internal) */
void rte_mempool_check_cookies(const struct rte_mempool *mp,
void * const *obj_table_const, unsigned n, int free)
@@ -1074,7 +1070,7 @@ void rte_mempool_check_cookies(const struct rte_mempool
*mp,
/* Force to drop the "const" attribute. This is done only when
* DEBUG is enabled */
- tmp = (void *) obj_table_const;
+ tmp = (void *)(uintptr_t)obj_table_const;
obj_table = tmp;
while (n--) {
@@ -1183,10 +1179,6 @@ mempool_audit_cookies(struct rte_mempool *mp)
#define mempool_audit_cookies(mp) do {} while(0)
#endif
-#ifndef __INTEL_COMPILER
-#pragma GCC diagnostic error "-Wcast-qual"
-#endif
-
/* check cookies before and after objects */
static void
mempool_audit_cache(const struct rte_mempool *mp)
--
2.43.0
[PATCH 2/2] mempool: fix unused warning with MSVC
Applying __rte_unused to a variable has no effect with MS
windows compiler. The temporary variable used if debug
enabled can just be eliminated.
Signed-off-by: Stephen Hemminger
---
lib/mempool/rte_mempool.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/lib/mempool/rte_mempool.c b/lib/mempool/rte_mempool.c
index 734e8a2feb..2ac815a3bb 100644
--- a/lib/mempool/rte_mempool.c
+++ b/lib/mempool/rte_mempool.c
@@ -164,7 +164,6 @@ mempool_add_elem(struct rte_mempool *mp, __rte_unused void
*opaque,
void *obj, rte_iova_t iova)
{
struct rte_mempool_objhdr *hdr;
- struct rte_mempool_objtlr *tlr __rte_unused;
/* set mempool ptr in header */
hdr = RTE_PTR_SUB(obj, sizeof(*hdr));
@@ -175,8 +174,7 @@ mempool_add_elem(struct rte_mempool *mp, __rte_unused void
*opaque,
#ifdef RTE_LIBRTE_MEMPOOL_DEBUG
hdr->cookie = RTE_MEMPOOL_HEADER_COOKIE2;
- tlr = rte_mempool_get_trailer(obj);
- tlr->cookie = RTE_MEMPOOL_TRAILER_COOKIE;
+ rte_mempool_get_trailer(obj)->cookie = RTE_MEMPOOL_TRAILER_COOKIE;
#endif
}
--
2.43.0
Re: [DPDK/core Bug 1409] arparse library assumes enum are 64 bit
On Sat, Mar 30, 2024 at 02:58:41AM +, bugzi...@dpdk.org wrote: > https://bugs.dpdk.org/show_bug.cgi?id=1409 > > Bug ID: 1409 >Summary: arparse library assumes enum are 64 bit >Product: DPDK >Version: 24.03 > Hardware: All > OS: All > Status: UNCONFIRMED > Severity: normal > Priority: Normal > Component: core > Assignee: dev@dpdk.org > Reporter: step...@networkplumber.org > Target Milestone: --- > > MSVC correctly flags that this line in rte_argparse.h is incorrect: > RTE_ARGPARSE_ARG_RESERVED_FIELD = RTE_GENMASK64(63, 48), > > The problem is that enum values are just an alias for int, and it can be 32 > bits. > > Taken from the current C Standard (C99): > http://www.open std.org/JTC1/SC22/WG14/www/docs/n1256.pdf > > 6.7.2.2 Enumeration specifiers > [...] > Constraints > The expression that defines the value of an enumeration constant shall be an > integer constant expression that has a value representable as an int. > [...] > Each enumerated type shall be compatible with char, a signed integer type, or > an unsigned integer type. The choice of type is implementation-defined, but > shall be capable of representing the values of all the members of the > enumeration. > > Since rte_argparse only uses 10 bits now. The suggested fix here is to: >1. Assume 32 bits >2. Get rid of the reserved field - reserved fields are bad idea > as some additional information i was aware of this issue and had already discussing it internally with the visual studio engineers. we reviewed relevant parts of C11 standard we believe there are 2 points of interest. the C11 standard does appear to direct the implementation to select an integer wide enough to hold the 64-bit enum value but it is only reasonable to do so when the target has a native 64-bit type. i.e. if your target has no 64-bit integer than the size of the above constant expression will be truncated. the MSVC compiler requires an extra command line argument to provide standard C conformant behavior (/Zc:enumTypes). when used with a C++ TU MSVC does select a 64-bit type for the prescribed constant expression but does not correctly select a 64-bit type with a C TU (this matters if we are exposing this enum type in a public header consumed by C++) i'm in the process of requesting that /Zc:enumTypes be brought into alignment with how it functions with C++ TU. * /Zc:enumTypes should result in "the same" type being selected for C or C++ TU, whatever that type may be. * /Zc:enumTypes in a C TU should select a 64-bit integer type for the above example value when the target supports 64-bit integer natively. as the current released compiler obviously does not conform to the above we may apply a conditionally compiled workaround that declares a 64-bit integer with an identifier matching the name of the un-scoped enum value. ty > -- > You are receiving this mail because: > You are the assignee for the bug.
Re: [PATCH 1/2] mempool: replace GCC pragma with cast
On Mon, Apr 01, 2024 at 10:01:52AM -0700, Stephen Hemminger wrote:
> Building mempool with MSVC generates a warning
> because of this pragma (same with clang when debug is enabled).
> The issue the pragma was working around can be better solved
> by using an additional cast.
>
> Fixes: af75078fece3 ("first public release")
>
> Signed-off-by: Stephen Hemminger
> ---
Acked-by: Tyler Retzlaff
Re: [PATCH 2/2] mempool: fix unused warning with MSVC
On Mon, Apr 01, 2024 at 10:01:53AM -0700, Stephen Hemminger wrote: > Applying __rte_unused to a variable has no effect with MS > windows compiler. The temporary variable used if debug > enabled can just be eliminated. > > Signed-off-by: Stephen Hemminger > --- Acked-by: Tyler Retzlaff
Re: [DPDK/core Bug 1409] arparse library assumes enum are 64 bit
On Mon, 1 Apr 2024 10:20:14 -0700 Tyler Retzlaff wrote: > On Sat, Mar 30, 2024 at 02:58:41AM +, bugzi...@dpdk.org wrote: > > https://bugs.dpdk.org/show_bug.cgi?id=1409 > > > > Bug ID: 1409 > >Summary: arparse library assumes enum are 64 bit > >Product: DPDK > >Version: 24.03 > > Hardware: All > > OS: All > > Status: UNCONFIRMED > > Severity: normal > > Priority: Normal > > Component: core > > Assignee: dev@dpdk.org > > Reporter: step...@networkplumber.org > > Target Milestone: --- > > > > MSVC correctly flags that this line in rte_argparse.h is incorrect: > > RTE_ARGPARSE_ARG_RESERVED_FIELD = RTE_GENMASK64(63, 48), > > > > The problem is that enum values are just an alias for int, and it can be 32 > > bits. > > > > Taken from the current C Standard (C99): > > http://www.open std.org/JTC1/SC22/WG14/www/docs/n1256.pdf > > > > 6.7.2.2 Enumeration specifiers > > [...] > > Constraints > > The expression that defines the value of an enumeration constant shall be an > > integer constant expression that has a value representable as an int. > > [...] > > Each enumerated type shall be compatible with char, a signed integer type, > > or > > an unsigned integer type. The choice of type is implementation-defined, but > > shall be capable of representing the values of all the members of the > > enumeration. > > > > Since rte_argparse only uses 10 bits now. The suggested fix here is to: > >1. Assume 32 bits > >2. Get rid of the reserved field - reserved fields are bad idea > > > > as some additional information i was aware of this issue and had already > discussing it internally with the visual studio engineers. we reviewed > relevant parts of C11 standard we believe there are 2 points of > interest. > > the C11 standard does appear to direct the implementation to select an > integer wide enough to hold the 64-bit enum value but it is only > reasonable to do so when the target has a native 64-bit type. i.e. if > your target has no 64-bit integer than the size of the above constant > expression will be truncated. > > the MSVC compiler requires an extra command line argument to provide > standard C conformant behavior (/Zc:enumTypes). when used with a C++ TU > MSVC does select a 64-bit type for the prescribed constant expression > but does not correctly select a 64-bit type with a C TU (this matters if > we are exposing this enum type in a public header consumed by C++) > > i'm in the process of requesting that /Zc:enumTypes be brought into > alignment with how it functions with C++ TU. > > * /Zc:enumTypes should result in "the same" type being selected for > C or C++ TU, whatever that type may be. > > * /Zc:enumTypes in a C TU should select a 64-bit integer type for the > above example value when the target supports 64-bit integer natively. > > as the current released compiler obviously does not conform to the above > we may apply a conditionally compiled workaround that declares a 64-bit > integer with an identifier matching the name of the un-scoped enum > value. All well and good, but the assumption of 64 bit enum's breaks on 32 bit builds which DPDK still has. The library didn't need the bits. Just deleting the unused reserved field and changing the shifts to be 32 fixes the issue.
Re: [PATCH v2 1/6] ethdev: support setting lanes
30/03/2024 12:38, huangdengdui: > But, there are different solutions for the device to report the setting > lane capability, as following: > 1. Like the current patch, reporting device capabilities in speed and >lane coupling mode. However, if we use this solution, we will have >to couple the the lanes setting with speed setting. > > 2. Like the Damodharam's RFC patch [1], the device reports the maximum >number of supported lanes. Users can config a lane randomly, >which is completely separated from the speed. > > 3. Similar to the FEC capability reported by a device, the device reports the >relationship table of the number of lanes supported by the speed, >for example: > speedlanes_capa > 50G 1,2 > 100G 1,2,4 > 200G 2,4 > > Options 1 and 2 have been discussed a lot above. > > For solution 1, the speed and lanes are over-coupled, and the implementation > is too > complex. But I think it's easier to understand and easier for the device to > report > capabilities. In addition, the ethtool reporting capability also uses this > mode. > > For solution 2, as huisong said that user don't know what lanes should or can > be set > for a specified speed on one NIC. > > I think that when the device reports the capability, the lanes should be > associated > with the speed. In this way, users can know which lanes are supported by the > current > speed and verify the configuration validity. > > So I think solution 3 is better. What do you think? I don't understand your proposals. Please could you show the function signature for each option?
[PATCH v2] graph: avoid accessing graph list when getting stats
In rte_graph_cluster_stats_get, the walk model of the first graph is
checked to determine if multi-core dispatch specific counters should be
updated or not. This global list is accessed without any locks.
If the global list is modified by another thread while
rte_graph_cluster_stats_get is called, it can result in undefined
behaviour.
Adding a lock would make it impossible to call
rte_graph_cluster_stats_get in packet processing code paths. Avoid
accessing the global list instead by storing a bool field in the private
rte_graph_cluster_stats structure.
Also update the default callback to avoid accessing the global list and
use a different default callback depending on the graph model.
Signed-off-by: Robin Jarry
---
Notes:
v2:
* (kiran) removed unnecessary loop in stats_mem_init.
lib/graph/graph_stats.c | 57 ++---
1 file changed, 36 insertions(+), 21 deletions(-)
diff --git a/lib/graph/graph_stats.c b/lib/graph/graph_stats.c
index 2fb808b21ec5..d71451a17b95 100644
--- a/lib/graph/graph_stats.c
+++ b/lib/graph/graph_stats.c
@@ -34,6 +34,7 @@ struct __rte_cache_aligned rte_graph_cluster_stats {
uint32_t cluster_node_size; /* Size of struct cluster_node */
rte_node_t max_nodes;
int socket_id;
+ bool dispatch;
void *cookie;
size_t sz;
@@ -74,17 +75,16 @@ print_banner_dispatch(FILE *f)
}
static inline void
-print_banner(FILE *f)
+print_banner(FILE *f, bool dispatch)
{
- if
(rte_graph_worker_model_get(STAILQ_FIRST(graph_list_head_get())->graph) ==
- RTE_GRAPH_MODEL_MCORE_DISPATCH)
+ if (dispatch)
print_banner_dispatch(f);
else
print_banner_default(f);
}
static inline void
-print_node(FILE *f, const struct rte_graph_cluster_node_stats *stat)
+print_node(FILE *f, const struct rte_graph_cluster_node_stats *stat, bool
dispatch)
{
double objs_per_call, objs_per_sec, cycles_per_call, ts_per_hz;
const uint64_t prev_calls = stat->prev_calls;
@@ -104,8 +104,7 @@ print_node(FILE *f, const struct
rte_graph_cluster_node_stats *stat)
objs_per_sec = ts_per_hz ? (objs - prev_objs) / ts_per_hz : 0;
objs_per_sec /= 100;
- if
(rte_graph_worker_model_get(STAILQ_FIRST(graph_list_head_get())->graph) ==
- RTE_GRAPH_MODEL_MCORE_DISPATCH) {
+ if (dispatch) {
fprintf(f,
"|%-31s|%-15" PRIu64 "|%-15" PRIu64 "|%-15" PRIu64
"|%-15" PRIu64 "|%-15" PRIu64
@@ -123,20 +122,17 @@ print_node(FILE *f, const struct
rte_graph_cluster_node_stats *stat)
}
static int
-graph_cluster_stats_cb(bool is_first, bool is_last, void *cookie,
+graph_cluster_stats_cb(bool dispatch, bool is_first, bool is_last, void
*cookie,
const struct rte_graph_cluster_node_stats *stat)
{
FILE *f = cookie;
- int model;
-
- model =
rte_graph_worker_model_get(STAILQ_FIRST(graph_list_head_get())->graph);
if (unlikely(is_first))
- print_banner(f);
+ print_banner(f, dispatch);
if (stat->objs)
- print_node(f, stat);
+ print_node(f, stat, dispatch);
if (unlikely(is_last)) {
- if (model == RTE_GRAPH_MODEL_MCORE_DISPATCH)
+ if (dispatch)
boarder_model_dispatch();
else
boarder();
@@ -145,6 +141,20 @@ graph_cluster_stats_cb(bool is_first, bool is_last, void
*cookie,
return 0;
};
+static int
+graph_cluster_stats_cb_rtc(bool is_first, bool is_last, void *cookie,
+ const struct rte_graph_cluster_node_stats *stat)
+{
+ return graph_cluster_stats_cb(false, is_first, is_last, cookie, stat);
+};
+
+static int
+graph_cluster_stats_cb_dispatch(bool is_first, bool is_last, void *cookie,
+ const struct rte_graph_cluster_node_stats *stat)
+{
+ return graph_cluster_stats_cb(true, is_first, is_last, cookie, stat);
+};
+
static struct rte_graph_cluster_stats *
stats_mem_init(struct cluster *cluster,
const struct rte_graph_cluster_stats_param *prm)
@@ -157,8 +167,13 @@ stats_mem_init(struct cluster *cluster,
/* Fix up callback */
fn = prm->fn;
- if (fn == NULL)
- fn = graph_cluster_stats_cb;
+ if (fn == NULL) {
+ const struct rte_graph *graph = cluster->graphs[0]->graph;
+ if (graph->model == RTE_GRAPH_MODEL_MCORE_DISPATCH)
+ fn = graph_cluster_stats_cb_dispatch;
+ else
+ fn = graph_cluster_stats_cb_rtc;
+ }
cluster_node_size = sizeof(struct cluster_node);
/* For a given cluster, max nodes will be the max number of graphs */
@@ -350,6 +365,8 @@ rte_graph_cluster_stats_create(const struct
rte_graph_cluster_stats_param *prm)
Re: [RFC PATCH] usertools: add telemetry exporter
Anthony Harivel, Mar 27, 2024 at 16:18:
Hi Robin,
Thanks for this patch. I did test it and it works as expected.
Nonetheless, maybe we can improve on some parts.
Hey Anthony, thanks a lot for testing!
In 'class TelemetrySocket', there is:
...
self.sock.connect(path)
data = json.loads(self.sock.recv(1024).decode())
...
Maybe we can improve with something like:
try:
rcv_data = self.sock.recv(1024)
if rcv_data:
data = json.loads(rcv_data.decode())
else:
print("No data received from socket.")
except json.JSONDecodeError as e:
print("Error decoding JSON:", e)
except Exception as e:
print("An error occurred:", e)
So that it handles a bit better the error cases.
This is undesired as it would actually mask the errors from the calling
code. Unless you can do something about the error, printing it should be
left to the calling code. I will handle these errors better in v2.
In the same way to implement more robust error handling mechanisms in:
def load_endpoints
...
except Exception as e:
LOG.error("Failed to load endpoint module '%s' from '%s': %s", name, f, e)
...
For example, you might catch FileNotFoundError, ImportError, or SyntaxError.
That could help to debug!
We could print the whole stack trace but I don't see what special
handling could be done depending on the exception. I will try to make
this better for v2.
About TelemetryEndpoint I would see something like:
class TelemetryEndpoint:
"""
Placeholder class only used for typing annotations.
"""
@staticmethod
def info() -> typing.Dict[MetricName, MetricInfo]:
"""
Mapping of metric names to their description and type.
"""
raise NotImplementedError()
@staticmethod
def metrics(sock: TelemetrySocket) -> typing.List[MetricValue]:
"""
Request data from sock and return it as metric values. Each metric
name must be present in info().
"""
try:
metrics = []
metrics_data = sock.fetch_metrics_data()
for metric_name, metric_value in metrics_data.items():
metrics.append((metric_name, metric_value, {}))
return metrics
except Exception as e:
LOG.error("Failed to fetch metrics data: %s", e)
# If unable to fetch metrics data, return an empty list
return []
With these changes, the metrics method of the TelemetryEndpoint class
could handle errors better and the exporter can continue functioning
even if there are issues with fetching metrics data.
I don't know if all of that makes sens or if it's just nitpicking!
I can also propose an enhanced version of your patch if you prefer.
As indicated in the docstring, this class is merely a placeholder. Its
code is never executed. It only stands to represent what functions must
be implemented in endpoints.
However, your point is valid. I will update my code to handle errors in
endpoints more gracefully and avoid failing the whole request if there
is only one error.
Thanks for the thorough review!
Re: pcapng_autotest unit test false positive
On Fri, Mar 22, 2024 at 7:34 PM Stephen Hemminger wrote: > > > Could you build a simple test to see if TSC every runs backwards on > this machine. Or there could be yet another math error. > Or maybe container TSC is huge an wrapping around? > > The point of the test is to make sure that there wasn't wraparound errors. Sorry about the wait on this one, but we did write that simple C program to check for whether TSC ever runs backwards on this system. It gets TSC using __rdtsc() because that's the same approach from the x86 rte_cycles.c. And it just loops for 10 seconds or so and compares n TSC to n-1 TSC, and if n's TSC is ever less than n-1's TSC it prints a message saying so. Otherwise at the end it prints that TSC is working normally. From running this the first time, it showed TSC as never running backwards. Another thing I can do is trigger a full set of testing (so that the system is under normal load) and then run the tsc checking program concurrently. Another idea - maybe multiple timestamps are gathered from different CPU registers during the same test, and they are misaligned for that reason. Maybe we can try reducing the cores for each unit test to 1 and checking whether the issue persists. Or there could be another math error as you say. And I should mention that now that I'm looking at this more closely I did see that unfortunately all these fail results are coming from a new debian 12 x86 environment which was added a few weeks ago, but mistakenly labeled as debian 11 x86. So, the fact that fails started can be explained by the fact that we added this new debian 12 container recently. So, I'll try a few more things and keep yall updated.
Re: [PATCH v1 0/2] deque: add multithread unsafe deque library
Thanks, Stephen, for the comment. Unfortunately, we don't have the dev setup nor the resources to test out this change using MSVC. Thank you, Aditya Ambadipudi From: Stephen Hemminger Sent: Monday, April 1, 2024 9:05 AM To: Aditya Ambadipudi Cc: dev@dpdk.org ; jack...@nvidia.com ; ma...@nvidia.com ; viachesl...@nvidia.com ; roret...@linux.microsoft.com ; konstantin.v.anan...@yandex.ru ; konstantin.anan...@huawei.com ; m...@smartsharesystems.com ; hof...@lysator.liu.se ; Honnappa Nagarahalli ; Dhruv Tripathi ; Wathsala Wathawana Vithanage ; ganeshadit...@gmail.com ; nd Subject: Re: [PATCH v1 0/2] deque: add multithread unsafe deque library On Sun, 31 Mar 2024 20:37:27 -0500 Aditya Ambadipudi wrote: > As previously discussed in the mailing list [1] we are sending out this > patch that provides the implementation and unit test cases for the > RTE_DEQUE library. This includes functions for creating a RTE_DEQUE > object. Allocating memory to it. Deleting that object and free'ing the > memory associated with it. Enqueue/Dequeue functions. Functions for > zero-copy API. > > [1] https://mails.dpdk.org/archives/dev/2023-August/275003.html Does this build without errors with the Microsoft Visual C compiler? Want to make sure that all new code does not create more work for the Windows maintainers.
Re: [PATCH v2 1/6] ethdev: support setting lanes
On Mon, Apr 1, 2024 at 1:07 PM Thomas Monjalon wrote: > > 30/03/2024 12:38, huangdengdui: > > But, there are different solutions for the device to report the setting > > lane capability, as following: > > 1. Like the current patch, reporting device capabilities in speed and > >lane coupling mode. However, if we use this solution, we will have > >to couple the the lanes setting with speed setting. > > > > 2. Like the Damodharam's RFC patch [1], the device reports the maximum > >number of supported lanes. Users can config a lane randomly, > >which is completely separated from the speed. > > > > 3. Similar to the FEC capability reported by a device, the device reports > > the > >relationship table of the number of lanes supported by the speed, > >for example: > > speedlanes_capa > > 50G 1,2 > > 100G 1,2,4 > > 200G 2,4 > > > > Options 1 and 2 have been discussed a lot above. > > > > For solution 1, the speed and lanes are over-coupled, and the > > implementation is too > > complex. But I think it's easier to understand and easier for the device to > > report > > capabilities. In addition, the ethtool reporting capability also uses this > > mode. > > > > For solution 2, as huisong said that user don't know what lanes should or > > can be set > > for a specified speed on one NIC. > > > > I think that when the device reports the capability, the lanes should be > > associated > > with the speed. In this way, users can know which lanes are supported by > > the current > > speed and verify the configuration validity. > > > > So I think solution 3 is better. What do you think? > > I don't understand your proposals. > Please could you show the function signature for each option? > > > testpmd can query the driver, and driver can export latest bit-map say in, rte_eth_speed_lanes_get()->supported_bmap 0 1Gb link speed 1 10Gb (NRZ: 10G per lane, 1 lane) link speed 2 25Gb (NRZ: 25G per lane, 1 lane) link speed 3 40Gb (NRZ: 10G per lane, 4 lanes) link speed 4 50Gb (NRZ: 25G per lane, 2 lanes) link speed 5 100Gb (NRZ: 25G per lane, 4 lanes) link speed 6 50Gb (PAM4-56: 50G per lane, 1 lane) link speed 7 100Gb (PAM4-56: 50G per lane, 2 lanes) link speed 8 200Gb (PAM4-56: 50G per lane, 4 lanes) link speed 9 400Gb (PAM4-56: 50G per lane, 8 lanes) link speed 10 100Gb (PAM4-112: 100G per lane, 1 lane) link speed 11 200Gb (PAM4-112: 100G per lane, 2 lanes) link speed 12 400Gb (PAM4-112: 100G per lane, 4 lanes) link speed 13 800Gb (PAM4-112: 100G per lane, 8 lanes) link speed 14 For future In cmd_config_speed_specific_parsed() if (parse_and_check_speed_duplex(res->value1, res->value2, &link_speed) < 0) return; + /* validate speed x lanes combo */ + if (!cmd_validate_lanes(res->id, link_speed)) + return; Driver can validate the rest of other internal link parameters in rte_eth_dev_start() before applying the config to the hardware. -- This electronic communication and the information and any files transmitted with it, or attached to it, are confidential and are intended solely for the use of the individual or entity to whom it is addressed and may contain information that is confidential, legally privileged, protected by privacy laws, or otherwise restricted from disclosure to anyone else. If you are not the intended recipient or the person responsible for delivering the e-mail to the intended recipient, you are hereby notified that any use, copying, distributing, dissemination, forwarding, printing, or copying of this e-mail is strictly prohibited. If you received this e-mail in error, please return the e-mail to the sender, delete it from your computer, and destroy any printed copy of it. smime.p7s Description: S/MIME Cryptographic Signature
Re: [DPDK/core Bug 1409] arparse library assumes enum are 64 bit
On Mon, Apr 01, 2024 at 12:34:08PM -0700, Stephen Hemminger wrote: > On Mon, 1 Apr 2024 10:20:14 -0700 > Tyler Retzlaff wrote: > > > On Sat, Mar 30, 2024 at 02:58:41AM +, bugzi...@dpdk.org wrote: > > > https://bugs.dpdk.org/show_bug.cgi?id=1409 > > > > > > Bug ID: 1409 > > >Summary: arparse library assumes enum are 64 bit > > >Product: DPDK > > >Version: 24.03 > > > Hardware: All > > > OS: All > > > Status: UNCONFIRMED > > > Severity: normal > > > Priority: Normal > > > Component: core > > > Assignee: dev@dpdk.org > > > Reporter: step...@networkplumber.org > > > Target Milestone: --- > > > > > > MSVC correctly flags that this line in rte_argparse.h is incorrect: > > > RTE_ARGPARSE_ARG_RESERVED_FIELD = RTE_GENMASK64(63, 48), > > > > > > The problem is that enum values are just an alias for int, and it can be > > > 32 > > > bits. > > > > > > Taken from the current C Standard (C99): > > > http://www.open std.org/JTC1/SC22/WG14/www/docs/n1256.pdf > > > > > > 6.7.2.2 Enumeration specifiers > > > [...] > > > Constraints > > > The expression that defines the value of an enumeration constant shall be > > > an > > > integer constant expression that has a value representable as an int. > > > [...] > > > Each enumerated type shall be compatible with char, a signed integer > > > type, or > > > an unsigned integer type. The choice of type is implementation-defined, > > > but > > > shall be capable of representing the values of all the members of the > > > enumeration. > > > > > > Since rte_argparse only uses 10 bits now. The suggested fix here is to: > > >1. Assume 32 bits > > >2. Get rid of the reserved field - reserved fields are bad idea > > > > > > > as some additional information i was aware of this issue and had already > > discussing it internally with the visual studio engineers. we reviewed > > relevant parts of C11 standard we believe there are 2 points of > > interest. > > > > the C11 standard does appear to direct the implementation to select an > > integer wide enough to hold the 64-bit enum value but it is only > > reasonable to do so when the target has a native 64-bit type. i.e. if > > your target has no 64-bit integer than the size of the above constant > > expression will be truncated. > > > > the MSVC compiler requires an extra command line argument to provide > > standard C conformant behavior (/Zc:enumTypes). when used with a C++ TU > > MSVC does select a 64-bit type for the prescribed constant expression > > but does not correctly select a 64-bit type with a C TU (this matters if > > we are exposing this enum type in a public header consumed by C++) > > > > i'm in the process of requesting that /Zc:enumTypes be brought into > > alignment with how it functions with C++ TU. > > > > * /Zc:enumTypes should result in "the same" type being selected for > > C or C++ TU, whatever that type may be. > > > > * /Zc:enumTypes in a C TU should select a 64-bit integer type for the > > above example value when the target supports 64-bit integer natively. > > > > as the current released compiler obviously does not conform to the above > > we may apply a conditionally compiled workaround that declares a 64-bit > > integer with an identifier matching the name of the un-scoped enum > > value. > > All well and good, but the assumption of 64 bit enum's breaks on > 32 bit builds which DPDK still has. The library didn't need the bits. > Just deleting the unused reserved field and changing the shifts to > be 32 fixes the issue. agreed. thanks for raising it.
Re: [PATCH v1 0/2] deque: add multithread unsafe deque library
On Mon, Apr 01, 2024 at 10:28:52PM +, Aditya Ambadipudi wrote: > Thanks, Stephen, for the comment. > > Unfortunately, we don't have the dev setup nor the resources to test out this > change using MSVC. what are the dependencies of this lib? you've provided an agnostic api and unit tests, you can enable it in the build and the CI will provide a minimum test bar. > > Thank you, > Aditya Ambadipudi > > > > From: Stephen Hemminger > Sent: Monday, April 1, 2024 9:05 AM > To: Aditya Ambadipudi > Cc: dev@dpdk.org ; jack...@nvidia.com ; > ma...@nvidia.com ; viachesl...@nvidia.com > ; roret...@linux.microsoft.com > ; konstantin.v.anan...@yandex.ru > ; konstantin.anan...@huawei.com > ; m...@smartsharesystems.com > ; hof...@lysator.liu.se ; > Honnappa Nagarahalli ; Dhruv Tripathi > ; Wathsala Wathawana Vithanage > ; ganeshadit...@gmail.com > ; nd > Subject: Re: [PATCH v1 0/2] deque: add multithread unsafe deque library > > On Sun, 31 Mar 2024 20:37:27 -0500 > Aditya Ambadipudi wrote: > > > As previously discussed in the mailing list [1] we are sending out this > > patch that provides the implementation and unit test cases for the > > RTE_DEQUE library. This includes functions for creating a RTE_DEQUE > > object. Allocating memory to it. Deleting that object and free'ing the > > memory associated with it. Enqueue/Dequeue functions. Functions for > > zero-copy API. > > > > [1] https://mails.dpdk.org/archives/dev/2023-August/275003.html > > Does this build without errors with the Microsoft Visual C compiler? > > Want to make sure that all new code does not create more work for the > Windows maintainers.
Re: pcapng_autotest unit test false positive
On Mon, 1 Apr 2024 18:26:44 -0400 Patrick Robb wrote: > Another idea - maybe multiple timestamps are gathered from different > CPU registers during the same test, and they are misaligned for that > reason. Maybe we can try reducing the cores for each unit test to 1 > and checking whether the issue persists. TSC is expected to be sync'd between cores. But of course packets can arrive out of order on different cores.
Re: [PATCH v1 0/2] deque: add multithread unsafe deque library
On Mon, 1 Apr 2024 22:28:52 + Aditya Ambadipudi wrote: > Thanks, Stephen, for the comment. > > Unfortunately, we don't have the dev setup nor the resources to test out this > change using MSVC. > > Thank you, > Aditya Ambadipudi All it requires is the community version of MSVC which is free. And setting up a Windows VM with KVM is free and easy. IMHO all new libraries have to build on all environments, unless they are enabling platform specific features.
Re: [PATCH v1 0/2] deque: add multithread unsafe deque library
> On Apr 1, 2024, at 7:47 PM, Stephen Hemminger > wrote: > > On Mon, 1 Apr 2024 22:28:52 + > Aditya Ambadipudi wrote: > >> Thanks, Stephen, for the comment. >> >> Unfortunately, we don't have the dev setup nor the resources to test out >> this change using MSVC. >> >> Thank you, >> Aditya Ambadipudi > > All it requires is the community version of MSVC which is free. And setting > up a Windows > VM with KVM is free and easy. > > IMHO all new libraries have to build on all environments, unless they are > enabling > platform specific features. I see that UNH CI is running Windows VMs, the tests are passing there. So, we do not need to anything.
Re: [PATCH v1 0/2] deque: add multithread unsafe deque library
On Tue, 2 Apr 2024 01:35:28 + Honnappa Nagarahalli wrote: > > On Apr 1, 2024, at 7:47 PM, Stephen Hemminger > > wrote: > > > > On Mon, 1 Apr 2024 22:28:52 + > > Aditya Ambadipudi wrote: > > > >> Thanks, Stephen, for the comment. > >> > >> Unfortunately, we don't have the dev setup nor the resources to test out > >> this change using MSVC. > >> > >> Thank you, > >> Aditya Ambadipudi > > > > All it requires is the community version of MSVC which is free. And setting > > up a Windows > > VM with KVM is free and easy. > > > > IMHO all new libraries have to build on all environments, unless they are > > enabling > > platform specific features. > I see that UNH CI is running Windows VMs, the tests are passing there. So, we > do not need to anything. > That only tests the clang part. You need to modify lib/meson.build to get it tested with the windows compiler.
Re: [PATCH v1 0/2] deque: add multithread unsafe deque library
> On Apr 1, 2024, at 9:00 PM, Stephen Hemminger > wrote: > > On Tue, 2 Apr 2024 01:35:28 + > Honnappa Nagarahalli wrote: > >>> On Apr 1, 2024, at 7:47 PM, Stephen Hemminger >>> wrote: >>> >>> On Mon, 1 Apr 2024 22:28:52 + >>> Aditya Ambadipudi wrote: >>> Thanks, Stephen, for the comment. Unfortunately, we don't have the dev setup nor the resources to test out this change using MSVC. Thank you, Aditya Ambadipudi >>> >>> All it requires is the community version of MSVC which is free. And setting >>> up a Windows >>> VM with KVM is free and easy. >>> >>> IMHO all new libraries have to build on all environments, unless they are >>> enabling >>> platform specific features. >> I see that UNH CI is running Windows VMs, the tests are passing there. So, >> we do not need to anything. >> > > That only tests the clang part. > You need to modify lib/meson.build to get it tested with the windows compiler. Any idea on when is this getting added to CI?
[DPDK/ethdev Bug 942] i40e: condition in `i40e_flow_parse_fdir_pattern()` is always false
https://bugs.dpdk.org/show_bug.cgi?id=942 dengkaiwen (kaiwenx.d...@intel.com) changed: What|Removed |Added Status|UNCONFIRMED |RESOLVED Resolution|--- |FIXED CC||kaiwenx.d...@intel.com --- Comment #2 from dengkaiwen (kaiwenx.d...@intel.com) --- Close this ticket. -- You are receiving this mail because: You are the assignee for the bug.
[DPDK/ethdev Bug 1275] About i40e statistics: When all packets overruns, rx_packets is 0, but rx_bytes still increasing
https://bugs.dpdk.org/show_bug.cgi?id=1275 dengkaiwen (kaiwenx.d...@intel.com) changed: What|Removed |Added Resolution|--- |INVALID Status|UNCONFIRMED |RESOLVED --- Comment #2 from dengkaiwen (kaiwenx.d...@intel.com) --- Close this ticket. -- You are receiving this mail because: You are the assignee for the bug.
Re: [PATCH v1 0/2] deque: add multithread unsafe deque library
On Tue, 2 Apr 2024 02:14:06 + Honnappa Nagarahalli wrote: > > On Apr 1, 2024, at 9:00 PM, Stephen Hemminger > > wrote: > > > > On Tue, 2 Apr 2024 01:35:28 + > > Honnappa Nagarahalli wrote: > > > >>> On Apr 1, 2024, at 7:47 PM, Stephen Hemminger > >>> wrote: > >>> > >>> On Mon, 1 Apr 2024 22:28:52 + > >>> Aditya Ambadipudi wrote: > >>> > Thanks, Stephen, for the comment. > > Unfortunately, we don't have the dev setup nor the resources to test out > this change using MSVC. > > Thank you, > Aditya Ambadipudi > >>> > >>> All it requires is the community version of MSVC which is free. And > >>> setting up a Windows > >>> VM with KVM is free and easy. > >>> > >>> IMHO all new libraries have to build on all environments, unless they are > >>> enabling > >>> platform specific features. > >> I see that UNH CI is running Windows VMs, the tests are passing there. So, > >> we do not need to anything. > >> > > > > That only tests the clang part. > > You need to modify lib/meson.build to get it tested with the windows > > compiler. > Any idea on when is this getting added to CI? You need to add this to next version of the patch. I tried it and MSVC has no problems with the new code. Another issue is the naming. Right now the choice of 'deque' generates lots of checkpatch errors, and every bit of new code that uses the library will get a warning as well. Can you think of a better name? diff --git a/lib/meson.build b/lib/meson.build index 8c8c1e98e2..127e4dc68c 100644 --- a/lib/meson.build +++ b/lib/meson.build @@ -75,6 +75,7 @@ if is_ms_compiler 'kvargs', 'telemetry', 'eal', +'deque', 'ring', ] endif
Re: [PATCH v1 0/2] deque: add multithread unsafe deque library
On Tue, Apr 02, 2024 at 03:03:13AM +, Aditya Ambadipudi wrote: > Hello Stephen, > > I have a copy of CLRS with me. And Deque is a very standard word in computer > science. Even CLRS which is considered one of the most foundational books in > computer science uses the word deque. i'm kind of inclined to agree with this. double ended queue is pretty well known as ``deque`` perhaps most notably from stl. https://en.cppreference.com/w/cpp/container/deque i would however strongly advise that there be no use of ``deque`` bare (without the rte_ prefix) in any public header. (i.e. inline function variables , parameter names, etc...). that would almost certainly result in frustration for C++ consumers of dpdk that may be doing the following: #include #include using namespace std; a quick pass of the patches and i don't see any instances without the rte_ prefix so only cautioning that we would want to avoid it. > > I don't think there is any better word to describe the datastructure we are > building other than "deque". > > Is there a way to add an exception for that word in the dictionary words > check we run? I genuinely think the readability of this library that we are > building will suffer if we don't use the word "deque" here. > > Thank you, > Aditya Ambadipudi > > [image/jpeg][image/jpeg] > > > From: Stephen Hemminger > Sent: Monday, April 1, 2024 9:53 PM > To: Honnappa Nagarahalli > Cc: Aditya Ambadipudi ; dev@dpdk.org > ; jack...@nvidia.com ; ma...@nvidia.com > ; viachesl...@nvidia.com ; > roret...@linux.microsoft.com ; > konstantin.v.anan...@yandex.ru ; > konstantin.anan...@huawei.com ; > m...@smartsharesystems.com ; > hof...@lysator.liu.se ; Dhruv Tripathi > ; Wathsala Wathawana Vithanage > ; nd > Subject: Re: [PATCH v1 0/2] deque: add multithread unsafe deque library > > On Tue, 2 Apr 2024 02:14:06 + > Honnappa Nagarahalli wrote: > > > > On Apr 1, 2024, at 9:00 PM, Stephen Hemminger > > > wrote: > > > > > > On Tue, 2 Apr 2024 01:35:28 + > > > Honnappa Nagarahalli wrote: > > > > > >>> On Apr 1, 2024, at 7:47 PM, Stephen Hemminger > > >>> wrote: > > >>> > > >>> On Mon, 1 Apr 2024 22:28:52 + > > >>> Aditya Ambadipudi wrote: > > >>> > > Thanks, Stephen, for the comment. > > > > Unfortunately, we don't have the dev setup nor the resources to test > > out this change using MSVC. > > > > Thank you, > > Aditya Ambadipudi > > >>> > > >>> All it requires is the community version of MSVC which is free. And > > >>> setting up a Windows > > >>> VM with KVM is free and easy. > > >>> > > >>> IMHO all new libraries have to build on all environments, unless they > > >>> are enabling > > >>> platform specific features. > > >> I see that UNH CI is running Windows VMs, the tests are passing there. > > >> So, we do not need to anything. > > >> > > > > > > That only tests the clang part. > > > You need to modify lib/meson.build to get it tested with the windows > > > compiler. > > Any idea on when is this getting added to CI? > > You need to add this to next version of the patch. > I tried it and MSVC has no problems with the new code. > > Another issue is the naming. Right now the choice of 'deque' generates lots of > checkpatch errors, and every bit of new code that uses the library will get a > warning > as well. Can you think of a better name? > > diff --git a/lib/meson.build b/lib/meson.build > index 8c8c1e98e2..127e4dc68c 100644 > --- a/lib/meson.build > +++ b/lib/meson.build > @@ -75,6 +75,7 @@ if is_ms_compiler > 'kvargs', > 'telemetry', > 'eal', > +'deque', > 'ring', > ] > endif
Re: [PATCH v1 0/2] deque: add multithread unsafe deque library
On Mon, Apr 01, 2024 at 07:53:48PM -0700, Stephen Hemminger wrote: > On Tue, 2 Apr 2024 02:14:06 + > Honnappa Nagarahalli wrote: > > > > On Apr 1, 2024, at 9:00 PM, Stephen Hemminger > > > wrote: > > > > > > On Tue, 2 Apr 2024 01:35:28 + > > > Honnappa Nagarahalli wrote: > > > > > >>> On Apr 1, 2024, at 7:47 PM, Stephen Hemminger > > >>> wrote: > > >>> > > >>> On Mon, 1 Apr 2024 22:28:52 + > > >>> Aditya Ambadipudi wrote: > > >>> > > Thanks, Stephen, for the comment. > > > > Unfortunately, we don't have the dev setup nor the resources to test > > out this change using MSVC. > > > > Thank you, > > Aditya Ambadipudi > > >>> > > >>> All it requires is the community version of MSVC which is free. And > > >>> setting up a Windows > > >>> VM with KVM is free and easy. > > >>> > > >>> IMHO all new libraries have to build on all environments, unless they > > >>> are enabling > > >>> platform specific features. > > >> I see that UNH CI is running Windows VMs, the tests are passing there. > > >> So, we do not need to anything. > > >> > > > > > > That only tests the clang part. > > > You need to modify lib/meson.build to get it tested with the windows > > > compiler. > > Any idea on when is this getting added to CI? > > You need to add this to next version of the patch. > I tried it and MSVC has no problems with the new code. > > Another issue is the naming. Right now the choice of 'deque' generates lots of > checkpatch errors, and every bit of new code that uses the library will get a > warning > as well. Can you think of a better name? > > diff --git a/lib/meson.build b/lib/meson.build > index 8c8c1e98e2..127e4dc68c 100644 > --- a/lib/meson.build > +++ b/lib/meson.build > @@ -75,6 +75,7 @@ if is_ms_compiler > 'kvargs', > 'telemetry', > 'eal', > +'deque', > 'ring', > ] > endif please do this. thanks
Re: [PATCH v1 0/2] deque: add multithread unsafe deque library
On 2024-04-02 02:47, Stephen Hemminger wrote: On Mon, 1 Apr 2024 22:28:52 + Aditya Ambadipudi wrote: Thanks, Stephen, for the comment. Unfortunately, we don't have the dev setup nor the resources to test out this change using MSVC. Thank you, Aditya Ambadipudi All it requires is the community version of MSVC which is free. And setting up a Windows VM with KVM is free and easy. IMHO all new libraries have to build on all environments, unless they are enabling platform specific features. Requiring all contributors to build and test on what is, in this context, a pretty obscure platform with a pretty obscure compiler seems like a bad idea to me. It will raise the bar for contributions further. In principle I agree though. Your contribution should not only build, but also run (and be tested) on all platforms. Otherwise, Windows isn't supported in the upstream, but rather we have a Windows port (which happens to live in the same source tree). I never tested any contribution on a FreeBSD system, but at least those use the de-facto standard compilers and a standard API (POSIX), so the likelihood of things actually working is greater (but maybe not great enough). Surely, this is something the tech board must have discussed when it agreed to supporting Windows *and* MSVC. Many if not most of the man-hours involved won't be spent by the Windows maintainer, but the individual future contributors.
