Skip to content

[PATCH v1] linux-gen: dpdk: misc fixes #2230

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
MatiasElo wants to merge 2 commits into
base: master
Choose a base branch
from
Open

[PATCH v1] linux-gen: dpdk: misc fixes #2230

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
74ac566
linux-gen: dpdk: increase minimum rx burst size
MatiasElo Jun 11, 2025
c6339bc
linux-gen: dpdk: fix crash with stopped pktio device
MatiasElo Jun 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 13 additions & 10 deletions platform/linux-generic/pktio/dpdk.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/* SPDX-License-Identifier: BSD-3-Clause
* Copyright (c) 2016-2018 Linaro Limited
* Copyright (c) 2019-2023 Nokia
* Copyright (c) 2019-2025 Nokia
*/

#include <odp/autoheader_internal.h>
Expand Down Expand Up @@ -88,7 +88,7 @@ ODP_STATIC_ASSERT((DPDK_NB_MBUF % DPDK_MEMPOOL_CACHE_SIZE == 0) &&
, "DPDK mempool cache size failure");

/* Minimum RX burst size */
#define DPDK_MIN_RX_BURST 4
#define DPDK_MIN_RX_BURST 8

ODP_STATIC_ASSERT(DPDK_MIN_RX_BURST <= UINT8_MAX, "DPDK_MIN_RX_BURST too large");

Expand Down Expand Up @@ -1345,14 +1345,14 @@ static int dpdk_setup_eth_dev(pktio_entry_t *pktio_entry)
static int dpdk_close(pktio_entry_t *pktio_entry)
{
pkt_dpdk_t *pkt_dpdk = pkt_priv(pktio_entry);
unsigned idx;
unsigned i, j;

rte_eth_dev_stop(pkt_dpdk->port_id);

/* Free cache packets */
for (i = 0; i < ODP_PKTIN_MAX_QUEUES; i++) {
idx = pkt_dpdk->rx_cache[i].idx;
for (uint32_t i = 0; i < ODP_PKTIN_MAX_QUEUES; i++) {
uint8_t idx = pkt_dpdk->rx_cache[i].idx;

for (j = 0; j < pkt_dpdk->rx_cache[i].count; j++)
for (uint8_t j = 0; j < pkt_dpdk->rx_cache[i].count; j++)
rte_pktmbuf_free(pkt_dpdk->rx_cache[i].pkt[idx++]);
}

Expand Down Expand Up @@ -2060,6 +2060,11 @@ static int dpdk_start(pktio_entry_t *pktio_entry)
uint16_t port_id = pkt_dpdk->port_id;
int ret;

/* Stop previously started DPDK device to enable reconfiguration */
if (pktio_entry->state == PKTIO_STATE_STOPPED ||
pktio_entry->state == PKTIO_STATE_STOP_PENDING)
rte_eth_dev_stop(port_id);

ret = rte_eth_dev_info_get(port_id, &dev_info);
if (ret != 0) {
_ODP_ERR("DPDK: rte_eth_dev_info_get() failed with return value: %d, port: %u\n",
Expand Down Expand Up @@ -2124,10 +2129,8 @@ static int dpdk_start(pktio_entry_t *pktio_entry)
return 0;
}

static int dpdk_stop(pktio_entry_t *pktio_entry)
static int dpdk_stop(pktio_entry_t *pktio_entry ODP_UNUSED)
{
rte_eth_dev_stop(pkt_priv(pktio_entry)->port_id);
Copy link
Copy Markdown
Collaborator

@JannePeltonen JannePeltonen Jun 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure this is the right approach.

The generic packet i/o code maintains the logical pktio state and already tries to ensure that the device level TX and RX routines are not called when the pktio is in the stopped state. Maybe the real problem is that the interface between the generic pktio code and the device level code is not clear enough and not adhered to by the generic code.

The generic pktio code seems to be racy with respect to maintaining and checking the pktio state. _pktio_stop() sets the pktio state to stopped/stop_pending only "after" it has called the device specific stop function. TX and RX routines may thus not see the state change early enough and call the device specific TX and RX routines when the device specific stop has already been done, which presumably caused the crash mentioned in the commit title. Furthermore, the state variable is not an atomic sync variable used with proper memory ordering, so just moving the write to it in _pktio_stop() would not be enough.

I think the common code should be fixed.

Copy link
Copy Markdown
Collaborator Author

@MatiasElo MatiasElo Jun 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the packet IO state maintenance, or lack of it, is the culprit behind this issue. This commit is just a quick fix/workaround to enable performance testing. Fixing the pktio state maintenance is a bigger task and we need to consider how much overhead is accepted for the fast path functions.


return 0;
}

Expand Down