Re: virtio: RSS support capa
On 9/7/24 23:55, Morten Brørup wrote: From: Morten Brørup [mailto:m...@smartsharesystems.com] Sent: Friday, 6 September 2024 21.38 Maxime, Chenbo, If the virtio PMD supports RSS, it should be announced in its capabilities. I think this should be added to virtio_dev_info_get(): if (host_features & (1ULL << VIRTIO_NET_F_RSS)) dev_info->rx_offload_capa |= RTE_ETH_RX_OFFLOAD_RSS_HASH; Or perhaps I'm misunderstanding this capability flag. I thought it indicated RSS ability, i.e. multi-queue, effectively shadowing rte_eth_conf.rxmode.mq_mode RTE_ETH_MQ_RX_RSS_FLAG. But maybe it doesn't. Maybe it indicates the ability to store the RSS hash value in the mbuf. The RTE_ETH_RX_OFFLOAD_RSS_HASH flag is completely undocumented. Can someone please clarify? RTE_ETH_RX_OFFLOAD_RSS_HASH means that the driver can provide RSS hash value in mbuf (it makes sense if HW can provide it to the driver).
Re: [PATCH 04/11] net/sfc: fix driver logtype token
On 9/7/24 17:54, David Marchand wrote:
The net/sfc driver logtype was registered under pmd.net.sfc."driver".
Fixes: 0f39f32482a1 ("net/sfc: remove use of EAL logtype")
Signed-off-by: David Marchand
---
drivers/net/sfc/sfc_ethdev.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c
index 24686a1eaf..3480a51642 100644
--- a/drivers/net/sfc/sfc_ethdev.c
+++ b/drivers/net/sfc/sfc_ethdev.c
@@ -3616,4 +3616,4 @@ RTE_PMD_REGISTER_PARAM_STRING(net_sfc_efx,
SFC_KVARG_FW_VARIANT "=" SFC_KVARG_VALUES_FW_VARIANT " "
SFC_KVARG_RXD_WAIT_TIMEOUT_NS "= "
SFC_KVARG_STATS_UPDATE_PERIOD_MS "=");
-RTE_LOG_REGISTER_SUFFIX(sfc_logtype_driver, "driver", NOTICE);
+RTE_LOG_REGISTER_SUFFIX(sfc_logtype_driver, driver, NOTICE);
Reviewed-by: Andrew Rybchenko
[PATCH] rcu: refactor rcu register/unregister functions
This simplifies the implementation of rte_rcu_qsbr_thread_register()
and rte_rcu_thread_unregister() functions. The simplified implementation
is easier to read.
Signed-off-by: Doug Foster
Reviewed-by: Honnappa Nagarahalli
Reviewed-by: Wathsala Vithanage
---
.mailmap | 1 +
lib/rcu/rte_rcu_qsbr.c | 77 --
2 files changed, 23 insertions(+), 55 deletions(-)
diff --git a/.mailmap b/.mailmap
index f76037213d..63d0f4187a 100644
--- a/.mailmap
+++ b/.mailmap
@@ -360,6 +360,7 @@ Don Provan
Don Wallwork
Doug Dziggel
Douglas Flint
+Doug Foster
Dr. David Alan Gilbert
Drocula Lambda
Dror Birkman
diff --git a/lib/rcu/rte_rcu_qsbr.c b/lib/rcu/rte_rcu_qsbr.c
index f08d974d07..40d7c566c8 100644
--- a/lib/rcu/rte_rcu_qsbr.c
+++ b/lib/rcu/rte_rcu_qsbr.c
@@ -81,8 +81,8 @@ rte_rcu_qsbr_init(struct rte_rcu_qsbr *v, uint32_t
max_threads)
int
rte_rcu_qsbr_thread_register(struct rte_rcu_qsbr *v, unsigned int thread_id)
{
- unsigned int i, id, success;
- uint64_t old_bmap, new_bmap;
+ unsigned int i, id;
+ uint64_t old_bmap;
if (v == NULL || thread_id >= v->max_threads) {
RCU_LOG(ERR, "Invalid input parameter");
@@ -97,31 +97,15 @@ rte_rcu_qsbr_thread_register(struct rte_rcu_qsbr *v,
unsigned int thread_id)
id = thread_id & __RTE_QSBR_THRID_MASK;
i = thread_id >> __RTE_QSBR_THRID_INDEX_SHIFT;
- /* Make sure that the counter for registered threads does not
-* go out of sync. Hence, additional checks are required.
-*/
- /* Check if the thread is already registered */
- old_bmap = rte_atomic_load_explicit(__RTE_QSBR_THRID_ARRAY_ELM(v, i),
- rte_memory_order_relaxed);
- if (old_bmap & 1UL << id)
- return 0;
+ /* Add the thread to the bitmap of registered threads */
+ old_bmap = rte_atomic_fetch_or_explicit(__RTE_QSBR_THRID_ARRAY_ELM(v,
i),
+ (1UL << id),
rte_memory_order_release);
- do {
- new_bmap = old_bmap | (1UL << id);
- success = rte_atomic_compare_exchange_strong_explicit(
- __RTE_QSBR_THRID_ARRAY_ELM(v, i),
- &old_bmap, new_bmap,
- rte_memory_order_release,
rte_memory_order_relaxed);
-
- if (success)
- rte_atomic_fetch_add_explicit(&v->num_threads,
- 1, rte_memory_order_relaxed);
- else if (old_bmap & (1UL << id))
- /* Someone else registered this thread.
-* Counter should not be incremented.
-*/
- return 0;
- } while (success == 0);
+ /* Increment the number of threads registered only if the thread was
not already
+* registered
+*/
+ if (!(old_bmap & (1UL << id)))
+ rte_atomic_fetch_add_explicit(&v->num_threads, 1,
rte_memory_order_relaxed);
return 0;
}
@@ -132,8 +116,8 @@ rte_rcu_qsbr_thread_register(struct rte_rcu_qsbr *v,
unsigned int thread_id)
int
rte_rcu_qsbr_thread_unregister(struct rte_rcu_qsbr *v, unsigned int thread_id)
{
- unsigned int i, id, success;
- uint64_t old_bmap, new_bmap;
+ unsigned int i, id;
+ uint64_t old_bmap;
if (v == NULL || thread_id >= v->max_threads) {
RCU_LOG(ERR, "Invalid input parameter");
@@ -148,35 +132,18 @@ rte_rcu_qsbr_thread_unregister(struct rte_rcu_qsbr *v,
unsigned int thread_id)
id = thread_id & __RTE_QSBR_THRID_MASK;
i = thread_id >> __RTE_QSBR_THRID_INDEX_SHIFT;
- /* Make sure that the counter for registered threads does not
-* go out of sync. Hence, additional checks are required.
+ /* Make sure any loads of the shared data structure are
+* completed before removal of the thread from the bitmap of
+* reporting threads.
*/
- /* Check if the thread is already unregistered */
- old_bmap = rte_atomic_load_explicit(__RTE_QSBR_THRID_ARRAY_ELM(v, i),
- rte_memory_order_relaxed);
- if (!(old_bmap & (1UL << id)))
- return 0;
+ old_bmap = rte_atomic_fetch_and_explicit(__RTE_QSBR_THRID_ARRAY_ELM(v,
i),
+~(1UL << id),
rte_memory_order_release);
- do {
- new_bmap = old_bmap & ~(1UL << id);
- /* Make sure any loads of the shared data structure are
-* completed before removal of the thread from the list of
-* reporting threads.
-*/
- success = rte_atomic_compare_exchange_strong_explicit(
- __RTE_QSBR_THRID_ARRAY_ELM(v, i),
-
Re: [PATCH 05/11] drivers: reuse default logtype for SFC drivers
On 9/7/24 17:54, David Marchand wrote: Nothing is fixed here, just reuse the value coming from the build framework rather than use a fixed string that may get wrong in the future. Signed-off-by: David Marchand Reviewed-by: Andrew Rybchenko
Re: [PATCH 11/11] drivers: use per line logging in helpers
On 9/7/24 17:54, David Marchand wrote:
Use RTE_LOG_LINE in existing macros that append a \n.
Signed-off-by: David Marchand
[snip]
diff --git a/drivers/common/sfc_efx/sfc_efx_log.h
b/drivers/common/sfc_efx/sfc_efx_log.h
index 1519ebdc17..b41ef3490b 100644
--- a/drivers/common/sfc_efx/sfc_efx_log.h
+++ b/drivers/common/sfc_efx/sfc_efx_log.h
@@ -12,11 +12,10 @@
/** Generic driver log type */
extern int sfc_efx_logtype;
+#define RTE_LOGTYPE_SFC_EFX sfc_efx_logtype
/** Log message, add a prefix and a line break */
#define SFC_EFX_LOG(level, ...) \
- rte_log(RTE_LOG_ ## level, sfc_efx_logtype, \
- RTE_FMT("sfc_efx: " RTE_FMT_HEAD(__VA_ARGS__ ,) "\n", \
Is "sfc_efx: " prefix dropped intentionally? Or should
RTE_LOG_LINE_PREFIX be used?
- RTE_FMT_TAIL(__VA_ARGS__ ,)))
+ RTE_LOG_LINE(level, SFC_EFX, ## __VA_ARGS__)
#endif /* _SFC_EFX_LOG_H_ */
[PATCH v2] ipsec: allow stateless IPsec processing
Introduce stateless packet preparation API for IPsec
processing. The new API would allow preparation of IPsec
packets without altering the internal state of an IPsec
session.
For outbound IPsec processing, the change enables user to
provide sequence number to be used for the IPsec operation.
Signed-off-by: Aakash Sasidharan
---
lib/ipsec/esp_outb.c | 85 +++
lib/ipsec/rte_ipsec.h | 68 ++
lib/ipsec/sa.c| 4 +-
lib/ipsec/sa.h| 8
4 files changed, 140 insertions(+), 25 deletions(-)
diff --git a/lib/ipsec/esp_outb.c b/lib/ipsec/esp_outb.c
index ec87b1dce2..de28cb166d 100644
--- a/lib/ipsec/esp_outb.c
+++ b/lib/ipsec/esp_outb.c
@@ -288,13 +288,12 @@ outb_pkt_xprepare(const struct rte_ipsec_sa *sa,
rte_be64_t sqc,
/*
* setup/update packets and crypto ops for ESP outbound tunnel case.
*/
-uint16_t
-esp_outb_tun_prepare(const struct rte_ipsec_session *ss, struct rte_mbuf *mb[],
- struct rte_crypto_op *cop[], uint16_t num)
+static inline uint16_t
+esp_outb_tun_prepare_helper(const struct rte_ipsec_session *ss, struct
rte_mbuf *mb[],
+ struct rte_crypto_op *cop[], uint16_t num, uint64_t sqn)
{
int32_t rc;
- uint32_t i, k, n;
- uint64_t sqn;
+ uint32_t i, k, n = num;
rte_be64_t sqc;
struct rte_ipsec_sa *sa;
struct rte_cryptodev_sym_session *cs;
@@ -305,11 +304,6 @@ esp_outb_tun_prepare(const struct rte_ipsec_session *ss,
struct rte_mbuf *mb[],
sa = ss->sa;
cs = ss->crypto.ses;
- n = num;
- sqn = esn_outb_update_sqn(sa, &n);
- if (n != num)
- rte_errno = EOVERFLOW;
-
k = 0;
for (i = 0; i != n; i++) {
@@ -339,6 +333,30 @@ esp_outb_tun_prepare(const struct rte_ipsec_session *ss,
struct rte_mbuf *mb[],
return k;
}
+uint16_t
+esp_outb_tun_prepare(const struct rte_ipsec_session *ss, struct rte_mbuf *mb[],
+ struct rte_crypto_op *cop[], uint16_t num)
+{
+ uint64_t sqn;
+ uint32_t n;
+
+ n = num;
+ sqn = esn_outb_update_sqn(ss->sa, &n);
+ if (n != num)
+ rte_errno = EOVERFLOW;
+
+ return esp_outb_tun_prepare_helper(ss, mb, cop, n, sqn);
+}
+
+uint16_t
+esp_outb_tun_prepare_stateless(const struct rte_ipsec_session *ss, struct
rte_mbuf *mb[],
+ struct rte_crypto_op *cop[], uint16_t num, struct rte_ipsec_state
*state)
+{
+ uint64_t sqn = state->sqn;
+
+ return esp_outb_tun_prepare_helper(ss, mb, cop, num, sqn);
+}
+
/*
* setup/update packet data and metadata for ESP outbound transport case.
*/
@@ -529,16 +547,15 @@ outb_cpu_crypto_prepare(const struct rte_ipsec_sa *sa,
uint32_t *pofs,
return clen;
}
-static uint16_t
-cpu_outb_pkt_prepare(const struct rte_ipsec_session *ss,
- struct rte_mbuf *mb[], uint16_t num,
- esp_outb_prepare_t prepare, uint32_t cofs_mask)
+static inline uint16_t
+cpu_outb_pkt_prepare_helper(const struct rte_ipsec_session *ss,
+ struct rte_mbuf *mb[], uint16_t num, esp_outb_prepare_t prepare,
+ uint32_t cofs_mask, uint64_t sqn)
{
int32_t rc;
- uint64_t sqn;
rte_be64_t sqc;
struct rte_ipsec_sa *sa;
- uint32_t i, k, n;
+ uint32_t i, k, n = num;
uint32_t l2, l3;
union sym_op_data icv;
struct rte_crypto_va_iova_ptr iv[num];
@@ -551,11 +568,6 @@ cpu_outb_pkt_prepare(const struct rte_ipsec_session *ss,
sa = ss->sa;
- n = num;
- sqn = esn_outb_update_sqn(sa, &n);
- if (n != num)
- rte_errno = EOVERFLOW;
-
for (i = 0, k = 0; i != n; i++) {
l2 = mb[i]->l2_len;
@@ -604,15 +616,40 @@ uint16_t
cpu_outb_tun_pkt_prepare(const struct rte_ipsec_session *ss,
struct rte_mbuf *mb[], uint16_t num)
{
- return cpu_outb_pkt_prepare(ss, mb, num, outb_tun_pkt_prepare, 0);
+ uint64_t sqn;
+ uint32_t n;
+
+ n = num;
+ sqn = esn_outb_update_sqn(ss->sa, &n);
+ if (n != num)
+ rte_errno = EOVERFLOW;
+
+ return cpu_outb_pkt_prepare_helper(ss, mb, n, outb_tun_pkt_prepare, 0,
sqn);
+}
+
+uint16_t
+cpu_outb_tun_pkt_prepare_stateless(const struct rte_ipsec_session *ss,
+ struct rte_mbuf *mb[], uint16_t num, struct rte_ipsec_state
*state)
+{
+ uint64_t sqn = state->sqn;
+
+ return cpu_outb_pkt_prepare_helper(ss, mb, num, outb_tun_pkt_prepare,
0, sqn);
}
uint16_t
cpu_outb_trs_pkt_prepare(const struct rte_ipsec_session *ss,
struct rte_mbuf *mb[], uint16_t num)
{
- return cpu_outb_pkt_prepare(ss, mb, num, outb_trs_pkt_prepare,
- UINT32_MAX);
+ uint64_t sqn;
+ uint32_t n;
+
+ n = num;
+ sqn = esn_outb_update_sqn(ss->sa, &n);
+ if (n != num)
+ rte_errno = EOVERFLOW;
+
+ return cpu_outb_pkt_prepare_helper(ss, mb, n, outb_trs
Does DPDK provide RX timestamps?
Hi. I am using Intel (i210) and AWS ENA network interface cards. I would like to measure the following RX latencies: 1) NIC to DPDK packet ring buffer 2) DPDK packet ring buffer to application via rte_eth_rx_burst. I don't mind measuring in nanoseconds or CPU cycles. Unfortunately I cannot find any mention of hardware timestamps. I found brief references to mbuf containing a timestamp in the dynamic fields, but nothing definitive. Could someone please clarify what the situation is? Thanks,
Re: [PATCH 0/3] eal: mark API's as stable
On Thu, 5 Sep 2024 16:18:13 +0200 Morten Brørup wrote: > > From: Jerin Jacob [mailto:jerinjac...@gmail.com] > > Sent: Thursday, 5 September 2024 16.02 > > > > On Thu, Sep 5, 2024 at 3:14 PM Morten Brørup > > wrote: > > > > > > > From: David Marchand [mailto:david.march...@redhat.com] > > > > Sent: Thursday, 5 September 2024 11.03 > > > > > > > > On Thu, Sep 5, 2024 at 10:55 AM Morten Brørup > > > > > > > > wrote: > > > > > > > > > > > From: David Marchand [mailto:david.march...@redhat.com] > > > > > > Sent: Thursday, 5 September 2024 09.59 > > > > > > > > > > > > On Wed, Sep 4, 2024 at 8:10 PM Stephen Hemminger > > > > > > wrote: > > > > > > > > > > > > > > The API's in ethtool from before 23.11 should be marked stable. > > > > > > > > > > > > EAL* ? > > > > > > > > > > > > > Should probably include the trace api's but that is more complex > > change. > > > > > > > > > > > > On the trace API itself it should be ok. > > > > > > > > > > No! > > > > > > > > *sigh* > > > > > > > > > > > > > > Trace must remain experimental until controlled by a meson option, > > > > > e.g. > > > > "enable_trace", whereby trace can be completely disabled and omitted > > > > from > > the > > > > compiled application/libraries/drivers at build time. > > > > > > > > This seems unrelated to marking the API stable as regardless of the > > > > API state at the moment, this code is always present. > > > > > > I cannot foresee if disabling trace at build time will require changes to > > > > > the trace API. So I'm being cautious here. > > > > > > However, if Jerin (as author of the trace subsystem) foresees that it > > > will > > be possible to disable trace at build time without affecting the trace API, > > I > > don't object to marking the trace API (or some of it) stable. > > > > I don't for foresee any ABI changes when adding disabling trace > > compile time support. > > Based on Jerin's feedback, I'm retracting my objection. > > > However, I don't understand why we need to do > > that. > > To reduce code size. > Relevant for embedded/memory-constrained systems. > > > In the sense, fast path functions are already having an option > > to compile out. > > Slow path functions can be disabled at runtime at the cost of 1 cycle > > as instrumentation cost. Having said that, I don't have any concern > > about disabling trace as an option. > > Great. > > > > > > > > > > > Before doing that, rte_trace_mode_get/set() and the accompanying enum > > rte_trace_mode should be changed to rte_trace_config_get/set() using a new > > struct rte_trace_config (containing the enum rte_trace_mode, and expandable > > with new fields as the need arises). This will prepare for e.g. tracing to > > other destinations than system memory, such as a remote trace collector on > > the > > network, like SYSLOG. > > I'm also retracting this precondition... > > If the need for further trace configuration should ever arise, > rte_trace_config_get/set() can be added later. > And rte_trace_mode_get/set(), if not marked as experimental anymore, will be > kept for backwards compatibility. > > > > > > > > Patches welcome if you want it stripped. > > > > > > Don't have time myself, so I suggested it as a code challenge instead. :-) > > > My feeling is that the the experimental flag is not intended as permanent "get out of ABI compatiablity"
Re: [RFC 1/3] uapi: introduce kernel uAPI headers importation
On Fri, 6 Sep 2024 00:15:26 +0200 Maxime Coquelin wrote: > This patch introduces uAPI headers importation into the > DPDK repository. This import is possible thanks to Linux > Kernel licence exception for syscalls: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/LICENSES/exceptions/Linux-syscall-note > > Header files are have to be explicitly imported, and > libraries and drivers have to explicitly enable their > inclusion. > > Guidelines are provided in the documentation, and a helper > script is also provided to ensure proper importation of the > header (unmodified content from a released Kernel version). > > Next version will introduce a script to check headers are > valids. > > Signed-off-by: Maxime Coquelin > --- > devtools/import-linux-uapi.sh | 48 > doc/guides/contributing/index.rst | 1 + > doc/guides/contributing/linux_uapi.rst | 63 ++ > meson.build| 4 ++ > 4 files changed, 116 insertions(+) > create mode 100755 devtools/import-linux-uapi.sh > create mode 100644 doc/guides/contributing/linux_uapi.rst > > diff --git a/devtools/import-linux-uapi.sh b/devtools/import-linux-uapi.sh > new file mode 100755 > index 00..efeffdd332 > --- /dev/null > +++ b/devtools/import-linux-uapi.sh > @@ -0,0 +1,48 @@ > +#!/bin/sh -e > +# SPDX-License-Identifier: BSD-3-Clause > +# Copyright (c) 2024 Red Hat, Inc. > + > +# > +# Import Linux Kernel uAPI header file > +# > + > +base_url="https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/plain/include/uapi/"; > +base_path="linux-headers/uapi/" Sorry, not a fan of this. This must be optional. Most other projects don't do this and it risks incompatibilities with the C library. Did you make sure the headers are exactly the same as the distro uses (for the same kernel version). Worried that this is not the exact same process that "make headers_install" might use. Also, ideally the tool would be selective.
Re: [RFC 1/4] thash: add RSS hash key generation API
On Fri, 6 Sep 2024 16:53:15 +
Vladimir Medvedkin wrote:
> diff --git a/lib/hash/version.map b/lib/hash/version.map
> index 11a5394a45..7d31fc1ba5 100644
> --- a/lib/hash/version.map
> +++ b/lib/hash/version.map
> @@ -52,6 +52,8 @@ EXPERIMENTAL {
>
> # added in 24.07
> rte_hash_rcu_qsbr_dq_reclaim;
> + # added in 24.11
> + rte_thash_gen_key;
> };
>
> INTERNAL {
> @@ -59,4 +61,4 @@ INTERNAL {
>
> rte_thash_gfni_stub;
> rte_thash_gfni_bulk_stub;
> -};
> +};
> \ No newline at end of file
Fix your editor settings to always end file with newline
Re: [RFC 2/4] hash: add dynamic polynomial calculation
On Fri, 6 Sep 2024 16:53:16 +
Vladimir Medvedkin wrote:
> +struct divisors {
> + int n; /* number of divisors */
> + int div_arr[MAX_DIVISORS];
> +};
Why int instead of a fixed size unsigned, like uint32_t?
Re: [RFC v2 0/3] Import Kernel uAPI header files
On Fri, 6 Sep 2024 17:23:34 +0200 Maxime Coquelin wrote: > This series enables importing Linux Kernel uAPI headers > into the DPDK repository. It aims at solving alignment > issues between the build system and the system applications > linked ot DPDK libraries are run on. > > It can also help simplify spaghetti code done to support > different versions of the Linux Kernel headers used by > the build system. > > Guidelines and import script are also part of first patch. > A uAPI checker script is added in this 2nd RFC, goals is to > use it in CI for any patch touching linux-headers/uapi. > > Follow-up patches are an example of imported uAPI inclusion > of VDUSE header into the Vhost library. > > Morten, I did not apply your Ack on patch 1, as it has > some significant changes and additions. I know of build systems are done in containers and are intentionally setup to not allow access to the public Internet. You will break them.
Re: [PATCH v2] doc: add new driver guidelines
Hi Ferruh, Thanks for the explanation. In the next new version: Acked-by: Chengwen Feng Thanks On 2024/9/6 16:27, Ferruh Yigit wrote: > On 9/6/2024 9:05 AM, fengchengwen wrote: >> On 2024/8/15 3:08, Stephen Hemminger wrote: >>> From: Nandini Persad >>> >>> This document was created to assist contributors in creating DPDK drivers >>> and provides suggestions and guidelines on how to upstream effectively. >>> >>> Co-authored-by: Ferruh Yigit >>> Co-authored-by: Thomas Monjalon >>> Signed-off-by: Nandini Persad >>> Reviewed-by: Stephen Hemminger >>> --- >>> >>> v2 - review feedback >>>- add co-author and reviewed-by >>> >>> doc/guides/contributing/index.rst | 1 + >>> doc/guides/contributing/new_driver.rst | 202 + >>> 2 files changed, 203 insertions(+) >>> create mode 100644 doc/guides/contributing/new_driver.rst >>> >> >> ... >> >>> + >>> +Finalizing >>> +-- >>> + >>> +Once the driver has been upstreamed, the author has >>> +a responsibility to the community to maintain it. >>> + >>> +This includes the public test report. Authors must send a public >>> +test report after the first upstreaming of the PMD. The same >>> +public test procedure may be reproduced regularly per release. >>> + >>> +After the PMD is upstreamed, the author should send a patch >>> +to update the website with the name of the new PMD and supported devices >>> +via the DPDK mailing list.. >> >> .. -> . >> >>> + >>> +For more information about the role of maintainers, see :doc:`patches`. >>> + >>> + >>> + >>> +Splitting into Patches >>> +-- >>> + >> >> ... >> >>> + >>> + >>> +The following order in the patch series is as suggested below. >>> + >>> +The first patch should have the driver's skeleton which should include: >>> + >>> +* Maintainer's file update >>> +* Driver documentation >>> +* Document must have links to official product documentation web page >>> +* The new document should be added into the index (`doc/guides/index.rst`) >> >> The new -> The new >> >> ... >> >>> + >>> +Additional Suggestions >>> +-- >>> + >>> +* We recommend using DPDK macros instead of inventing new ones in the PMD. >>> +* Do not include unused headers. Use the ./devtools/process-iwyu.py tool. >>> +* Do not disable compiler warnings in the build file. >>> +* Do not use #ifdef with driver-defined macros, instead prefer runtime >>> configuration. >>> +* Document device parameters in the driver guide. >>> +* Make device operations struct 'const'. >>> +* Use dynamic logging. >>> +* Do not use DPDK version checks in the upstream code. >> >> Could you explain it (DPDK version check) ? >> > > It refers usage of 'RTE_VERSION_NUM' macro. This may be required for out > of tree drivers, as they may be supporting multiple DPDK version. > > Not sure adding too much details for sure, what about following update: > `* Do not use DPDK version checks (via RTE_VERSION_NUM) in the upstream > code.` Got it, thanks > > >>> +* Be sure to have SPDX license tags and copyright notice on each side. >>> + Use ./devtools/check-spdx-tag.sh >>> +* Run the Coccinelle scripts ./devtools/cocci.sh which check for common >>> cleanups such as >>> + useless null checks before calling free routines. >>> + >>> +Dependencies >>> + >>> + >>> +At times, drivers may have dependencies to external software. >>> +For driver dependencies, same DPDK rules for dependencies applies. >>> +Dependencies should be publicly and freely available, >>> +or this is a blocker for upstreaming the driver. >> >> Could you explain it (what's the blocker) ? >> > > It is trying to say, this prevents upstreaming, wording can be updated > to clarify, what about following: > > `Dependencies should be publicly and freely available to be able to > upstream the driver.` +1 > > >>> + >>> + >>> +.. _tool_list: >>> + >>> +Test Tools >>> +-- >>> + >>> +Build and check the driver's documentation. Make sure there are no >>> +warnings and driver shows up in the relevant index page. >>> + >>> +Be sure to run the following test tools per patch in a patch series: >>> + >>> +* checkpatches.sh >>> +* check-git-log.sh >>> +* check-meson.py >>> +* check-doc-vs-code.sh >>> >> >> Some drivers already provide private APIs, I think we should add note >> for "not add private APIs, prefer to extend the corresponding framework API" >> for new drivers. >> > > Ack. > What about adding this to "Additional Suggestions", like following: > `Do not introduce public APIs directly from the driver.` +1 > > . >
Re: [PATCH 09/11] drivers: remove redundant newline from logs
for drivers/dma/hisilicon/ and drivers/net/hns3/ Acked-by: Chengwen Feng On 2024/9/7 22:54, David Marchand wrote: > Fix places where two newline characters may be logged. > > Cc: sta...@dpdk.org > > Signed-off-by: David Marchand > ---
Re: [PATCH 11/11] drivers: use per line logging in helpers
for drivers/dma/hisilicon/ and drivers/net/hns3/ Acked-by: Chengwen Feng On 2024/9/7 22:54, David Marchand wrote: > Use RTE_LOG_LINE in existing macros that append a \n. > > Signed-off-by: David Marchand
Re: [PATCH v10 1/3] event/dlb2: add support for independent enqueue
This commit should after "[PATCH v10 2/3] eventdev: add support for independent enqueue" because this commit use the macro which defined in later commit. Suggest order: 1. lib's commit 2. driver's commits On 2024/8/31 0:23, Abdullah Sevincer wrote: > DLB devices need events to be enqueued in the same order they are > dequeued. Applications are not suppose to change event order between > dequeue and to enqueue. Since Eventdev standard does not add such > restrictions independent enqueue support is needed for DLB PMD so that > it restores dequeue order on enqueue if applications happen to change > it. It also adds missing releases in places where events are dropped > by the application and it expects implicit release to handle it. > > By default the feature will be off on all DLB ports and they will > behave the same as older releases. To enable reordering feature, > applications need to add the flag RTE_EVENT_PORT_CFG_INDEPENDENT_ENQ > to port configuration if only the device advertises the capability > RTE_EVENT_DEV_CAP_INDEPENDENT_ENQ. > > Signed-off-by: Abdullah Sevincer > Acked-by: Mattias Rönnblom > --- ... > New Features > > > -.. This section should contain new features added in this release. > - Sample format: > +* **Updated DLB2 Driver for independent enqueue feature** > > - * **Add a title in the past tense with a full stop.** > - > - Add a short 1-2 sentence description in the past tense. > - The description should be enough to allow someone scanning > - the release notes to understand the new feature. > - > - If the feature adds a lot of sub-features you can use a bullet list > - like this: > - > - * Added feature foo to do something. > - * Enhanced feature bar to do something else. > - > - Refer to the previous release notes for examples. > - > - Suggested order in release notes items: > - * Core libs (EAL, mempool, ring, mbuf, buses) > - * Device abstraction libs and PMDs (ordered alphabetically by vendor > name) > - - ethdev (lib, PMDs) > - - cryptodev (lib, PMDs) > - - eventdev (lib, PMDs) > - - etc > - * Other libs > - * Apps, Examples, Tools (if significant) > - > - This section is a comment. Do not overwrite or remove it. > - Also, make sure to start the actual text at the margin. > - === The above line will remove when DPDK 24.11 released, please don't remove it when developing. ...
Re: [PATCH 0/3] eal: mark API's as stable
On Fri, Sep 6, 2024 at 7:33 PM Ferruh Yigit wrote: > > On 9/6/2024 2:11 PM, Jerin Jacob wrote: > > On Fri, Sep 6, 2024 at 3:04 PM Ferruh Yigit wrote: > >> > >> On 9/5/2024 8:58 AM, David Marchand wrote: > >>> On Wed, Sep 4, 2024 at 8:10 PM Stephen Hemminger > >>> wrote: > > The API's in ethtool from before 23.11 should be marked stable. > >>> > >>> EAL* ? > >>> > Should probably include the trace api's but that is more complex change. > >>> > >>> On the trace API itself it should be ok. > >>> The problem is with the tracepoint variables themselves, and I don't > >>> think we should mark them stable. > >>> > >> > >> We cleaned tracepoint variables from ethdev map file, why they exist for > >> 'eal'? > >> > >> I can see .map file has bunch of "__rte_eal_trace_generic_*", I think > >> they exists to support 'rte_eal_trace_generic_*()' APIs which can be > >> called from other libraries. > >> > >> Do we really need them? > >> Why not whoever calls them directly call 'rte_trace_point_emit_*' instead? > >> As these rte_eal_trace_generic_*()' not used at all, I assume this is > >> what done already. > >> > >> @Jerin, > >> what do think to remove 'rte_eal_trace_generic_*()' APIs, so trace > >> always keeps local to library, and don't bloat the eal .map file? > > > > The purpose of exposing rte_eal_trace_generic_* is that, applications > > can add generic trace points > > in the application. > > > > Can't applications use 'rte_trace_point_emit_*()' directly, as libraries > does? It is two different usages. 'rte_eal_trace_generic_ case is more like, application wants to simply emit int via generic trace API but not add any _new_ trace point.
Re: [PATCH 0/3] eal: mark API's as stable
On Fri, Sep 6, 2024 at 8:12 PM Morten Brørup wrote:
>
> > From: Ferruh Yigit [mailto:ferruh.yi...@amd.com]
> > Sent: Friday, 6 September 2024 16.12
> >
> > On 9/6/2024 11:04 AM, Morten Brørup wrote:
> > >> From: Ferruh Yigit [mailto:ferruh.yi...@amd.com]
> > >> Sent: Friday, 6 September 2024 10.54
> > >>
> > >> On 9/5/2024 3:01 PM, Jerin Jacob wrote:
> > >>> On Thu, Sep 5, 2024 at 3:14 PM Morten Brørup
> > >>>
> > >> wrote:
> >
> > > From: David Marchand [mailto:david.march...@redhat.com]
> > > Sent: Thursday, 5 September 2024 11.03
> > >
> > > On Thu, Sep 5, 2024 at 10:55 AM Morten Brørup
> > >
> > > wrote:
> > >>
> > >>> From: David Marchand [mailto:david.march...@redhat.com]
> > >>> Sent: Thursday, 5 September 2024 09.59
> > >>>
> > >>> On Wed, Sep 4, 2024 at 8:10 PM Stephen Hemminger
> > >>> wrote:
> >
> > The API's in ethtool from before 23.11 should be marked stable.
> > >>>
> > >>> EAL* ?
> > >>>
> > Should probably include the trace api's but that is more complex
> > >> change.
> > >>>
> > >>> On the trace API itself it should be ok.
> > >>
> > >> No!
> > >
> > > *sigh*
> > >
> > >>
> > >> Trace must remain experimental until controlled by a meson option,
> > >> e.g.
> > > "enable_trace", whereby trace can be completely disabled and omitted
> > from
> > >> the
> > > compiled application/libraries/drivers at build time.
> > >
> > > This seems unrelated to marking the API stable as regardless of the
> > > API state at the moment, this code is always present.
> >
> > I cannot foresee if disabling trace at build time will require changes
> > to
> > >> the trace API. So I'm being cautious here.
> >
> > However, if Jerin (as author of the trace subsystem) foresees that it
> > will
> > >> be possible to disable trace at build time without affecting the trace
> > >> API,
> > I
> > >> don't object to marking the trace API (or some of it) stable.
> > >>>
> > >>> I don't for foresee any ABI changes when adding disabling trace
> > >>> compile time support. However, I don't understand why we need to do
> > >>> that. In the sense, fast path functions are already having an option
> > >>> to compile out.
> > >>> Slow path functions can be disabled at runtime at the cost of 1 cycle
> > >>> as instrumentation cost. Having said that, I don't have any concern
> > >>> about disabling trace as an option.
> > >>>
> > >>
> > >> I agree with Jerin, I don't see motivation to disable slow path traces
> > >> when they can be disabled in runtime.
> > >> And fast path traces already have compile flag to disable them.
> > >>
> > >> Build time configurations in long term has problems too, so I am for not
> > >> using them unless we don't have to.
> > >
> > > For some use cases, trace is dead code, and should be omitted.
> > > You don't want dead code in production systems.
> > >
> > > Please remember that DPDK is also being used in highly optimized embedded
> > systems, hardware appliances and other systems where memory is not abundant.
> > >
> > > DPDK is not only for cloud and distros. ;-)
> > >
> > > The CI only tests DPDK with a build time configuration expected to be
> > > usable
> > for distros.
> > > I'm not asking to change that.
> > > I'm only asking for more build time configurability to support other use
> > cases.
> > >
> >
> > I see, but that build time configuration argument exists in multiple
> > aspects. And with meson switch we lean to dynamic configuration approach.
>
> It can be rte_config.h instead of Meson option. Either is perfectly fine with
> me.
>
> >
> > When a build time config introduced, again and again we are having cases
> > that specific code enabled with compile time macro broken and nobody
> > noticed.
>
> I acknowledge this risk.
if we use
if (0)
{
}
scheme instead of #ifdef scheme.
The compiler will check this leg even it is not active.
>
> Trace doesn't interact with anything, so I consider the risk extremely low in
> this case.
>
> > Having code enabled always and configured in runtime produces more
> > robust deliverable.
>
> The same can be said about the Linux kernel, but yet it is configurable.
>
> >
> > We are aware that DPDK is used in embedded device, but they are not
> > mostly very resource restricted devices, is it really matter to have a
> > few megabytes (I didn't check but I expect this the max binary size can
> > increase with tracing code) larger DPDK binary, does it really makes any
> > difference?
>
> Code size also affects system boot time, because those superfluous megabytes
> take time to decompress when booting from FLASH memory.
> For reference, the size of our system image (including Linux kernel, OpenSSL,
> the DPDK application, webserver, GUI, CLI, SNMP and everything) is 12 MB
> compressed.
>
> From a security aspect, trace also increases the attack surface and can
> pot
RE: [PATCH 07/11] net/txgbe: move wrapper to base driver
On Sat, Sep 7, 2024 10:54 PM, David Marchand wrote:
> BP_LOG() is only used in the base driver.
>
> Signed-off-by: David Marchand
> ---
> drivers/net/txgbe/base/txgbe_osdep.h | 8
> drivers/net/txgbe/txgbe_logs.h | 7 ---
> 2 files changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/txgbe/base/txgbe_osdep.h
> b/drivers/net/txgbe/base/txgbe_osdep.h
> index 62d16a6abb..91c8abf12e 100644
> --- a/drivers/net/txgbe/base/txgbe_osdep.h
> +++ b/drivers/net/txgbe/base/txgbe_osdep.h
> @@ -28,6 +28,14 @@
> #define TMZ_VADDR(mz) ((mz)->addr)
> #define TDEV_NAME(eth_dev) ((eth_dev)->device->name)
>
> +extern int txgbe_logtype_bp;
> +#define RTE_LOGTYPE_TXGBE_BP txgbe_logtype_bp
> +#define BP_LOG(fmt, ...) \
> + RTE_LOG(DEBUG, TXGBE_BP, \
> + "[%"PRIu64".%"PRIu64"]%s(%d): " fmt, \
> + usec_stamp() / 100, usec_stamp() % 100, \
> + __func__, __LINE__, ## __VA_ARGS__)
> +
> #define ASSERT(x) do { \
> if (!(x)) \
> PMD_DRV_LOG(ERR, "TXGBE: %d", x); \
> diff --git a/drivers/net/txgbe/txgbe_logs.h b/drivers/net/txgbe/txgbe_logs.h
> index 74f49ab9ef..b5a5a9233f 100644
> --- a/drivers/net/txgbe/txgbe_logs.h
> +++ b/drivers/net/txgbe/txgbe_logs.h
> @@ -51,11 +51,4 @@ extern int txgbe_logtype_tx_free;
> #define DEBUGOUT(fmt, args...)PMD_DRV_LOG(DEBUG, fmt, ##args)
> #define PMD_INIT_FUNC_TRACE() PMD_DRV_LOG(DEBUG, ">>")
>
> -extern int txgbe_logtype_bp;
> -#define BP_LOG(fmt, args...) \
> - rte_log(RTE_LOG_DEBUG, txgbe_logtype_bp, \
> - "[%"PRIu64".%"PRIu64"]%s(%d): " fmt, \
> - usec_stamp() / 100, usec_stamp() % 100, \
> - __func__, __LINE__, ##args)
> -
> #endif /* _TXGBE_LOGS_H_ */
> --
> 2.46.0
>
Hi,
Does this have to change? It looks a little weird.
Re: [PATCH 07/11] net/txgbe: move wrapper to base driver
On Mon, Sep 9, 2024 at 8:18 AM Jiawen Wu wrote:
>
> On Sat, Sep 7, 2024 10:54 PM, David Marchand wrote:
> > BP_LOG() is only used in the base driver.
> >
> > Signed-off-by: David Marchand
> > ---
> > drivers/net/txgbe/base/txgbe_osdep.h | 8
> > drivers/net/txgbe/txgbe_logs.h | 7 ---
> > 2 files changed, 8 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/net/txgbe/base/txgbe_osdep.h
> > b/drivers/net/txgbe/base/txgbe_osdep.h
> > index 62d16a6abb..91c8abf12e 100644
> > --- a/drivers/net/txgbe/base/txgbe_osdep.h
> > +++ b/drivers/net/txgbe/base/txgbe_osdep.h
> > @@ -28,6 +28,14 @@
> > #define TMZ_VADDR(mz) ((mz)->addr)
> > #define TDEV_NAME(eth_dev) ((eth_dev)->device->name)
> >
> > +extern int txgbe_logtype_bp;
> > +#define RTE_LOGTYPE_TXGBE_BP txgbe_logtype_bp
> > +#define BP_LOG(fmt, ...) \
> > + RTE_LOG(DEBUG, TXGBE_BP, \
> > + "[%"PRIu64".%"PRIu64"]%s(%d): " fmt, \
> > + usec_stamp() / 100, usec_stamp() % 100, \
> > + __func__, __LINE__, ## __VA_ARGS__)
> > +
> > #define ASSERT(x) do { \
> > if (!(x)) \
> > PMD_DRV_LOG(ERR, "TXGBE: %d", x); \
> > diff --git a/drivers/net/txgbe/txgbe_logs.h b/drivers/net/txgbe/txgbe_logs.h
> > index 74f49ab9ef..b5a5a9233f 100644
> > --- a/drivers/net/txgbe/txgbe_logs.h
> > +++ b/drivers/net/txgbe/txgbe_logs.h
> > @@ -51,11 +51,4 @@ extern int txgbe_logtype_tx_free;
> > #define DEBUGOUT(fmt, args...)PMD_DRV_LOG(DEBUG, fmt, ##args)
> > #define PMD_INIT_FUNC_TRACE() PMD_DRV_LOG(DEBUG, ">>")
> >
> > -extern int txgbe_logtype_bp;
> > -#define BP_LOG(fmt, args...) \
> > - rte_log(RTE_LOG_DEBUG, txgbe_logtype_bp, \
> > - "[%"PRIu64".%"PRIu64"]%s(%d): " fmt, \
> > - usec_stamp() / 100, usec_stamp() % 100, \
> > - __func__, __LINE__, ##args)
> > -
> > #endif /* _TXGBE_LOGS_H_ */
> > --
> > 2.46.0
> >
>
> Hi,
>
> Does this have to change? It looks a little weird.
Can you be more specific about the part that you don't like?
There is a change in behavior, I agree: messages would now be prefixed
with the logtype, here TXGBE_BP.
I can revert this part if you prefer.
As for moving the macro, in all other drivers in DPDK, the osdep.h
header provides helpers for the base driver code, so it is the right
location.
--
David Marchand
Re: [PATCH 11/11] drivers: use per line logging in helpers
On Sun, Sep 8, 2024 at 10:55 AM Andrew Rybchenko
wrote:
>
> On 9/7/24 17:54, David Marchand wrote:
> > Use RTE_LOG_LINE in existing macros that append a \n.
> >
> > Signed-off-by: David Marchand
>
> [snip]
>
> > diff --git a/drivers/common/sfc_efx/sfc_efx_log.h
> > b/drivers/common/sfc_efx/sfc_efx_log.h
> > index 1519ebdc17..b41ef3490b 100644
> > --- a/drivers/common/sfc_efx/sfc_efx_log.h
> > +++ b/drivers/common/sfc_efx/sfc_efx_log.h
> > @@ -12,11 +12,10 @@
> >
> > /** Generic driver log type */
> > extern int sfc_efx_logtype;
> > +#define RTE_LOGTYPE_SFC_EFX sfc_efx_logtype
> >
> > /** Log message, add a prefix and a line break */
> > #define SFC_EFX_LOG(level, ...) \
> > - rte_log(RTE_LOG_ ## level, sfc_efx_logtype, \
> > - RTE_FMT("sfc_efx: " RTE_FMT_HEAD(__VA_ARGS__ ,) "\n", \
>
> Is "sfc_efx: " prefix dropped intentionally? Or should
> RTE_LOG_LINE_PREFIX be used?
By moving to the RTE_LOG macro (and friends), a prefix is
automatically appended via the log type.
#define RTE_LOG(l, t, ...) \
rte_log(RTE_LOG_ ## l, \
RTE_LOGTYPE_ ## t, # t ": " __VA_ARGS__)
So here, the logs prefix is not dropped, but changed from sfc_efx: to SFC_EFX:
It is possible to keep it unchanged by defining RTE_LOGTYPE_sfc_efx is
you want to stick to it.
--
David Marchand
