Question about RTE ring
Hi, As part of a project I have a question about the rte ring. I’m using rte ring multi producer/single consumer. The producers are several process. If one producer is enqueuing an element and crashed (kill pid) in the middle of the enqueuing, can it compromise the ring ? Thanks !
[PATCH v5] net/i40e: support FEC feature
This patch enabled querying Forward Error Correction(FEC) capabilities,
set FEC mode and get current FEC mode functions.
Signed-off-by: Qiming Yang
Signed-off-by: Zhichao Zeng
---
v5: fix some judgments
v4: fix some logic
v3: optimize code details
v2: update NIC feature document
---
doc/guides/nics/features/i40e.ini | 1 +
doc/guides/rel_notes/release_24_07.rst | 4 +
drivers/net/i40e/i40e_ethdev.c | 237 +
3 files changed, 242 insertions(+)
diff --git a/doc/guides/nics/features/i40e.ini
b/doc/guides/nics/features/i40e.ini
index ef7514c44b..4610444ace 100644
--- a/doc/guides/nics/features/i40e.ini
+++ b/doc/guides/nics/features/i40e.ini
@@ -32,6 +32,7 @@ Traffic manager = Y
CRC offload = Y
VLAN offload = Y
QinQ offload = P
+FEC = Y
L3 checksum offload = P
L4 checksum offload = P
Inner L3 checksum= P
diff --git a/doc/guides/rel_notes/release_24_07.rst
b/doc/guides/rel_notes/release_24_07.rst
index a69f24cf99..1e65f70d6c 100644
--- a/doc/guides/rel_notes/release_24_07.rst
+++ b/doc/guides/rel_notes/release_24_07.rst
@@ -55,6 +55,10 @@ New Features
Also, make sure to start the actual text at the margin.
===
+* **Updated Intel i40e driver.**
+
+ * Added support for configuring the Forward Error Correction(FEC) mode,
querying
+ * FEC capabilities and current FEC mode from a device.
Removed Items
-
diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 380ce1a720..2235bbefda 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -406,6 +406,10 @@ static void i40e_ethertype_filter_restore(struct i40e_pf
*pf);
static void i40e_tunnel_filter_restore(struct i40e_pf *pf);
static void i40e_filter_restore(struct i40e_pf *pf);
static void i40e_notify_all_vfs_link_status(struct rte_eth_dev *dev);
+static int i40e_fec_get_capability(struct rte_eth_dev *dev,
+ struct rte_eth_fec_capa *speed_fec_capa, unsigned int num);
+static int i40e_fec_get(struct rte_eth_dev *dev, uint32_t *fec_capa);
+static int i40e_fec_set(struct rte_eth_dev *dev, uint32_t fec_capa);
static const char *const valid_keys[] = {
ETH_I40E_FLOATING_VEB_ARG,
@@ -521,6 +525,9 @@ static const struct eth_dev_ops i40e_eth_dev_ops = {
.tm_ops_get = i40e_tm_ops_get,
.tx_done_cleanup = i40e_tx_done_cleanup,
.get_monitor_addr = i40e_get_monitor_addr,
+ .fec_get_capability = i40e_fec_get_capability,
+ .fec_get = i40e_fec_get,
+ .fec_set = i40e_fec_set,
};
/* store statistics names and its offset in stats structure */
@@ -12297,6 +12304,236 @@ i40e_cloud_filter_qinq_create(struct i40e_pf *pf)
return ret;
}
+static int
+i40e_fec_get_capability(struct rte_eth_dev *dev,
+ struct rte_eth_fec_capa *speed_fec_capa, __rte_unused unsigned int num)
+{
+ struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+
+ if (hw->mac.type == I40E_MAC_X722 &&
+ !(hw->flags & I40E_HW_FLAG_X722_FEC_REQUEST_CAPABLE)) {
+ PMD_DRV_LOG(ERR, "Setting FEC encoding not supported by"
+" firmware. Please update the NVM image.\n");
+ return -ENOTSUP;
+ }
+
+ if (hw->device_id == I40E_DEV_ID_25G_SFP28 ||
+ hw->device_id == I40E_DEV_ID_25G_B) {
+ if (speed_fec_capa) {
+ speed_fec_capa->speed = RTE_ETH_SPEED_NUM_25G;
+ speed_fec_capa->capa =
RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC) |
+RTE_ETH_FEC_MODE_CAPA_MASK(BASER) |
+RTE_ETH_FEC_MODE_CAPA_MASK(AUTO) |
+RTE_ETH_FEC_MODE_CAPA_MASK(RS);
+ }
+
+ /* since HW only supports 25G */
+ return 1;
+ } else if (hw->device_id == I40E_DEV_ID_KX_X722) {
+ if (speed_fec_capa) {
+ speed_fec_capa->speed = RTE_ETH_SPEED_NUM_25G;
+ speed_fec_capa->capa = RTE_ETH_FEC_MODE_CAPA_MASK(AUTO)
|
+RTE_ETH_FEC_MODE_CAPA_MASK(RS);
+ }
+ return 1;
+ }
+
+ return -ENOTSUP;
+}
+
+static int
+i40e_fec_get(struct rte_eth_dev *dev, uint32_t *fec_capa)
+{
+ struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+ struct i40e_aq_get_phy_abilities_resp abilities = {0};
+ struct i40e_link_status link_status = {0};
+ uint8_t current_fec_mode = 0, fec_config = 0;
+ bool link_up, enable_lse;
+ int ret = 0;
+
+ enable_lse = dev->data->dev_conf.intr_conf.lsc ? true : false;
+ /* Get link info */
+ ret = i40e_aq_get_link_info(hw,
RE: Question about RTE ring
Hi, > > As part of a project I have a question about the rte ring. > I’m using rte ring multi producer/single consumer. > The producers are several process. > If one producer is enqueuing an element and crashed (kill pid) in the middle > of the > enqueuing, can it compromise the ring ? I suppose you are using rte_ring as IPC mechanism between multiple processes, correct? In theory - yes, if your producer crashed during enqueue() to the ring, then yes, the ring might be affected. If producer already moved prod.head and crashed before updating prod.tail, then no other producers will be able to enqueue() into the ring, till you'll do reset() for it. I expect such situation really rare and hard to reproduce, but in theory it is possible. Konstantin
[PATCH 0/3] cryptodev: add API to get used queue pair depth
Added a new fast path API to get the number of used crypto device queue pair depth at any given point. An implementation in cnxk crypto driver is also added along with a test case in test app. The addition of new API causes an ABI warning. This is suppressed as the updated struct rte_crypto_fp_ops is an internal structure and not to be used by application directly. Akhil Goyal (3): cryptodev: add API to get used queue pair depth crypto/cnxk: support queue pair depth API test/crypto: add QP depth used count case app/test/test_cryptodev.c| 117 +++ devtools/libabigail.abignore | 3 + drivers/crypto/cnxk/cn10k_cryptodev.c| 1 + drivers/crypto/cnxk/cn9k_cryptodev.c | 2 + drivers/crypto/cnxk/cnxk_cryptodev_ops.c | 15 +++ drivers/crypto/cnxk/cnxk_cryptodev_ops.h | 2 + lib/cryptodev/cryptodev_pmd.c| 1 + lib/cryptodev/cryptodev_pmd.h| 2 + lib/cryptodev/cryptodev_trace_points.c | 3 + lib/cryptodev/rte_cryptodev.h| 45 + lib/cryptodev/rte_cryptodev_core.h | 7 +- lib/cryptodev/rte_cryptodev_trace_fp.h | 7 ++ 12 files changed, 204 insertions(+), 1 deletion(-) -- 2.25.1
[PATCH 1/3] cryptodev: add API to get used queue pair depth
Added a new fast path API to get used queue pair
descriptors of a specific queue pair of a device.
Applications may monitor the depth used and enqueue
crypto ops accordingly.
Signed-off-by: Akhil Goyal
---
devtools/libabigail.abignore | 3 ++
lib/cryptodev/cryptodev_pmd.c | 1 +
lib/cryptodev/cryptodev_pmd.h | 2 ++
lib/cryptodev/cryptodev_trace_points.c | 3 ++
lib/cryptodev/rte_cryptodev.h | 45 ++
lib/cryptodev/rte_cryptodev_core.h | 7 +++-
lib/cryptodev/rte_cryptodev_trace_fp.h | 7
7 files changed, 67 insertions(+), 1 deletion(-)
diff --git a/devtools/libabigail.abignore b/devtools/libabigail.abignore
index 645d289a77..bd63f42008 100644
--- a/devtools/libabigail.abignore
+++ b/devtools/libabigail.abignore
@@ -37,3 +37,6 @@
[suppress_type]
name = rte_eth_fp_ops
has_data_member_inserted_between = {offset_of(reserved2), end}
+[suppress_type]
+ name = rte_crypto_fp_ops
+ has_data_member_inserted_between = {offset_of(reserved), end}
diff --git a/lib/cryptodev/cryptodev_pmd.c b/lib/cryptodev/cryptodev_pmd.c
index d8073a601d..87ced122b4 100644
--- a/lib/cryptodev/cryptodev_pmd.c
+++ b/lib/cryptodev/cryptodev_pmd.c
@@ -236,6 +236,7 @@ cryptodev_fp_ops_set(struct rte_crypto_fp_ops *fp_ops,
fp_ops->qp.data = dev->data->queue_pairs;
fp_ops->qp.enq_cb = dev->enq_cbs;
fp_ops->qp.deq_cb = dev->deq_cbs;
+ fp_ops->qp_depth_used = dev->qp_depth_used;
}
void *
diff --git a/lib/cryptodev/cryptodev_pmd.h b/lib/cryptodev/cryptodev_pmd.h
index d195b81771..c22cc0908d 100644
--- a/lib/cryptodev/cryptodev_pmd.h
+++ b/lib/cryptodev/cryptodev_pmd.h
@@ -117,6 +117,8 @@ struct __rte_cache_aligned rte_cryptodev {
struct rte_cryptodev_cb_rcu *enq_cbs;
/** User application callback for post dequeue processing */
struct rte_cryptodev_cb_rcu *deq_cbs;
+ /** Pointer to PMD function to get used queue pair depth */
+ crypto_qp_depth_used_t qp_depth_used;
};
/** Global structure used for maintaining state of allocated crypto devices */
diff --git a/lib/cryptodev/cryptodev_trace_points.c
b/lib/cryptodev/cryptodev_trace_points.c
index 8c47ab1e78..7403412553 100644
--- a/lib/cryptodev/cryptodev_trace_points.c
+++ b/lib/cryptodev/cryptodev_trace_points.c
@@ -194,3 +194,6 @@ RTE_TRACE_POINT_REGISTER(rte_cryptodev_trace_op_pool_create,
RTE_TRACE_POINT_REGISTER(rte_cryptodev_trace_count,
lib.cryptodev.count)
+
+RTE_TRACE_POINT_REGISTER(rte_cryptodev_trace_qp_depth_used,
+ lib.cryptodev.qp_depth_used)
diff --git a/lib/cryptodev/rte_cryptodev.h b/lib/cryptodev/rte_cryptodev.h
index 00ba6a234a..d6d7938f84 100644
--- a/lib/cryptodev/rte_cryptodev.h
+++ b/lib/cryptodev/rte_cryptodev.h
@@ -2005,6 +2005,51 @@ rte_cryptodev_enqueue_burst(uint8_t dev_id, uint16_t
qp_id,
return fp_ops->enqueue_burst(qp, ops, nb_ops);
}
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
+ *
+ * Get the number of used descriptors or depth of a cryptodev queue pair.
+ *
+ * This function retrieves the number of used descriptors in a crypto queue.
+ * Applications can use this API in the fast path to inspect QP occupancy and
+ * take appropriate action.
+ *
+ * Since it is a fast-path function, no check is performed on dev_id and qp_id.
+ * Caller must therefore ensure that the device is enabled and queue pair is
setup.
+ *
+ * @param dev_id The identifier of the device.
+ * @param qp_id The index of the queue pair for which used
descriptor
+ * count is to be retrieved. The value
+ * must be in the range [0, nb_queue_pairs - 1]
+ * previously supplied to
*rte_cryptodev_configure*.
+ *
+ * @return
+ * The number of used descriptors on the specified queue pair, or:
+ * - (-ENOTSUP) if the device does not support this function.
+ */
+
+__rte_experimental
+static inline int
+rte_cryptodev_qp_depth_used(uint8_t dev_id, uint16_t qp_id)
+{
+ const struct rte_crypto_fp_ops *fp_ops;
+ void *qp;
+ int rc;
+
+ fp_ops = &rte_crypto_fp_ops[dev_id];
+ qp = fp_ops->qp.data[qp_id];
+
+ if (fp_ops->qp_depth_used == NULL) {
+ rc = -ENOTSUP;
+ goto out;
+ }
+
+ rc = fp_ops->qp_depth_used(qp);
+out:
+ rte_cryptodev_trace_qp_depth_used(dev_id, qp_id);
+ return rc;
+}
#ifdef __cplusplus
diff --git a/lib/cryptodev/rte_cryptodev_core.h
b/lib/cryptodev/rte_cryptodev_core.h
index 8d7e58d76d..9d68a026d9 100644
--- a/lib/cryptodev/rte_cryptodev_core.h
+++ b/lib/cryptodev/rte_cryptodev_core.h
@@ -24,6 +24,9 @@ typedef uint16_t (*enqueue_pkt_burst_t)(void *qp,
struct rte_crypto_op **ops, uint16_t nb_ops);
/**< Enqueue packets for processing on queue pair of a device. */
+typedef uint32_t (*crypto_qp_depth_used_t)(void *
[PATCH 2/3] crypto/cnxk: support queue pair depth API
Added support to get the used queue pair depth
for a specific queue on cn10k platform.
Signed-off-by: Akhil Goyal
---
drivers/crypto/cnxk/cn10k_cryptodev.c| 1 +
drivers/crypto/cnxk/cn9k_cryptodev.c | 2 ++
drivers/crypto/cnxk/cnxk_cryptodev_ops.c | 15 +++
drivers/crypto/cnxk/cnxk_cryptodev_ops.h | 2 ++
4 files changed, 20 insertions(+)
diff --git a/drivers/crypto/cnxk/cn10k_cryptodev.c
b/drivers/crypto/cnxk/cn10k_cryptodev.c
index 5ed918e18e..70bef13cda 100644
--- a/drivers/crypto/cnxk/cn10k_cryptodev.c
+++ b/drivers/crypto/cnxk/cn10k_cryptodev.c
@@ -99,6 +99,7 @@ cn10k_cpt_pci_probe(struct rte_pci_driver *pci_drv
__rte_unused,
dev->driver_id = cn10k_cryptodev_driver_id;
dev->feature_flags = cnxk_cpt_default_ff_get();
+ dev->qp_depth_used = cnxk_cpt_qp_depth_used;
cn10k_cpt_set_enqdeq_fns(dev, vf);
cn10k_sec_ops_override();
diff --git a/drivers/crypto/cnxk/cn9k_cryptodev.c
b/drivers/crypto/cnxk/cn9k_cryptodev.c
index 47b0874185..818458bd6f 100644
--- a/drivers/crypto/cnxk/cn9k_cryptodev.c
+++ b/drivers/crypto/cnxk/cn9k_cryptodev.c
@@ -15,6 +15,7 @@
#include "cn9k_ipsec.h"
#include "cnxk_cryptodev.h"
#include "cnxk_cryptodev_capabilities.h"
+#include "cnxk_cryptodev_ops.h"
#include "cnxk_cryptodev_sec.h"
#include "roc_api.h"
@@ -96,6 +97,7 @@ cn9k_cpt_pci_probe(struct rte_pci_driver *pci_drv
__rte_unused,
dev->dev_ops = &cn9k_cpt_ops;
dev->driver_id = cn9k_cryptodev_driver_id;
dev->feature_flags = cnxk_cpt_default_ff_get();
+ dev->qp_depth_used = cnxk_cpt_qp_depth_used;
cnxk_cpt_caps_populate(vf);
diff --git a/drivers/crypto/cnxk/cnxk_cryptodev_ops.c
b/drivers/crypto/cnxk/cnxk_cryptodev_ops.c
index 1dd1dbac9a..2af4318023 100644
--- a/drivers/crypto/cnxk/cnxk_cryptodev_ops.c
+++ b/drivers/crypto/cnxk/cnxk_cryptodev_ops.c
@@ -496,6 +496,21 @@ cnxk_cpt_queue_pair_setup(struct rte_cryptodev *dev,
uint16_t qp_id,
return ret;
}
+uint32_t
+cnxk_cpt_qp_depth_used(void *qptr)
+{
+ struct cnxk_cpt_qp *qp = qptr;
+ struct pending_queue *pend_q;
+ union cpt_fc_write_s fc;
+
+ pend_q = &qp->pend_q;
+
+ fc.u64[0] = rte_atomic_load_explicit(qp->lmtline.fc_addr,
rte_memory_order_relaxed);
+
+ return RTE_MAX(pending_queue_infl_cnt(pend_q->head, pend_q->tail,
pend_q->pq_mask),
+ fc.s.qsize);
+}
+
unsigned int
cnxk_cpt_sym_session_get_size(struct rte_cryptodev *dev __rte_unused)
{
diff --git a/drivers/crypto/cnxk/cnxk_cryptodev_ops.h
b/drivers/crypto/cnxk/cnxk_cryptodev_ops.h
index e7bba25cb8..708fad910d 100644
--- a/drivers/crypto/cnxk/cnxk_cryptodev_ops.h
+++ b/drivers/crypto/cnxk/cnxk_cryptodev_ops.h
@@ -142,6 +142,8 @@ int cnxk_ae_session_cfg(struct rte_cryptodev *dev,
void cnxk_cpt_dump_on_err(struct cnxk_cpt_qp *qp);
int cnxk_cpt_queue_pair_event_error_query(struct rte_cryptodev *dev, uint16_t
qp_id);
+uint32_t cnxk_cpt_qp_depth_used(void *qptr);
+
static __rte_always_inline void
pending_queue_advance(uint64_t *index, const uint64_t mask)
{
--
2.25.1
[PATCH 3/3] test/crypto: add QP depth used count case
Added a test case to verify the new API
rte_cryptodev_qp_depth_used() to get the used
depth of a crypto device queue pair.
Signed-off-by: Akhil Goyal
---
app/test/test_cryptodev.c | 117 ++
1 file changed, 117 insertions(+)
diff --git a/app/test/test_cryptodev.c b/app/test/test_cryptodev.c
index 1703ebccf1..f2d249f6b8 100644
--- a/app/test/test_cryptodev.c
+++ b/app/test/test_cryptodev.c
@@ -2400,6 +2400,121 @@ static const uint8_t ms_hmac_digest2[] = {
/* End Session 2 */
+#define MAX_OPS_PROCESSED (MAX_NUM_OPS_INFLIGHT - 1)
+static int
+test_queue_pair_descriptor_count(void)
+{
+ struct crypto_testsuite_params *ts_params = &testsuite_params;
+ struct crypto_unittest_params *ut_params = &unittest_params;
+ struct rte_crypto_op *ops_deq[MAX_OPS_PROCESSED] = { NULL };
+ struct rte_crypto_op *ops[MAX_OPS_PROCESSED] = { NULL };
+ struct rte_cryptodev_sym_capability_idx cap_idx;
+ int qp_depth = 0;
+ int i;
+
+ RTE_VERIFY(gbl_action_type != RTE_SECURITY_ACTION_TYPE_CPU_CRYPTO);
+
+ /* Verify if the queue pair depth API is supported by driver */
+ qp_depth = rte_cryptodev_qp_depth_used(ts_params->valid_devs[0], 0);
+ if (qp_depth == -ENOTSUP)
+ return TEST_SKIPPED;
+
+ /* Verify the capabilities */
+ cap_idx.type = RTE_CRYPTO_SYM_XFORM_AUTH;
+ cap_idx.algo.auth = RTE_CRYPTO_AUTH_SHA1_HMAC;
+ if (rte_cryptodev_sym_capability_get(ts_params->valid_devs[0],
&cap_idx) == NULL)
+ return TEST_SKIPPED;
+
+ cap_idx.type = RTE_CRYPTO_SYM_XFORM_CIPHER;
+ cap_idx.algo.cipher = RTE_CRYPTO_CIPHER_AES_CBC;
+ if (rte_cryptodev_sym_capability_get(ts_params->valid_devs[0],
&cap_idx) == NULL)
+ return TEST_SKIPPED;
+
+ /* Setup Cipher Parameters */
+ ut_params->cipher_xform.type = RTE_CRYPTO_SYM_XFORM_CIPHER;
+ ut_params->cipher_xform.next = &ut_params->auth_xform;
+ ut_params->cipher_xform.cipher.algo = RTE_CRYPTO_CIPHER_AES_CBC;
+ ut_params->cipher_xform.cipher.op = RTE_CRYPTO_CIPHER_OP_ENCRYPT;
+ ut_params->cipher_xform.cipher.key.data = aes_cbc_key;
+ ut_params->cipher_xform.cipher.key.length = CIPHER_KEY_LENGTH_AES_CBC;
+ ut_params->cipher_xform.cipher.iv.offset = IV_OFFSET;
+ ut_params->cipher_xform.cipher.iv.length = CIPHER_IV_LENGTH_AES_CBC;
+
+ /* Setup HMAC Parameters */
+ ut_params->auth_xform.type = RTE_CRYPTO_SYM_XFORM_AUTH;
+ ut_params->auth_xform.next = NULL;
+ ut_params->auth_xform.auth.op = RTE_CRYPTO_AUTH_OP_GENERATE;
+ ut_params->auth_xform.auth.algo = RTE_CRYPTO_AUTH_SHA1_HMAC;
+ ut_params->auth_xform.auth.key.length = HMAC_KEY_LENGTH_SHA1;
+ ut_params->auth_xform.auth.key.data = hmac_sha1_key;
+ ut_params->auth_xform.auth.digest_length = DIGEST_BYTE_LENGTH_SHA1;
+
+ rte_errno = 0;
+ ut_params->sess =
rte_cryptodev_sym_session_create(ts_params->valid_devs[0],
+ &ut_params->cipher_xform, ts_params->session_mpool);
+ if (rte_errno == ENOTSUP)
+ return TEST_SKIPPED;
+
+ TEST_ASSERT_NOT_NULL(ut_params->sess, "Session creation failed");
+
+ TEST_ASSERT_EQUAL(rte_crypto_op_bulk_alloc(ts_params->op_mpool,
+ RTE_CRYPTO_OP_TYPE_SYMMETRIC, ops, MAX_OPS_PROCESSED),
+ MAX_OPS_PROCESSED, "failed to generate burst of crypto
ops");
+
+ /* Generate crypto op data structure */
+ for (i = 0; i < MAX_OPS_PROCESSED; i++) {
+ struct rte_mbuf *m;
+ uint8_t *digest;
+
+ /* Generate test mbuf data and space for digest */
+ m = setup_test_string(ts_params->mbuf_pool, catch_22_quote,
QUOTE_512_BYTES, 0);
+ TEST_ASSERT_NOT_NULL(m, "Failed to allocate mbuf");
+
+ digest = (uint8_t *)rte_pktmbuf_append(m,
DIGEST_BYTE_LENGTH_SHA1);
+ TEST_ASSERT_NOT_NULL(digest, "no room to append digest");
+
+ rte_crypto_op_attach_sym_session(ops[i], ut_params->sess);
+
+ /* set crypto operation source mbuf */
+ ops[i]->sym->m_src = m;
+
+ /* Set crypto operation authentication parameters */
+ ops[i]->sym->auth.digest.data = digest;
+ ops[i]->sym->auth.digest.phys_addr = rte_pktmbuf_iova_offset(m,
QUOTE_512_BYTES);
+
+ ops[i]->sym->auth.data.offset = 0;
+ ops[i]->sym->auth.data.length = QUOTE_512_BYTES;
+
+ /* Copy IV at the end of the crypto operation */
+ memcpy(rte_crypto_op_ctod_offset(ops[i], uint8_t *, IV_OFFSET),
aes_cbc_iv,
+ CIPHER_IV_LENGTH_AES_CBC);
+
+ /* Set crypto operation cipher parameters */
+ ops[i]->sym->cipher.data.offset = 0;
+ ops[i]->sym->cipher.data.length = QUOTE_512_BYTES;
+
+
TEST_ASSERT_EQU
[PATCH] app/crypto-perf: support IPsec/TLS segmented buffers
Added support to allow segmented buffers for IPsec
and tls-record security offload cases.
Signed-off-by: Akhil Goyal
---
app/test-crypto-perf/cperf_ops.c | 55
1 file changed, 34 insertions(+), 21 deletions(-)
diff --git a/app/test-crypto-perf/cperf_ops.c b/app/test-crypto-perf/cperf_ops.c
index d3fd115bc0..4ca001b721 100644
--- a/app/test-crypto-perf/cperf_ops.c
+++ b/app/test-crypto-perf/cperf_ops.c
@@ -43,10 +43,8 @@ test_ipsec_vec_populate(struct rte_mbuf *m, const struct
cperf_options *options,
struct rte_ipv4_hdr *ip = rte_pktmbuf_mtod(m, struct rte_ipv4_hdr *);
if (options->is_outbound) {
- memcpy(ip, test_vector->plaintext.data,
- sizeof(struct rte_ipv4_hdr));
-
- ip->total_length = rte_cpu_to_be_16(m->data_len);
+ memcpy(ip, test_vector->plaintext.data, sizeof(struct
rte_ipv4_hdr));
+ ip->total_length = rte_cpu_to_be_16(m->pkt_len);
}
}
@@ -131,8 +129,6 @@ cperf_set_ops_security_ipsec(struct rte_crypto_op **ops,
{
void *sec_sess = sess;
const uint32_t test_buffer_size = options->test_buffer_size;
- const uint32_t headroom_sz = options->headroom_sz;
- const uint32_t segment_sz = options->segment_sz;
uint64_t tsc_start_temp, tsc_end_temp;
uint16_t i = 0;
@@ -141,20 +137,27 @@ cperf_set_ops_security_ipsec(struct rte_crypto_op **ops,
for (i = 0; i < nb_ops; i++) {
struct rte_crypto_sym_op *sym_op = ops[i]->sym;
struct rte_mbuf *m = sym_op->m_src;
+ uint32_t offset = test_buffer_size;
ops[i]->status = RTE_CRYPTO_OP_STATUS_NOT_PROCESSED;
rte_security_attach_session(ops[i], sec_sess);
- sym_op->m_src = (struct rte_mbuf *)((uint8_t *)ops[i] +
- src_buf_offset);
+ sym_op->m_src = (struct rte_mbuf *)((uint8_t *)ops[i] +
src_buf_offset);
+ sym_op->m_src->pkt_len = test_buffer_size;
- /* In case of IPsec, headroom is consumed by PMD,
-* hence resetting it.
+ while ((m->next != NULL) && (offset >= m->data_len)) {
+ offset -= m->data_len;
+ m = m->next;
+ }
+ m->data_len = offset;
+ /*
+* If there is not enough room in segment,
+* place the digest in the next segment
*/
- m->data_off = headroom_sz;
-
- m->buf_len = segment_sz;
- m->data_len = test_buffer_size;
- m->pkt_len = test_buffer_size;
+ if (rte_pktmbuf_tailroom(m) < options->digest_sz) {
+ m = m->next;
+ offset = 0;
+ }
+ m->next = NULL;
sym_op->m_dst = NULL;
}
@@ -186,8 +189,6 @@ cperf_set_ops_security_tls(struct rte_crypto_op **ops,
uint64_t *tsc_start)
{
const uint32_t test_buffer_size = options->test_buffer_size;
- const uint32_t headroom_sz = options->headroom_sz;
- const uint32_t segment_sz = options->segment_sz;
uint16_t i = 0;
RTE_SET_USED(imix_idx);
@@ -197,16 +198,28 @@ cperf_set_ops_security_tls(struct rte_crypto_op **ops,
for (i = 0; i < nb_ops; i++) {
struct rte_crypto_sym_op *sym_op = ops[i]->sym;
struct rte_mbuf *m = sym_op->m_src;
+ uint32_t offset = test_buffer_size;
ops[i]->status = RTE_CRYPTO_OP_STATUS_NOT_PROCESSED;
ops[i]->param1.tls_record.content_type = 0x17;
rte_security_attach_session(ops[i], sess);
sym_op->m_src = (struct rte_mbuf *)((uint8_t *)ops[i] +
src_buf_offset);
+ sym_op->m_src->pkt_len = test_buffer_size;
- m->data_off = headroom_sz;
- m->buf_len = segment_sz;
- m->data_len = test_buffer_size;
- m->pkt_len = test_buffer_size;
+ while ((m->next != NULL) && (offset >= m->data_len)) {
+ offset -= m->data_len;
+ m = m->next;
+ }
+ m->data_len = offset;
+ /*
+* If there is not enough room in segment,
+* place the digest in the next segment
+*/
+ if ((rte_pktmbuf_tailroom(m)) < options->digest_sz) {
+ m = m->next;
+ m->data_len = 0;
+ }
+ m->next = NULL;
sym_op->m_dst = NULL;
}
--
2.25.1
Re: [PATCH v2 2/8] net/ice: enhance debug when HW fails to transmit
On Mon, Apr 8, 2024 at 5:23 PM Bruce Richardson
wrote:
>
> On Fri, Apr 05, 2024 at 04:45:56PM +0200, David Marchand wrote:
> > At the moment, if the driver sets an incorrect Tx descriptor, the HW
> > will raise a MDD event reported as:
> > ice_interrupt_handler(): OICR: MDD event
> >
> > Add some debug info for this case and the VF index in all logs.
> >
> > Signed-off-by: David Marchand
> > ---
> > drivers/net/ice/ice_ethdev.c | 29 +
> > 1 file changed, 25 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
> > index 87385d2649..fd494e6b3b 100644
> > --- a/drivers/net/ice/ice_ethdev.c
> > +++ b/drivers/net/ice/ice_ethdev.c
> > @@ -1389,6 +1389,7 @@ ice_interrupt_handler(void *param)
> > uint32_t oicr;
> > uint32_t reg;
> > uint8_t pf_num;
> > + uint16_t vf_num;
> > uint8_t event;
> > uint16_t queue;
> > int ret;
> > @@ -1432,28 +1433,48 @@ ice_interrupt_handler(void *param)
> > if (reg & GL_MDET_TX_PQM_VALID_M) {
> > pf_num = (reg & GL_MDET_TX_PQM_PF_NUM_M) >>
> >GL_MDET_TX_PQM_PF_NUM_S;
> > + vf_num = (reg & GL_MDET_TX_PQM_VF_NUM_M) >>
> > + GL_MDET_TX_PQM_VF_NUM_S;
> > event = (reg & GL_MDET_TX_PQM_MAL_TYPE_M) >>
> > GL_MDET_TX_PQM_MAL_TYPE_S;
> > queue = (reg & GL_MDET_TX_PQM_QNUM_M) >>
> > GL_MDET_TX_PQM_QNUM_S;
> >
> > PMD_DRV_LOG(WARNING, "Malicious Driver Detection
> > event "
> > - "%d by PQM on TX queue %d PF# %d",
> > - event, queue, pf_num);
> > + "%d by PQM on TX queue %d PF# %d VF# %d",
> > + event, queue, pf_num, vf_num);
> > }
> >
> Would this output be misleading in the case where there is no VF involved
> and the actual MDD error comes from the PF?
I will check, but IIRC, the queue here is an "absolute" queue number
that should help figure out whether it is the PF or a VF in the case
vf_num == 0.
--
David Marchand
Re: [PATCH v2 2/8] net/ice: enhance debug when HW fails to transmit
On Thu, Apr 11, 2024 at 10:30:19AM +0200, David Marchand wrote:
> On Mon, Apr 8, 2024 at 5:23 PM Bruce Richardson
> wrote:
> >
> > On Fri, Apr 05, 2024 at 04:45:56PM +0200, David Marchand wrote:
> > > At the moment, if the driver sets an incorrect Tx descriptor, the HW
> > > will raise a MDD event reported as:
> > > ice_interrupt_handler(): OICR: MDD event
> > >
> > > Add some debug info for this case and the VF index in all logs.
> > >
> > > Signed-off-by: David Marchand
> > > ---
> > > drivers/net/ice/ice_ethdev.c | 29 +
> > > 1 file changed, 25 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
> > > index 87385d2649..fd494e6b3b 100644
> > > --- a/drivers/net/ice/ice_ethdev.c
> > > +++ b/drivers/net/ice/ice_ethdev.c
> > > @@ -1389,6 +1389,7 @@ ice_interrupt_handler(void *param)
> > > uint32_t oicr;
> > > uint32_t reg;
> > > uint8_t pf_num;
> > > + uint16_t vf_num;
> > > uint8_t event;
> > > uint16_t queue;
> > > int ret;
> > > @@ -1432,28 +1433,48 @@ ice_interrupt_handler(void *param)
> > > if (reg & GL_MDET_TX_PQM_VALID_M) {
> > > pf_num = (reg & GL_MDET_TX_PQM_PF_NUM_M) >>
> > >GL_MDET_TX_PQM_PF_NUM_S;
> > > + vf_num = (reg & GL_MDET_TX_PQM_VF_NUM_M) >>
> > > + GL_MDET_TX_PQM_VF_NUM_S;
> > > event = (reg & GL_MDET_TX_PQM_MAL_TYPE_M) >>
> > > GL_MDET_TX_PQM_MAL_TYPE_S;
> > > queue = (reg & GL_MDET_TX_PQM_QNUM_M) >>
> > > GL_MDET_TX_PQM_QNUM_S;
> > >
> > > PMD_DRV_LOG(WARNING, "Malicious Driver Detection
> > > event "
> > > - "%d by PQM on TX queue %d PF# %d",
> > > - event, queue, pf_num);
> > > + "%d by PQM on TX queue %d PF# %d VF#
> > > %d",
> > > + event, queue, pf_num, vf_num);
> > > }
> > >
> > Would this output be misleading in the case where there is no VF involved
> > and the actual MDD error comes from the PF?
>
> I will check, but IIRC, the queue here is an "absolute" queue number
> that should help figure out whether it is the PF or a VF in the case
> vf_num == 0.
>
Yes, that is my understanding too. Maybe in future we can use the queue
number to identify if it's a VF of not. If the PF is the error cause, I
think the VF details should be omitted.
/Bruce
[RFC PATCH v2] dts: skip test cases based on capabilities
The devices under test may not support the capabilities required by
various test cases. Add support for:
1. Marking test suites and test cases with required capabilities,
2. Getting which required capabilities are supported by the device under
test,
3. And then skipping test suites and test cases if their required
capabilities are not supported by the device.
Signed-off-by: Juraj Linkeš
---
dts/framework/remote_session/__init__.py | 2 +-
dts/framework/remote_session/testpmd_shell.py | 44 -
dts/framework/runner.py | 46 --
dts/framework/test_result.py | 90 +++
dts/framework/test_suite.py | 25 ++
dts/framework/testbed_model/sut_node.py | 25 +-
dts/tests/TestSuite_hello_world.py| 4 +-
7 files changed, 204 insertions(+), 32 deletions(-)
diff --git a/dts/framework/remote_session/__init__.py
b/dts/framework/remote_session/__init__.py
index 1910c81c3c..f18a9f2259 100644
--- a/dts/framework/remote_session/__init__.py
+++ b/dts/framework/remote_session/__init__.py
@@ -22,7 +22,7 @@
from .python_shell import PythonShell
from .remote_session import CommandResult, RemoteSession
from .ssh_session import SSHSession
-from .testpmd_shell import TestPmdShell
+from .testpmd_shell import NicCapability, TestPmdShell
def create_remote_session(
diff --git a/dts/framework/remote_session/testpmd_shell.py
b/dts/framework/remote_session/testpmd_shell.py
index cb2ab6bd00..f6783af621 100644
--- a/dts/framework/remote_session/testpmd_shell.py
+++ b/dts/framework/remote_session/testpmd_shell.py
@@ -16,7 +16,9 @@
"""
import time
-from enum import auto
+from collections.abc import MutableSet
+from enum import Enum, auto
+from functools import partial
from pathlib import PurePath
from typing import Callable, ClassVar
@@ -229,3 +231,43 @@ def close(self) -> None:
"""Overrides :meth:`~.interactive_shell.close`."""
self.send_command("quit", "")
return super().close()
+
+def get_capas_rxq(
+self, supported_capabilities: MutableSet, unsupported_capabilities:
MutableSet
+) -> None:
+"""Get all rxq capabilities and divide them into supported and
unsupported.
+
+Args:
+supported_capabilities: A set where capabilities which are
supported will be stored.
+unsupported_capabilities: A set where capabilities which are
+not supported will be stored.
+"""
+self._logger.debug("Getting rxq capabilities.")
+command = "show rxq info 0 0"
+rxq_info = self.send_command(command)
+for line in rxq_info.split("\n"):
+bare_line = line.strip()
+if bare_line.startswith("RX scattered packets:"):
+if bare_line.endswith("on"):
+supported_capabilities.add(NicCapability.scattered_rx)
+else:
+unsupported_capabilities.add(NicCapability.scattered_rx)
+
+
+class NicCapability(Enum):
+"""A mapping between capability names and the associated
:class:`TestPmdShell` methods.
+
+The :class:`TestPmdShell` method executes the command that checks
+whether the capability is supported.
+
+The signature of each :class:`TestPmdShell` method must be::
+
+fn(self, supported_capabilities: MutableSet, unsupported_capabilities:
MutableSet) -> None
+
+The function must execute the testpmd command from which the capability
support can be obtained.
+If multiple capabilities can be obtained from the same testpmd command,
each should be obtained
+in one function. These capabilities should then be added to
`supported_capabilities` or
+`unsupported_capabilities` based on their support.
+"""
+
+scattered_rx = partial(TestPmdShell.get_capas_rxq)
diff --git a/dts/framework/runner.py b/dts/framework/runner.py
index db8e3ba96b..7407ea30b8 100644
--- a/dts/framework/runner.py
+++ b/dts/framework/runner.py
@@ -501,6 +501,12 @@ def _run_test_suites(
The method assumes the build target we're testing has already been
built on the SUT node.
The current build target thus corresponds to the current DPDK build
present on the SUT node.
+Before running any suites, the method determines whether they should
be skipped
+by inspecting any required capabilities the test suite needs and
comparing those
+to capabilities supported by the tested NIC. If all capabilities are
supported,
+the suite is run. If all test cases in a test suite would be skipped,
the whole test suite
+is skipped (the setup and teardown is not run).
+
If a blocking test suite (such as the smoke test suite) fails, the
rest of the test suites
in the current build target won't be executed.
@@ -512,10 +518,30 @@ def _run_test_suites(
test_suites_with_cases: The test suites with test cases to run.
"""
Re: Question about RTE ring
Thans for your response ! Yes, using rte_ring between multiple process. So in this case you’re saying the behavior is undefined ? In my case another process crashed after that. > Le 11 avr. 2024 à 11:08, Konstantin Ananyev a > écrit : > > > > Hi, >> >> As part of a project I have a question about the rte ring. >> I’m using rte ring multi producer/single consumer. >> The producers are several process. >> If one producer is enqueuing an element and crashed (kill pid) in the middle >> of the >> enqueuing, can it compromise the ring ? > > I suppose you are using rte_ring as IPC mechanism between multiple processes, > correct? > In theory - yes, if your producer crashed during enqueue() to the ring, then > yes, the ring might be affected. > If producer already moved prod.head and crashed before updating prod.tail, > then no other producers > will be able to enqueue() into the ring, till you'll do reset() for it. > I expect such situation really rare and hard to reproduce, but in theory it > is possible. > Konstantin
[PATCH v5] net/i40e: support FEC feature
This patch enabled querying Forward Error Correction(FEC) capabilities,
set FEC mode and get current FEC mode functions.
Signed-off-by: Qiming Yang
Signed-off-by: Zhichao Zeng
---
v5: fix some judgments
v4: fix some logic
v3: optimize code details
v2: update NIC feature document
---
doc/guides/nics/features/i40e.ini | 1 +
doc/guides/rel_notes/release_24_07.rst | 4 +
drivers/net/i40e/i40e_ethdev.c | 237 +
3 files changed, 242 insertions(+)
diff --git a/doc/guides/nics/features/i40e.ini
b/doc/guides/nics/features/i40e.ini
index ef7514c44b..4610444ace 100644
--- a/doc/guides/nics/features/i40e.ini
+++ b/doc/guides/nics/features/i40e.ini
@@ -32,6 +32,7 @@ Traffic manager = Y
CRC offload = Y
VLAN offload = Y
QinQ offload = P
+FEC = Y
L3 checksum offload = P
L4 checksum offload = P
Inner L3 checksum= P
diff --git a/doc/guides/rel_notes/release_24_07.rst
b/doc/guides/rel_notes/release_24_07.rst
index a69f24cf99..1e65f70d6c 100644
--- a/doc/guides/rel_notes/release_24_07.rst
+++ b/doc/guides/rel_notes/release_24_07.rst
@@ -55,6 +55,10 @@ New Features
Also, make sure to start the actual text at the margin.
===
+* **Updated Intel i40e driver.**
+
+ * Added support for configuring the Forward Error Correction(FEC) mode,
querying
+ * FEC capabilities and current FEC mode from a device.
Removed Items
-
diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 380ce1a720..bc4a62f64b 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -406,6 +406,10 @@ static void i40e_ethertype_filter_restore(struct i40e_pf
*pf);
static void i40e_tunnel_filter_restore(struct i40e_pf *pf);
static void i40e_filter_restore(struct i40e_pf *pf);
static void i40e_notify_all_vfs_link_status(struct rte_eth_dev *dev);
+static int i40e_fec_get_capability(struct rte_eth_dev *dev,
+ struct rte_eth_fec_capa *speed_fec_capa, unsigned int num);
+static int i40e_fec_get(struct rte_eth_dev *dev, uint32_t *fec_capa);
+static int i40e_fec_set(struct rte_eth_dev *dev, uint32_t fec_capa);
static const char *const valid_keys[] = {
ETH_I40E_FLOATING_VEB_ARG,
@@ -521,6 +525,9 @@ static const struct eth_dev_ops i40e_eth_dev_ops = {
.tm_ops_get = i40e_tm_ops_get,
.tx_done_cleanup = i40e_tx_done_cleanup,
.get_monitor_addr = i40e_get_monitor_addr,
+ .fec_get_capability = i40e_fec_get_capability,
+ .fec_get = i40e_fec_get,
+ .fec_set = i40e_fec_set,
};
/* store statistics names and its offset in stats structure */
@@ -12297,6 +12304,236 @@ i40e_cloud_filter_qinq_create(struct i40e_pf *pf)
return ret;
}
+static int
+i40e_fec_get_capability(struct rte_eth_dev *dev,
+ struct rte_eth_fec_capa *speed_fec_capa, __rte_unused unsigned int num)
+{
+ struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+
+ if (hw->mac.type == I40E_MAC_X722 &&
+ !(hw->flags & I40E_HW_FLAG_X722_FEC_REQUEST_CAPABLE)) {
+ PMD_DRV_LOG(ERR, "Setting FEC encoding not supported by"
+" firmware. Please update the NVM image.\n");
+ return -ENOTSUP;
+ }
+
+ if (hw->device_id == I40E_DEV_ID_25G_SFP28 ||
+ hw->device_id == I40E_DEV_ID_25G_B) {
+ if (speed_fec_capa) {
+ speed_fec_capa->speed = RTE_ETH_SPEED_NUM_25G;
+ speed_fec_capa->capa =
RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC) |
+RTE_ETH_FEC_MODE_CAPA_MASK(BASER) |
+RTE_ETH_FEC_MODE_CAPA_MASK(AUTO) |
+RTE_ETH_FEC_MODE_CAPA_MASK(RS);
+ }
+
+ /* since HW only supports 25G */
+ return 1;
+ } else if (hw->device_id == I40E_DEV_ID_KX_X722) {
+ if (speed_fec_capa) {
+ speed_fec_capa->speed = RTE_ETH_SPEED_NUM_25G;
+ speed_fec_capa->capa = RTE_ETH_FEC_MODE_CAPA_MASK(AUTO)
|
+RTE_ETH_FEC_MODE_CAPA_MASK(RS);
+ }
+ return 1;
+ }
+
+ return -ENOTSUP;
+}
+
+static int
+i40e_fec_get(struct rte_eth_dev *dev, uint32_t *fec_capa)
+{
+ struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+ struct i40e_aq_get_phy_abilities_resp abilities = {0};
+ struct i40e_link_status link_status = {0};
+ uint8_t current_fec_mode = 0, fec_config = 0;
+ bool link_up, enable_lse;
+ int ret = 0;
+
+ enable_lse = dev->data->dev_conf.intr_conf.lsc ? true : false;
+ /* Get link info */
+ ret = i40e_aq_get_link_info(hw,
RE: [PATCH] config/arm: add Ampere AmpereOneX platform
I will resend a new version. Thanks. Best Regards, Yutang Jiang > -Original Message- > From: Wathsala Wathawana Vithanage > Sent: Thursday, April 11, 2024 8:32 AM > To: Wathsala Wathawana Vithanage ; Yutang > Jiang OS ; dev@dpdk.org > Cc: Open Source Submission ; Yutang Jiang > ; Ruifeng Wang > ; nd ; juraj.lin...@pantheon.tech; > nd ; nd > Subject: RE: [PATCH] config/arm: add Ampere AmpereOneX platform > > [EXTERNAL EMAIL NOTICE: This email originated from an external sender. > Please be mindful of safe email handling and proprietary information > protection > practices.] > > > > > > > > Signed-off-by: Yutang Jiang > > > Signed-off-by: Yutang Jiang > > > --- > > Looks like this patch is signed off by Yutang twice with two different emails. > Please remove one and submit again. > > Thank you.
RE: Question about RTE ring
> > Thans for your response ! > > Yes, using rte_ring between multiple process. > > So in this case you’re saying the behavior is undefined ? > In my case another process crashed after that. Without a proper debug session it is really hard to tell what is going on. If the situation is reproducible, I'd suggest to run it with gdb and see. > > > Le 11 avr. 2024 à 11:08, Konstantin Ananyev > > a écrit : > > > > > > > > Hi, > >> > >> As part of a project I have a question about the rte ring. > >> I’m using rte ring multi producer/single consumer. > >> The producers are several process. > >> If one producer is enqueuing an element and crashed (kill pid) in the > >> middle of the > >> enqueuing, can it compromise the ring ? > > > > I suppose you are using rte_ring as IPC mechanism between multiple > > processes, correct? > > In theory - yes, if your producer crashed during enqueue() to the ring, > > then yes, the ring might be affected. > > If producer already moved prod.head and crashed before updating prod.tail, > > then no other producers > > will be able to enqueue() into the ring, till you'll do reset() for it. > > I expect such situation really rare and hard to reproduce, but in theory it > > is possible. > > Konstantin
DPDK Release Status Meeting 2024-04-11
Release status meeting minutes 2024-04-11 = Agenda: * Release Dates * Subtrees * Roadmaps * LTS * Defects * Opens Participants: * AMD * ARM * Debian/Microsoft * Intel * Marvell * Nvidia [No] * Red Hat Release Dates - The following are the current/updated working dates for 24.03: * V1: 29 December 2023 * RC1: 21 February 2024 * RC2: 8 March2024 * RC3: 15 March2024 * Release: 27 March2024 * 24.07 Proposed dates: - Proposal deadline (RFC/v1 patches): 26 April 2024 - API freeze (-rc1): 7 June 2024 - PMD features freeze (-rc2): 21 June 2024 - Builtin applications features freeze (-rc3): 28 June 2024 - Release: 10 July 2023 https://core.dpdk.org/roadmap/#dates Subtrees * next-net * New Napatech PMD. * next-net-intel * Some patches applied from last release. * There will be a number of base code updates in this release. * Test failure: https://mails.dpdk.org/archives/test-report/2024-April/634973.html * next-net-mlx * No update. * next-net-mvl * No update. * next-eventdev * No update. * next-baseband * Merging 1 patchset from previous release. * next-virtio * 2 series posted. * next-crypto * Some patches from previous release. * Patches from Pensando. * main * Some issues with latest OVS and DPDK 23.11. * Looking at changes for Graph library. * Ongoing changes for Windows. * 24.07 Proposed dates: - Proposal deadline (RFC/v1 patches): 26 April 2024 - API freeze (-rc1): 7 June 2024 - PMD features freeze (-rc2): 21 June 2024 - Builtin applications features freeze (-rc3): 28 June 2024 - Release: 10 July 2023 LTS --- Please add acks to confirm validation support for a 3 year LTS window: http://inbox.dpdk.org/dev/20240117161804.223582-1-ktray...@redhat.com/ * 23.11.1 - In progress. * 22.11.5 - In progress. * 21.11.7 - In progress. * 20.11.10 - Will only be updated with CVE and critical fixes. * 19.11.15 - Will only be updated with CVE and critical fixes. * Distros * Debian 12 contains DPDK v22.11 * Ubuntu 24.04-LTS will contain DPDK v23.11 * Ubuntu 23.04 contains DPDK v22.11 Defects --- * Bugzilla links, 'Bugs', added for hosted projects * https://www.dpdk.org/hosted-projects/ DPDK Release Status Meetings The DPDK Release Status Meeting is intended for DPDK Committers to discuss the status of the master tree and sub-trees, and for project managers to track progress or milestone dates. The meeting occurs on every Thursday at 9:30 UTC over Jitsi on https://meet.jit.si/DPDK You don't need an invite to join the meeting but if you want a calendar reminder just send an email to "John McNamara john.mcnam...@intel.com" for the invite.
[PATCH] net/ice: support FEC feature
This patch enable three Forward Error Correction(FEC) related ops
in ice driver. As no speed information can get from HW, this patch
only show FEC capability.
Signed-off-by: Mingjin Ye
---
doc/guides/nics/features/ice.ini | 1 +
doc/guides/nics/ice.rst | 5 +
drivers/net/ice/ice_ethdev.c | 176 +++
3 files changed, 182 insertions(+)
diff --git a/doc/guides/nics/features/ice.ini b/doc/guides/nics/features/ice.ini
index 62869ef0a0..a9be394696 100644
--- a/doc/guides/nics/features/ice.ini
+++ b/doc/guides/nics/features/ice.ini
@@ -9,6 +9,7 @@
[Features]
Speed capabilities = Y
Link speed configuration = Y
+FEC = Y
Link status = Y
Link status event= Y
Rx interrupt = Y
diff --git a/doc/guides/nics/ice.rst b/doc/guides/nics/ice.rst
index 3deeea9e6c..3d7e4ed7f1 100644
--- a/doc/guides/nics/ice.rst
+++ b/doc/guides/nics/ice.rst
@@ -323,6 +323,11 @@ The DCF PMD needs to advertise and acquire DCF capability
which allows DCF to
send AdminQ commands that it would like to execute over to the PF and receive
responses for the same from PF.
+Forward Error Correction (FEC)
+
+
+Supports get/set FEC mode and get FEC capability.
+
Generic Flow Support
diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
index 87385d2649..56d0f2bb28 100644
--- a/drivers/net/ice/ice_ethdev.c
+++ b/drivers/net/ice/ice_ethdev.c
@@ -181,6 +181,10 @@ static int ice_timesync_read_time(struct rte_eth_dev *dev,
static int ice_timesync_write_time(struct rte_eth_dev *dev,
const struct timespec *timestamp);
static int ice_timesync_disable(struct rte_eth_dev *dev);
+static int ice_fec_get_capability(struct rte_eth_dev *dev, struct
rte_eth_fec_capa *speed_fec_capa,
+ unsigned int num);
+static int ice_fec_get(struct rte_eth_dev *dev, uint32_t *fec_capa);
+static int ice_fec_set(struct rte_eth_dev *dev, uint32_t fec_capa);
static const uint32_t *ice_buffer_split_supported_hdr_ptypes_get(struct
rte_eth_dev *dev,
size_t *no_of_elements);
@@ -298,6 +302,9 @@ static const struct eth_dev_ops ice_eth_dev_ops = {
.timesync_write_time = ice_timesync_write_time,
.timesync_disable = ice_timesync_disable,
.tm_ops_get = ice_tm_ops_get,
+ .fec_get_capability = ice_fec_get_capability,
+ .fec_get = ice_fec_get,
+ .fec_set = ice_fec_set,
.buffer_split_supported_hdr_ptypes_get =
ice_buffer_split_supported_hdr_ptypes_get,
};
@@ -6644,6 +6651,175 @@ ice_buffer_split_supported_hdr_ptypes_get(struct
rte_eth_dev *dev __rte_unused,
return ptypes;
}
+static int
+ice_fec_get_capa_num(struct ice_aqc_get_phy_caps_data *pcaps,
+ struct rte_eth_fec_capa *speed_fec_capa)
+{
+ int num = 0;
+
+ if (!pcaps)
+ return ICE_ERR_NO_MEMORY;
+
+ if (pcaps->caps & ICE_AQC_PHY_EN_AUTO_FEC) {
+ if (speed_fec_capa)
+ speed_fec_capa[num].capa =
RTE_ETH_FEC_MODE_CAPA_MASK(AUTO);
+ num++;
+ }
+
+ if (pcaps->link_fec_options & ICE_AQC_PHY_FEC_10G_KR_40G_KR4_EN ||
+ pcaps->link_fec_options & ICE_AQC_PHY_FEC_10G_KR_40G_KR4_REQ ||
+ pcaps->link_fec_options & ICE_AQC_PHY_FEC_25G_KR_CLAUSE74_EN ||
+ pcaps->link_fec_options & ICE_AQC_PHY_FEC_25G_KR_REQ) {
+ if (speed_fec_capa)
+ speed_fec_capa[num].capa =
RTE_ETH_FEC_MODE_CAPA_MASK(BASER);
+ num++;
+ }
+
+ if (pcaps->link_fec_options & ICE_AQC_PHY_FEC_25G_RS_528_REQ ||
+ pcaps->link_fec_options & ICE_AQC_PHY_FEC_25G_RS_544_REQ ||
+ pcaps->link_fec_options & ICE_AQC_PHY_FEC_25G_RS_CLAUSE91_EN) {
+ if (speed_fec_capa)
+ speed_fec_capa[num].capa =
RTE_ETH_FEC_MODE_CAPA_MASK(RS);
+ num++;
+ }
+
+ if (pcaps->link_fec_options == 0) {
+ if (speed_fec_capa)
+ speed_fec_capa[num].capa =
RTE_ETH_FEC_MODE_CAPA_MASK(NOFEC);
+ num++;
+ }
+
+ return num;
+}
+
+static int
+ice_fec_get_capability(struct rte_eth_dev *dev, struct rte_eth_fec_capa
*speed_fec_capa,
+ unsigned int num)
+{
+ struct ice_hw *hw = ICE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+ struct ice_aqc_get_phy_caps_data *pcaps;
+ unsigned int capa_num;
+ int ret;
+
+ pcaps = (struct ice_aqc_get_phy_caps_data *)
+ ice_malloc(hw, sizeof(*pcaps));
+ if (!pcaps)
+ return ICE_ERR_NO_MEMORY;
+
+ ret = ice_aq_get_phy_caps(hw->port_info, false,
ICE_AQC_REPORT_TOPO_CAP_MEDIA,
+ pcaps, NULL);
Re: [PATCH 6/6] dts: add statefulness to TestPmdShell
I overlooked this reply initially. On Wed, Apr 10, 2024 at 1:35 PM Luca Vizzarro wrote: > > On 10/04/2024 08:41, Juraj Linkeš wrote: > >> > >>> @@ -723,7 +731,13 @@ def _start_application(self, get_privileged_command: > >>> Callable[[str], str] | None > >>> if self._app_args.app_params is None: > >>> self._app_args.app_params = TestPmdParameters() > >>> > >>> -self.number_of_ports = len(self._app_args.ports) if > >>> self._app_args.ports is not None else 0 > >>> +assert isinstance(self._app_args.app_params, TestPmdParameters) > >>> + > >> > >> This is tricky because ideally we wouldn't have the assertion here, > >> but I understand why it is needed because Eal parameters have app args > >> which can be any instance of params. I'm not sure of the best way to > >> solve this, because making testpmd parameters extend from eal would > >> break the general scheme that you have in place, and having an > >> extension of EalParameters that enforces this app_args is > >> TestPmdParameters would solve the issues, but might be a little > >> clunky. Is there a way we can use a generic to get python to just > >> understand that, in this case, this will always be TestPmdParameters? > >> If not I might prefer making a private class where this is > >> TestPmdParameters, just because there aren't really any other > >> assertions that we use elsewhere and an unexpected exception from this > >> (even though I don't think that can happen) could cause people some > >> issues. > >> > >> It might be the case that an assertion is the easiest way to deal with > >> it though, what do you think? > >> > > > > We could change the signature (just the type of app_args) of the init > > method - I think we should be able to create a type that's > > EalParameters with .app_params being TestPmdParameters or None. The > > init method would just call super(). > > > > Something like the above is basically necessary with inheritance where > > subclasses are all extensions (not just implementations) of the > > superclass (having differences in API). > > > > I believe this is indeed a tricky one. But, unfortunately, I am not > understanding the solution that is being proposed. To me, it just feels > like using a generic factory like: > >self.sut_node.create_interactive_shell(..) > > is one of the reasons to bring in the majority of these complexities. > I've been thinking about these interactive shell constructors for some time and I think the factory pattern is not well suitable for this. Factories work well with classes with the same API (i.e. implementations of abstract classes that don't add anything extra), but are much less useful when dealing with classes with different behaviors, such as the interactive shells. We see this here, different apps are going to require different args and that alone kinda breaks the factory pattern. I think we'll need to either ditch these factories and instead just have methods that return the proper shell (and the methods would only exist in classes where they belong, e.g. testpmd only makes sense on an SUT). Or we could overload each factory (the support has only been added in 3.11 with @typing.overload, but is also available in typing_extensions, so we would be able to use it with the extra dependency) where different signatures would return different objects. In both cases the caller won't have to import the class and the method signature is going to be clearer. We have this pattern with sut/tg nodes. I decided to move away from the node factory because it didn't add much and in fact the code was only clunkier. The interactive shell is not quite the same, as the shells are not standalone in the same way the nodes are (the shells are tied to nodes). Let me know what you think about all this - both Luca and Jeremy. > What do you mean by creating this new type that combines EalParams and > TestPmdParams? Let me illustrate this on the TestPmdShell __init__() method I had in mind: def __init__(self, interactive_session: SSHClient, logger: DTSLogger, get_privileged_command: Callable[[str], str] | None, app_args: EalTestPmdParams | None = None, timeout: float = SETTINGS.timeout, ) -> None: super().__init__(interactive_session, logger, get_privileged_command) self.state = TestPmdState() Where EalTestPmdParams would be something that enforces that app_args.app_params is of the TestPmdParameters type. But thinking more about this, we're probably better off switching the params composition. Instead of TestPmdParameters being part of EalParameters, we do it the other way around. This way the type of app_args could just be TestPmdParameters and the types should work. Or we pass the args separately, but that would likely require ditching the factories and replacing them with methods (or overloading them). And hopefully the imports won't be impossible to solve. :-)
Re: [PATCH v3] net/netvsc: fix number Tx queues > Rx queues
On 3/19/2024 2:16 PM, Alan Elder wrote:
> The previous code allowed the number of Tx queues to be set higher than
> the number of Rx queues. If a packet was sent on a Tx queue with index
>> = number Rx queues there was a segfault.
> This commit fixes the issue by creating an Rx queue for every Tx queue
> meaning that an event buffer is allocated to handle receiving Tx
> completion messages.
>
> mbuf pool and Rx ring are not allocated for these additional Rx queues
> and RSS configuration ensures that no packets are received on them.
>
> Fixes: 4e9c73e96e83 ("net/netvsc: add Hyper-V network device")
> Cc: sthem...@microsoft.com
> Cc: sta...@dpdk.org
>
> Signed-off-by: Alan Elder
>
Hi Alan,
What is the root cause of the crash, is it in driver scope or application?
Re: [PATCH 6/6] dts: add statefulness to TestPmdShell
On 11/04/2024 11:30, Juraj Linkeš wrote: I've been thinking about these interactive shell constructors for some time and I think the factory pattern is not well suitable for this. Factories work well with classes with the same API (i.e. implementations of abstract classes that don't add anything extra), but are much less useful when dealing with classes with different behaviors, such as the interactive shells. We see this here, different apps are going to require different args and that alone kinda breaks the factory pattern. I think we'll need to either ditch these factories and instead just have methods that return the proper shell (and the methods would only exist in classes where they belong, e.g. testpmd only makes sense on an SUT). Or we could overload each factory (the support has only been added in 3.11 with @typing.overload, but is also available in typing_extensions, so we would be able to use it with the extra dependency) where different signatures would return different objects. In both cases the caller won't have to import the class and the method signature is going to be clearer. We have this pattern with sut/tg nodes. I decided to move away from the node factory because it didn't add much and in fact the code was only clunkier. The interactive shell is not quite the same, as the shells are not standalone in the same way the nodes are (the shells are tied to nodes). Let me know what you think about all this - both Luca and Jeremy. When writing this series, I went down the path of creating a `create_testpmd_shell` method at some point as a solution to these problems. Realising after that it may be too big of a change, and possibly best left to a discussion exactly like this one. Generics used at this level may be a bit too much, especially for Python, as support is not *that* great. I am of the opinion that having a dedicated wrapper is easier for the developer and the user. Generics are not needed to this level anyways, as we have a limited selection of shells that are actually going to be used. We can also swap the wrapping process to simplify things, instead of: shell = self.sut_node.create_interactive_shell(TestPmdShell, ..) do: shell = TestPmdShell(self.sut_node, ..) Let the Shell class ingest the node, and not the other way round. The current approach appears to me to be top-down instead of bottom-up. We take the most abstracted part and we work our way down. But all we want is concreteness to the end user (developer). Let me illustrate this on the TestPmdShell __init__() method I had in mind: def __init__(self, interactive_session: SSHClient, logger: DTSLogger, get_privileged_command: Callable[[str], str] | None, app_args: EalTestPmdParams | None = None, timeout: float = SETTINGS.timeout, ) -> None: super().__init__(interactive_session, logger, get_privileged_command) self.state = TestPmdState() Where EalTestPmdParams would be something that enforces that app_args.app_params is of the TestPmdParameters type. But thinking more about this, we're probably better off switching the params composition. Instead of TestPmdParameters being part of EalParameters, we do it the other way around. This way the type of app_args could just be TestPmdParameters and the types should work. Or we pass the args separately, but that would likely require ditching the factories and replacing them with methods (or overloading them). And hopefully the imports won't be impossible to solve. :-) It is what I feared, and I think it may become even more convoluted. As you said, ditching the factories will simplify things and make it more straightforward. So, we wouldn't find ourselves in problems like these. I don't have a strong preference in approach between: * overloading node methods * dedicated node methods * let the shells ingest nodes instead But if I were to give priority, I'd take it from last to first. Letting shells ingest nodes will decouple the situation adding an extra step of simplification. I may not see the full picture though. The two are reasonable but, having a dedicated node method will stop the requirement to import the shell we need, and it's pretty much equivalent... but overloading also is very new to Python, so I may prefer to stick to more established. Letting TestPmdParams take EalParams, instead of the other way around, would naturally follow the bottom-up approach too. Allowing Params to arbitrarily append string arguments – as proposed, would also allow users to use a plain (EalParams + string). So sounds like a good approach overall.
Re: [PATCH] ethdev: fix strict aliasing lead to link cannot be up
Hi Morten,
On 2024/4/11 14:58, Morten Brørup wrote:
>> From: Chengwen Feng [mailto:fengcheng...@huawei.com]
>> Sent: Thursday, 11 April 2024 05.08
>>
>> Fix a problem introduced by a compiler upgrade (from gcc10 to gcc12.3),
>> which will lead the hns3 NIC can't link up. The root cause is strict
>> aliasing violation in rte_eth_linkstatus_set() with hns3 driver, see
>> [1] for more details.
>>
>> This commit use union to avoid such aliasing violation.
>>
>> [1] Strict aliasing problem with rte_eth_linkstatus_set()
>> https://marc.info/?l=dpdk-dev&m=171274148514777&w=3
>>
>> Cc: sta...@dpdk.org
>>
>> Signed-off-by: Chengwen Feng
>> Signed-off-by: Dengdui Huang
>> ---
>
> The patch mixes atomic and non-atomic access.
> This is not new for DPDK, which used to rely on compiler built-in atomics.
>
> I'm not sure it needs to be changed, but my suggestion is inline below.
> I don't think it makes any practical different for 64 bit arch, but it might
> for 32 bit arch.
>
>> lib/ethdev/ethdev_driver.h | 23 +++
>> lib/ethdev/rte_ethdev.h| 16 ++--
>> 2 files changed, 17 insertions(+), 22 deletions(-)
>>
>> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
>> index 0dbf2dd6a2..9d831d5c84 100644
>> --- a/lib/ethdev/ethdev_driver.h
>> +++ b/lib/ethdev/ethdev_driver.h
>> @@ -1674,18 +1674,13 @@ static inline int
>> rte_eth_linkstatus_set(struct rte_eth_dev *dev,
>> const struct rte_eth_link *new_link)
>> {
>> -RTE_ATOMIC(uint64_t) *dev_link = (uint64_t __rte_atomic *)&(dev-
>>> data->dev_link);
>> -union {
>> -uint64_t val64;
>> -struct rte_eth_link link;
>> -} orig;
>> -
>> -RTE_BUILD_BUG_ON(sizeof(*new_link) != sizeof(uint64_t));
>> +struct rte_eth_link old_link;
>>
>> -orig.val64 = rte_atomic_exchange_explicit(dev_link, *(const
>> uint64_t *)new_link,
>> -rte_memory_order_seq_cst);
>> +old_link.val64 = rte_atomic_exchange_explicit(&dev->data-
>>> dev_link.val64,
>
> old_link.val64 should be written using:
> rte_atomic_store_explicit(&old_link.val64, ..., rte_memory_order_seq_cst)
I'm afraid I don't agree this, the &dev->data->dev_link.val64 should use atomic
not the stack variable old_link.
>
>> + new_link->val64,
>
> new_link->val64 should be read using:
> rte_atomic_load_explicit(&new_link->val64, rte_memory_order_seq_cst)
The same reason with above.
>
>> + rte_memory_order_seq_cst);
>
>>
>> -return (orig.link.link_status == new_link->link_status) ? -1 : 0;
>> +return (old_link.link_status == new_link->link_status) ? -1 : 0;
>> }
>>
>> /**
>> @@ -1701,12 +1696,8 @@ static inline void
>> rte_eth_linkstatus_get(const struct rte_eth_dev *dev,
>> struct rte_eth_link *link)
>> {
>> -RTE_ATOMIC(uint64_t) *src = (uint64_t __rte_atomic *)&(dev->data-
>>> dev_link);
>> -uint64_t *dst = (uint64_t *)link;
>> -
>> -RTE_BUILD_BUG_ON(sizeof(*link) != sizeof(uint64_t));
>> -
>> -*dst = rte_atomic_load_explicit(src, rte_memory_order_seq_cst);
>> +link->val64 = rte_atomic_load_explicit(&dev->data->dev_link.val64,
>
> link->val64 should be written using:
> rte_atomic_store_explicit(&link->val64, ..., rte_memory_order_seq_cst)
The same reason with above, the &dev->data->dev_link.val64 should use atomic
not the stack variable link.
>
>> + rte_memory_order_seq_cst);
>> }
>>
>> /**
>> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
>> index 147257d6a2..0b5d3d2318 100644
>> --- a/lib/ethdev/rte_ethdev.h
>> +++ b/lib/ethdev/rte_ethdev.h
>> @@ -332,12 +332,16 @@ struct rte_eth_stats {
>> /**
>> * A structure used to retrieve link-level information of an Ethernet
>> port.
>> */
>> -__extension__
>> -struct __rte_aligned(8) rte_eth_link { /**< aligned for atomic64
>> read/write */
>> -uint32_t link_speed;/**< RTE_ETH_SPEED_NUM_ */
>> -uint16_t link_duplex : 1; /**< RTE_ETH_LINK_[HALF/FULL]_DUPLEX
>> */
>> -uint16_t link_autoneg : 1; /**< RTE_ETH_LINK_[AUTONEG/FIXED] */
>> -uint16_t link_status : 1; /**< RTE_ETH_LINK_[DOWN/UP] */
>> +struct rte_eth_link {
>> +union {
>> +uint64_t val64; /**< used for atomic64 read/write */
>
> The type of val64 should be:
> RTE_ATOMIC(uint64_t)
ack
Plus: yes, this patch mixes atomic and non-atomic access, but the main reason
is that we want to simplify the implementation. If we want to separate it
clearly,
maybe we should defined as this:
struct rte_eth_link {
union {
RTE_ATOMIC(uint64_t) atomic64; /**< used for atomic64 read/write */
struct {
uint64_t val64;
};
struct {
uint32_t link_speed;/**< RTE_ETH_SPEED_NUM_ */
uint16_t link_duplex : 1; /**
[PATCH v2] ethdev: fix strict aliasing lead to link cannot be up
Fix a problem introduced by a compiler upgrade (from gcc10 to gcc12.3),
which will lead the hns3 NIC can't link up. The root cause is strict
aliasing violation in rte_eth_linkstatus_set() with hns3 driver, see
[1] for more details.
This commit use union to avoid such aliasing violation.
[1] Strict aliasing problem with rte_eth_linkstatus_set()
https://marc.info/?l=dpdk-dev&m=171274148514777&w=3
Cc: sta...@dpdk.org
Signed-off-by: Chengwen Feng
Signed-off-by: Dengdui Huang
---
v2: add RTE_ATOMIC(uint64_t) wrap which address Morten's comment.
---
lib/ethdev/ethdev_driver.h | 23 +++
lib/ethdev/rte_ethdev.h| 16 ++--
2 files changed, 17 insertions(+), 22 deletions(-)
diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
index 0dbf2dd6a2..9d831d5c84 100644
--- a/lib/ethdev/ethdev_driver.h
+++ b/lib/ethdev/ethdev_driver.h
@@ -1674,18 +1674,13 @@ static inline int
rte_eth_linkstatus_set(struct rte_eth_dev *dev,
const struct rte_eth_link *new_link)
{
- RTE_ATOMIC(uint64_t) *dev_link = (uint64_t __rte_atomic
*)&(dev->data->dev_link);
- union {
- uint64_t val64;
- struct rte_eth_link link;
- } orig;
-
- RTE_BUILD_BUG_ON(sizeof(*new_link) != sizeof(uint64_t));
+ struct rte_eth_link old_link;
- orig.val64 = rte_atomic_exchange_explicit(dev_link, *(const uint64_t
*)new_link,
- rte_memory_order_seq_cst);
+ old_link.val64 =
rte_atomic_exchange_explicit(&dev->data->dev_link.val64,
+ new_link->val64,
+ rte_memory_order_seq_cst);
- return (orig.link.link_status == new_link->link_status) ? -1 : 0;
+ return (old_link.link_status == new_link->link_status) ? -1 : 0;
}
/**
@@ -1701,12 +1696,8 @@ static inline void
rte_eth_linkstatus_get(const struct rte_eth_dev *dev,
struct rte_eth_link *link)
{
- RTE_ATOMIC(uint64_t) *src = (uint64_t __rte_atomic
*)&(dev->data->dev_link);
- uint64_t *dst = (uint64_t *)link;
-
- RTE_BUILD_BUG_ON(sizeof(*link) != sizeof(uint64_t));
-
- *dst = rte_atomic_load_explicit(src, rte_memory_order_seq_cst);
+ link->val64 = rte_atomic_load_explicit(&dev->data->dev_link.val64,
+ rte_memory_order_seq_cst);
}
/**
diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
index 147257d6a2..ccf43e468a 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -332,12 +332,16 @@ struct rte_eth_stats {
/**
* A structure used to retrieve link-level information of an Ethernet port.
*/
-__extension__
-struct __rte_aligned(8) rte_eth_link { /**< aligned for atomic64 read/write */
- uint32_t link_speed;/**< RTE_ETH_SPEED_NUM_ */
- uint16_t link_duplex : 1; /**< RTE_ETH_LINK_[HALF/FULL]_DUPLEX */
- uint16_t link_autoneg : 1; /**< RTE_ETH_LINK_[AUTONEG/FIXED] */
- uint16_t link_status : 1; /**< RTE_ETH_LINK_[DOWN/UP] */
+struct rte_eth_link {
+ union {
+ RTE_ATOMIC(uint64_t) val64; /**< used for atomic64 read/write */
+ struct {
+ uint32_t link_speed;/**< RTE_ETH_SPEED_NUM_ */
+ uint16_t link_duplex : 1; /**<
RTE_ETH_LINK_[HALF/FULL]_DUPLEX */
+ uint16_t link_autoneg : 1; /**<
RTE_ETH_LINK_[AUTONEG/FIXED] */
+ uint16_t link_status : 1; /**< RTE_ETH_LINK_[DOWN/UP]
*/
+ };
+ };
};
/**@{@name Link negotiation
--
2.17.1
[PATCH v3] ethdev: fix strict aliasing lead to link cannot be up
Fix a problem introduced by a compiler upgrade (from gcc10 to gcc12.3),
which will lead the hns3 NIC can't link up. The root cause is strict
aliasing violation in rte_eth_linkstatus_set() with hns3 driver, see
[1] for more details.
This commit use union to avoid such aliasing violation.
[1] Strict aliasing problem with rte_eth_linkstatus_set()
https://marc.info/?l=dpdk-dev&m=171274148514777&w=3
Cc: sta...@dpdk.org
Signed-off-by: Chengwen Feng
Signed-off-by: Dengdui Huang
---
v3: fix checkpatch warning "missing --in-reply-to".
v2: add RTE_ATOMIC(uint64_t) wrap which address Morten's comment.
---
lib/ethdev/ethdev_driver.h | 23 +++
lib/ethdev/rte_ethdev.h| 16 ++--
2 files changed, 17 insertions(+), 22 deletions(-)
diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
index 0dbf2dd6a2..9d831d5c84 100644
--- a/lib/ethdev/ethdev_driver.h
+++ b/lib/ethdev/ethdev_driver.h
@@ -1674,18 +1674,13 @@ static inline int
rte_eth_linkstatus_set(struct rte_eth_dev *dev,
const struct rte_eth_link *new_link)
{
- RTE_ATOMIC(uint64_t) *dev_link = (uint64_t __rte_atomic
*)&(dev->data->dev_link);
- union {
- uint64_t val64;
- struct rte_eth_link link;
- } orig;
-
- RTE_BUILD_BUG_ON(sizeof(*new_link) != sizeof(uint64_t));
+ struct rte_eth_link old_link;
- orig.val64 = rte_atomic_exchange_explicit(dev_link, *(const uint64_t
*)new_link,
- rte_memory_order_seq_cst);
+ old_link.val64 =
rte_atomic_exchange_explicit(&dev->data->dev_link.val64,
+ new_link->val64,
+ rte_memory_order_seq_cst);
- return (orig.link.link_status == new_link->link_status) ? -1 : 0;
+ return (old_link.link_status == new_link->link_status) ? -1 : 0;
}
/**
@@ -1701,12 +1696,8 @@ static inline void
rte_eth_linkstatus_get(const struct rte_eth_dev *dev,
struct rte_eth_link *link)
{
- RTE_ATOMIC(uint64_t) *src = (uint64_t __rte_atomic
*)&(dev->data->dev_link);
- uint64_t *dst = (uint64_t *)link;
-
- RTE_BUILD_BUG_ON(sizeof(*link) != sizeof(uint64_t));
-
- *dst = rte_atomic_load_explicit(src, rte_memory_order_seq_cst);
+ link->val64 = rte_atomic_load_explicit(&dev->data->dev_link.val64,
+ rte_memory_order_seq_cst);
}
/**
diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
index 147257d6a2..ccf43e468a 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -332,12 +332,16 @@ struct rte_eth_stats {
/**
* A structure used to retrieve link-level information of an Ethernet port.
*/
-__extension__
-struct __rte_aligned(8) rte_eth_link { /**< aligned for atomic64 read/write */
- uint32_t link_speed;/**< RTE_ETH_SPEED_NUM_ */
- uint16_t link_duplex : 1; /**< RTE_ETH_LINK_[HALF/FULL]_DUPLEX */
- uint16_t link_autoneg : 1; /**< RTE_ETH_LINK_[AUTONEG/FIXED] */
- uint16_t link_status : 1; /**< RTE_ETH_LINK_[DOWN/UP] */
+struct rte_eth_link {
+ union {
+ RTE_ATOMIC(uint64_t) val64; /**< used for atomic64 read/write */
+ struct {
+ uint32_t link_speed;/**< RTE_ETH_SPEED_NUM_ */
+ uint16_t link_duplex : 1; /**<
RTE_ETH_LINK_[HALF/FULL]_DUPLEX */
+ uint16_t link_autoneg : 1; /**<
RTE_ETH_LINK_[AUTONEG/FIXED] */
+ uint16_t link_status : 1; /**< RTE_ETH_LINK_[DOWN/UP]
*/
+ };
+ };
};
/**@{@name Link negotiation
--
2.17.1
Re: [PATCH 6/6] dts: add statefulness to TestPmdShell
On Thu, Apr 11, 2024 at 1:47 PM Luca Vizzarro wrote: > > On 11/04/2024 11:30, Juraj Linkeš wrote: > > I've been thinking about these interactive shell constructors for some > > time and I think the factory pattern is not well suitable for this. > > Factories work well with classes with the same API (i.e. > > implementations of abstract classes that don't add anything extra), > > but are much less useful when dealing with classes with different > > behaviors, such as the interactive shells. We see this here, different > > apps are going to require different args and that alone kinda breaks > > the factory pattern. I think we'll need to either ditch these > > factories and instead just have methods that return the proper shell > > (and the methods would only exist in classes where they belong, e.g. > > testpmd only makes sense on an SUT). Or we could overload each factory > > (the support has only been added in 3.11 with @typing.overload, but is > > also available in typing_extensions, so we would be able to use it > > with the extra dependency) where different signatures would return > > different objects. In both cases the caller won't have to import the > > class and the method signature is going to be clearer. > > > > We have this pattern with sut/tg nodes. I decided to move away from > > the node factory because it didn't add much and in fact the code was > > only clunkier. The interactive shell is not quite the same, as the > > shells are not standalone in the same way the nodes are (the shells > > are tied to nodes). Let me know what you think about all this - both > > Luca and Jeremy. > > When writing this series, I went down the path of creating a > `create_testpmd_shell` method at some point as a solution to these > problems. Realising after that it may be too big of a change, and > possibly best left to a discussion exactly like this one. > The changes we discuss below don't seem that big. What do you think, do we just add another patch to the series? > Generics used at this level may be a bit too much, especially for > Python, as support is not *that* great. I am of the opinion that having > a dedicated wrapper is easier for the developer and the user. Generics > are not needed to this level anyways, as we have a limited selection of > shells that are actually going to be used. > > We can also swap the wrapping process to simplify things, instead of: > >shell = self.sut_node.create_interactive_shell(TestPmdShell, ..) > > do: > >shell = TestPmdShell(self.sut_node, ..) > > Let the Shell class ingest the node, and not the other way round. > I thought about this a bit as well, it's a good approach. The current design is top-down, as you say, in that "I have a node and I do things with the node, including starting testpmd on the node". But it could also be "I have a node, but I also have other non-node resources at my disposal and it's up to me how I utilize those". If we can make the imports work then this is likely the best option. > The current approach appears to me to be top-down instead of bottom-up. > We take the most abstracted part and we work our way down. But all we > want is concreteness to the end user (developer). > > > Let me illustrate this on the TestPmdShell __init__() method I had in mind: > > > > def __init__(self, interactive_session: SSHClient, > > logger: DTSLogger, > > get_privileged_command: Callable[[str], str] | None, > > app_args: EalTestPmdParams | None = None, > > timeout: float = SETTINGS.timeout, > > ) -> None: > > super().__init__(interactive_session, logger, get_privileged_command) > > self.state = TestPmdState() > > > > Where EalTestPmdParams would be something that enforces that > > app_args.app_params is of the TestPmdParameters type. > > > > But thinking more about this, we're probably better off switching the > > params composition. Instead of TestPmdParameters being part of > > EalParameters, we do it the other way around. This way the type of > > app_args could just be TestPmdParameters and the types should work. > > Or we pass the args separately, but that would likely require ditching > > the factories and replacing them with methods (or overloading them). > > > > And hopefully the imports won't be impossible to solve. :-) > > It is what I feared, and I think it may become even more convoluted. As > you said, ditching the factories will simplify things and make it more > straightforward. So, we wouldn't find ourselves in problems like these. > > I don't have a strong preference in approach between: > * overloading node methods > * dedicated node methods > * let the shells ingest nodes instead > > But if I were to give priority, I'd take it from last to first. Letting > shells ingest nodes will decouple the situation adding an extra step of > simplification. +1 for simplification. > I may not see the full picture though. The two are > reasonable but, having a dedicated node method will stop the
[DPDK/DTS Bug 1414] DTS: Remove the POC OS UDP test case
https://bugs.dpdk.org/show_bug.cgi?id=1414 Bug ID: 1414 Summary: DTS: Remove the POC OS UDP test case Product: DPDK Version: unspecified Hardware: All OS: All Status: UNCONFIRMED Severity: normal Priority: Normal Component: DTS Assignee: dev@dpdk.org Reporter: juraj.lin...@pantheon.tech CC: juraj.lin...@pantheon.tech, pr...@iol.unh.edu Target Milestone: --- The test case was meant to showcase some of the Scapy traffic generator features. It doesn't use testpmd or anything else from DPDK, so it should be removed as soon as possible, likely when we have a couple (or maybe just one) of real testpmd test suites. -- You are receiving this mail because: You are the assignee for the bug.
RE: [PATCH v3] ethdev: fix strict aliasing lead to link cannot be up
> From: Chengwen Feng [mailto:fengcheng...@huawei.com]
> Sent: Thursday, 11 April 2024 14.04
>
> Fix a problem introduced by a compiler upgrade (from gcc10 to gcc12.3),
> which will lead the hns3 NIC can't link up. The root cause is strict
> aliasing violation in rte_eth_linkstatus_set() with hns3 driver, see
> [1] for more details.
>
> This commit use union to avoid such aliasing violation.
>
> [1] Strict aliasing problem with rte_eth_linkstatus_set()
> https://marc.info/?l=dpdk-dev&m=171274148514777&w=3
>
> Cc: sta...@dpdk.org
>
> Signed-off-by: Chengwen Feng
> Signed-off-by: Dengdui Huang
>
> ---
> v3: fix checkpatch warning "missing --in-reply-to".
> v2: add RTE_ATOMIC(uint64_t) wrap which address Morten's comment.
>
> ---
> lib/ethdev/ethdev_driver.h | 23 +++
> lib/ethdev/rte_ethdev.h| 16 ++--
> 2 files changed, 17 insertions(+), 22 deletions(-)
>
> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
> index 0dbf2dd6a2..9d831d5c84 100644
> --- a/lib/ethdev/ethdev_driver.h
> +++ b/lib/ethdev/ethdev_driver.h
> @@ -1674,18 +1674,13 @@ static inline int
> rte_eth_linkstatus_set(struct rte_eth_dev *dev,
> const struct rte_eth_link *new_link)
> {
> - RTE_ATOMIC(uint64_t) *dev_link = (uint64_t __rte_atomic *)&(dev-
> >data->dev_link);
> - union {
> - uint64_t val64;
> - struct rte_eth_link link;
> - } orig;
> -
> - RTE_BUILD_BUG_ON(sizeof(*new_link) != sizeof(uint64_t));
> + struct rte_eth_link old_link;
>
> - orig.val64 = rte_atomic_exchange_explicit(dev_link, *(const
> uint64_t *)new_link,
> - rte_memory_order_seq_cst);
> + old_link.val64 = rte_atomic_exchange_explicit(&dev->data-
> >dev_link.val64,
You are right; old_link has local scope and is on the stack, so atomic store is
not required.
And since rte_eth_linkstatus_set() is an internal function called from the
driver only, it is probably safe to assume that *new_link is on the caller's
stack and doesn't change while being accessed by this function.
I guess that new_link is passed by reference for performance and
future-proofing reasons; it could have been passed by value instead. If it was
passed by value, atomic access would certainly not be required.
In other words: You are right here too; new_link does not require atomic load.
> + new_link->val64,
> + rte_memory_order_seq_cst);
>
> - return (orig.link.link_status == new_link->link_status) ? -1 : 0;
> + return (old_link.link_status == new_link->link_status) ? -1 : 0;
> }
>
> /**
> @@ -1701,12 +1696,8 @@ static inline void
> rte_eth_linkstatus_get(const struct rte_eth_dev *dev,
> struct rte_eth_link *link)
> {
> - RTE_ATOMIC(uint64_t) *src = (uint64_t __rte_atomic *)&(dev->data-
> >dev_link);
> - uint64_t *dst = (uint64_t *)link;
> -
> - RTE_BUILD_BUG_ON(sizeof(*link) != sizeof(uint64_t));
> -
> - *dst = rte_atomic_load_explicit(src, rte_memory_order_seq_cst);
> + link->val64 = rte_atomic_load_explicit(&dev->data->dev_link.val64,
> +rte_memory_order_seq_cst);
It is not safe to assume that the link pointer points to local memory on the
caller's stack.
The link pointer might point to a shared memory area, used by multiple
threads/processes, so it needs to be stored atomically using
rte_atomic_store_explicit(&link->val64, ..., rte_memory_order_seq_cst).
> }
>
> /**
> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
> index 147257d6a2..ccf43e468a 100644
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -332,12 +332,16 @@ struct rte_eth_stats {
> /**
> * A structure used to retrieve link-level information of an Ethernet
> port.
> */
> -__extension__
> -struct __rte_aligned(8) rte_eth_link { /**< aligned for atomic64
> read/write */
> - uint32_t link_speed;/**< RTE_ETH_SPEED_NUM_ */
> - uint16_t link_duplex : 1; /**< RTE_ETH_LINK_[HALF/FULL]_DUPLEX
> */
> - uint16_t link_autoneg : 1; /**< RTE_ETH_LINK_[AUTONEG/FIXED] */
> - uint16_t link_status : 1; /**< RTE_ETH_LINK_[DOWN/UP] */
> +struct rte_eth_link {
> + union {
> + RTE_ATOMIC(uint64_t) val64; /**< used for atomic64
> read/write */
> + struct {
> + uint32_t link_speed;/**< RTE_ETH_SPEED_NUM_
> */
> + uint16_t link_duplex : 1; /**<
> RTE_ETH_LINK_[HALF/FULL]_DUPLEX */
> + uint16_t link_autoneg : 1; /**<
> RTE_ETH_LINK_[AUTONEG/FIXED] */
> + uint16_t link_status : 1; /**<
> RTE_ETH_LINK_[DOWN/UP] */
> + };
> + };
> };
>
> /**@{@name Link negotiation
> --
> 2.17.1
RE: [PATCH] ethdev: fix strict aliasing lead to link cannot be up
> From: fengchengwen [mailto:fengcheng...@huawei.com]
> Sent: Thursday, 11 April 2024 13.58
[...]
> Plus: yes, this patch mixes atomic and non-atomic access, but the main
> reason is that we want to simplify the implementation.
Yes, your design in patch v3 follows the current standard design pattern for
atomics in DPDK.
I agree with you on this.
Thank you for describing the alternative, though.
> If we want to separate it clearly,
> maybe we should defined as this:
> struct rte_eth_link {
> union {
> RTE_ATOMIC(uint64_t) atomic64; /**< used for atomic64
> read/write */
> struct {
> uint64_t val64;
> };
> struct {
> uint32_t link_speed; /**< RTE_ETH_SPEED_NUM_ */
> uint16_t link_duplex : 1; /**<
> RTE_ETH_LINK_[HALF/FULL]_DUPLEX */
> uint16_t link_autoneg : 1; /**<
> RTE_ETH_LINK_[AUTONEG/FIXED] */
> uint16_t link_status : 1; /**< RTE_ETH_LINK_[DOWN/UP]
> */
> };
> };
> };
PS: More review comments provided in reply to the v3 patch.
Re: [EXTERNAL] [PATCH v7 2/4] hash: optimize compare signature for NEON
On 3/20/24 07:37, Pavan Nikhilesh Bhagavatula wrote:
Upon a successful comparison, NEON sets all the bits in the lane to 1
We can skip shifting by simply masking with specific masks.
Signed-off-by: Yoan Picchi
Reviewed-by: Ruifeng Wang
Reviewed-by: Nathan Brown
---
lib/hash/arch/arm/compare_signatures.h | 24 +++-
1 file changed, 11 insertions(+), 13 deletions(-)
diff --git a/lib/hash/arch/arm/compare_signatures.h
b/lib/hash/arch/arm/compare_signatures.h
index 1af6ba8190..b5a457f936 100644
--- a/lib/hash/arch/arm/compare_signatures.h
+++ b/lib/hash/arch/arm/compare_signatures.h
@@ -30,23 +30,21 @@ compare_signatures_dense(uint16_t
*hitmask_buffer,
switch (sig_cmp_fn) {
#if RTE_HASH_BUCKET_ENTRIES <= 8
case RTE_HASH_COMPARE_NEON: {
- uint16x8_t vmat, vsig, x;
- int16x8_t shift = {0, 1, 2, 3, 4, 5, 6, 7};
- uint16_t low, high;
+ uint16x8_t vmat, hit1, hit2;
+ const uint16x8_t mask = {0x1, 0x2, 0x4, 0x8, 0x10, 0x20,
0x40, 0x80};
+ const uint16x8_t vsig = vld1q_dup_u16((uint16_t const
*)&sig);
- vsig = vld1q_dup_u16((uint16_t const *)&sig);
/* Compare all signatures in the primary bucket */
- vmat = vceqq_u16(vsig,
- vld1q_u16((uint16_t const *)prim_bucket_sigs));
- x = vshlq_u16(vandq_u16(vmat, vdupq_n_u16(0x0001)),
shift);
- low = (uint16_t)(vaddvq_u16(x));
+ vmat = vceqq_u16(vsig, vld1q_u16(prim_bucket_sigs));
+ hit1 = vandq_u16(vmat, mask);
+
/* Compare all signatures in the secondary bucket */
- vmat = vceqq_u16(vsig,
- vld1q_u16((uint16_t const *)sec_bucket_sigs));
- x = vshlq_u16(vandq_u16(vmat, vdupq_n_u16(0x0001)),
shift);
- high = (uint16_t)(vaddvq_u16(x));
- *hitmask_buffer = low | high << RTE_HASH_BUCKET_ENTRIES;
+ vmat = vceqq_u16(vsig, vld1q_u16(sec_bucket_sigs));
+ hit2 = vandq_u16(vmat, mask);
+ hit2 = vshlq_n_u16(hit2, RTE_HASH_BUCKET_ENTRIES);
+ hit2 = vorrq_u16(hit1, hit2);
+ *hitmask_buffer = vaddvq_u16(hit2);
Since vaddv is expensive could you convert it to vshrn?
https://community.arm.com/arm-community-blogs/b/infrastructure-solutions-blog/posts/porting-x86-vector-bitmask-optimizations-to-arm-neon
https://github.com/DPDK/dpdk/blob/main/examples/l3fwd/l3fwd_neon.h#L226
Thank you for those links, it was a good read.
Unfortunatly I don't think it is a good use case here. A decent part of
the speedup I get is by using a dense hitmask: ie every bit count with
no padding. Using the vshrn would have 4 bits of padding, and stripping
them would be more expensive than using a regular reduce.
}
break;
#endif
--
2.25.1
Re: [PATCH 6/6] dts: add statefulness to TestPmdShell
On 11/04/2024 13:13, Juraj Linkeš wrote: The changes we discuss below don't seem that big. What do you think, do we just add another patch to the series? Sure thing, I can take this and add it to v2. I thought about this a bit as well, it's a good approach. The current design is top-down, as you say, in that "I have a node and I do things with the node, including starting testpmd on the node". But it could also be "I have a node, but I also have other non-node resources at my disposal and it's up to me how I utilize those". If we can make the imports work then this is likely the best option. +1 for simplification. > Let's try shells ingesting nodes if the imports work out then. If not, we can fall back to dedicated node methods. Sounds good!
Re: [PATCH] net/gve: add IPv4 checksum offloading capability
On 3/14/2024 12:18 PM, Rushil Gupta wrote: > Gvnic's DQO format allows offloading IPv4 checksum. > Made changes to Tx and Rx path to translate DPDK flags > to descriptor for offloading (and vice-versa). > Add ptype adminq support to only add this flags for > supported L3/L4 packet-types. > > Signed-off-by: Rushil Gupta > Reviewed-by: Joshua Washington > Applied to dpdk-next-net/main, thanks.
Re: [PATCH] ethdev: fix strict aliasing lead to link cannot be up
On Thu, 11 Apr 2024 03:07:49 + Chengwen Feng wrote: > Fix a problem introduced by a compiler upgrade (from gcc10 to gcc12.3), > which will lead the hns3 NIC can't link up. The root cause is strict > aliasing violation in rte_eth_linkstatus_set() with hns3 driver, see > [1] for more details. > > This commit use union to avoid such aliasing violation. > > [1] Strict aliasing problem with rte_eth_linkstatus_set() > https://marc.info/?l=dpdk-dev&m=171274148514777&w=3 > > Cc: sta...@dpdk.org > > Signed-off-by: Chengwen Feng > Signed-off-by: Dengdui Huang > --- The patch to use union is correct. Examining the link status fuller raises a couple of pre-existing issues. 1. Why is this an inline function, there is no way this is in the fast path of any driver or application? 2. Why is it marked sequential consistent and not relaxed? How could there be a visible relationship between link status and other variables. Drivers would not be using the link status state as internal variable.
[DPDK/ethdev Bug 1415] Calling rte_eth_bond_8023ad_dedicated_queues_enable() leads to exhaustion of the LACP packet pool
https://bugs.dpdk.org/show_bug.cgi?id=1415 Bug ID: 1415 Summary: Calling rte_eth_bond_8023ad_dedicated_queues_enable() leads to exhaustion of the LACP packet pool Product: DPDK Version: 23.11 Hardware: All OS: All Status: UNCONFIRMED Severity: normal Priority: Normal Component: ethdev Assignee: dev@dpdk.org Reporter: mic...@digirati.com.br Target Milestone: --- When dedicated queues are enabled on a bond interface by calling rte_eth_bond_8023ad_dedicated_queues_enable(), DPDK eventually starts repeatedly loggin "tx_machine(580) - Failed to allocate LACP packet from pool". According to the code of tx_machine(), this log entry means that the mbuf pool created to send LACP packets (i.e. port->mbuf_pool) is exhausted. The problem occurs about 10 minutes after my application (i.e. https://github.com/AltraMayor/gatekeeper ) starts, and I can reproduce it with one or two members in the bond interface. The problem does not occur if I remove the call to rte_eth_bond_8023ad_dedicated_queues_enable(). -- You are receiving this mail because: You are the assignee for the bug.
RE: [EXTERNAL] Re: [PATCH v3] net/netvsc: fix number Tx queues > Rx queues
> -Original Message-
> From: Ferruh Yigit
> Sent: Thursday, April 11, 2024 7:38 AM
> To: Alan Elder ; Long Li ;
> Andrew Rybchenko
> Cc: dev@dpdk.org; stephen
> Subject: [EXTERNAL] Re: [PATCH v3] net/netvsc: fix number Tx queues > Rx
> queues
>
> On 3/19/2024 2:16 PM, Alan Elder wrote:
> > The previous code allowed the number of Tx queues to be set higher
> > than the number of Rx queues. If a packet was sent on a Tx queue with
> > index
> >> = number Rx queues there was a segfault.
> > This commit fixes the issue by creating an Rx queue for every Tx queue
> > meaning that an event buffer is allocated to handle receiving Tx
> > completion messages.
> >
> > mbuf pool and Rx ring are not allocated for these additional Rx queues
> > and RSS configuration ensures that no packets are received on them.
> >
> > Fixes: 4e9c73e96e83 ("net/netvsc: add Hyper-V network device")
> > Cc: sthem...@microsoft.com
> > Cc: sta...@dpdk.org
> >
> > Signed-off-by: Alan Elder
> >
>
> Hi Alan,
>
> What is the root cause of the crash, is it in driver scope or application?
Hi Ferruh,
The root cause of the crash was in the driver - a packet received on a Tx queue
that had no corresponding Rx queue would cause the dev->data->rx_queues[] array
to be accessed past the length of the array.
https://github.com/DPDK/dpdk/blob/main/drivers/net/netvsc/hn_rxtx.c#L1071
Thanks,
Alan
Re: [PATCH v3] ethdev: fix strict aliasing lead to link cannot be up
Hi Morten,
On 2024/4/11 20:44, Morten Brørup wrote:
>> From: Chengwen Feng [mailto:fengcheng...@huawei.com]
>> Sent: Thursday, 11 April 2024 14.04
>>
>> Fix a problem introduced by a compiler upgrade (from gcc10 to gcc12.3),
>> which will lead the hns3 NIC can't link up. The root cause is strict
>> aliasing violation in rte_eth_linkstatus_set() with hns3 driver, see
>> [1] for more details.
>>
>> This commit use union to avoid such aliasing violation.
>>
>> [1] Strict aliasing problem with rte_eth_linkstatus_set()
>> https://marc.info/?l=dpdk-dev&m=171274148514777&w=3
>>
>> Cc: sta...@dpdk.org
>>
>> Signed-off-by: Chengwen Feng
>> Signed-off-by: Dengdui Huang
>>
>> ---
>> v3: fix checkpatch warning "missing --in-reply-to".
>> v2: add RTE_ATOMIC(uint64_t) wrap which address Morten's comment.
>>
>> ---
>> lib/ethdev/ethdev_driver.h | 23 +++
>> lib/ethdev/rte_ethdev.h| 16 ++--
>> 2 files changed, 17 insertions(+), 22 deletions(-)
>>
>> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
>> index 0dbf2dd6a2..9d831d5c84 100644
>> --- a/lib/ethdev/ethdev_driver.h
>> +++ b/lib/ethdev/ethdev_driver.h
>> @@ -1674,18 +1674,13 @@ static inline int
>> rte_eth_linkstatus_set(struct rte_eth_dev *dev,
>> const struct rte_eth_link *new_link)
>> {
>> -RTE_ATOMIC(uint64_t) *dev_link = (uint64_t __rte_atomic *)&(dev-
>>> data->dev_link);
>> -union {
>> -uint64_t val64;
>> -struct rte_eth_link link;
>> -} orig;
>> -
>> -RTE_BUILD_BUG_ON(sizeof(*new_link) != sizeof(uint64_t));
>> +struct rte_eth_link old_link;
>>
>> -orig.val64 = rte_atomic_exchange_explicit(dev_link, *(const
>> uint64_t *)new_link,
>> -rte_memory_order_seq_cst);
>> +old_link.val64 = rte_atomic_exchange_explicit(&dev->data-
>>> dev_link.val64,
>
> You are right; old_link has local scope and is on the stack, so atomic store
> is not required.
>
> And since rte_eth_linkstatus_set() is an internal function called from the
> driver only, it is probably safe to assume that *new_link is on the caller's
> stack and doesn't change while being accessed by this function.
> I guess that new_link is passed by reference for performance and
> future-proofing reasons; it could have been passed by value instead. If it
> was passed by value, atomic access would certainly not be required.
> In other words: You are right here too; new_link does not require atomic load.
>
>> + new_link->val64,
>> + rte_memory_order_seq_cst);
>>
>> -return (orig.link.link_status == new_link->link_status) ? -1 : 0;
>> +return (old_link.link_status == new_link->link_status) ? -1 : 0;
>> }
>>
>> /**
>> @@ -1701,12 +1696,8 @@ static inline void
>> rte_eth_linkstatus_get(const struct rte_eth_dev *dev,
>> struct rte_eth_link *link)
>> {
>> -RTE_ATOMIC(uint64_t) *src = (uint64_t __rte_atomic *)&(dev->data-
>>> dev_link);
>> -uint64_t *dst = (uint64_t *)link;
>> -
>> -RTE_BUILD_BUG_ON(sizeof(*link) != sizeof(uint64_t));
>> -
>> -*dst = rte_atomic_load_explicit(src, rte_memory_order_seq_cst);
>> +link->val64 = rte_atomic_load_explicit(&dev->data->dev_link.val64,
>> + rte_memory_order_seq_cst);
>
> It is not safe to assume that the link pointer points to local memory on the
> caller's stack.
> The link pointer might point to a shared memory area, used by multiple
> threads/processes, so it needs to be stored atomically using
> rte_atomic_store_explicit(&link->val64, ..., rte_memory_order_seq_cst).
I checked every call of rte_eth_linkstatus_get in DPDK, and all the link
parameters are local variables.
The dev->data->dev_link is placed in shared memory which could access from
different threads/processes, it seems no need maintain another link struct
which act the same role.
So I think we should keep current impl, and not using
rte_atomic_store_explicit(&link->val64,...
Thanks
>
>> }
>>
>> /**
>> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
>> index 147257d6a2..ccf43e468a 100644
>> --- a/lib/ethdev/rte_ethdev.h
>> +++ b/lib/ethdev/rte_ethdev.h
>> @@ -332,12 +332,16 @@ struct rte_eth_stats {
>> /**
>> * A structure used to retrieve link-level information of an Ethernet
>> port.
>> */
>> -__extension__
>> -struct __rte_aligned(8) rte_eth_link { /**< aligned for atomic64
>> read/write */
>> -uint32_t link_speed;/**< RTE_ETH_SPEED_NUM_ */
>> -uint16_t link_duplex : 1; /**< RTE_ETH_LINK_[HALF/FULL]_DUPLEX
>> */
>> -uint16_t link_autoneg : 1; /**< RTE_ETH_LINK_[AUTONEG/FIXED] */
>> -uint16_t link_status : 1; /**< RTE_ETH_LINK_[DOWN/UP] */
>> +struct rte_eth_link {
>> +union {
>> +RTE_ATOMIC(uint64_t) val64; /**< used for atomic64
>> read/write */
>> +struct
