* [PATCH net v2] net: sfc: add missing xdp queue reinitialization
@ 2022-03-30 16:37 Taehee Yoo
2022-04-01 11:00 ` patchwork-bot+netdevbpf
2022-04-01 11:06 ` Martin Habets
0 siblings, 2 replies; 6+ messages in thread
From: Taehee Yoo @ 2022-03-30 16:37 UTC (permalink / raw)
To: davem, kuba, pabeni, netdev, bpf, ecree.xilinx, habetsm.xilinx,
ast, daniel, hawk, john.fastabend, cmclachlan
Cc: ap420073
After rx/tx ring buffer size is changed, kernel panic occurs when
it acts XDP_TX or XDP_REDIRECT.
When tx/rx ring buffer size is changed(ethtool -G), sfc driver
reallocates and reinitializes rx and tx queues and their buffer
(tx_queue->buffer).
But it misses reinitializing xdp queues(efx->xdp_tx_queues).
So, while it is acting XDP_TX or XDP_REDIRECT, it uses the uninitialized
tx_queue->buffer.
A new function efx_set_xdp_channels() is separated from efx_set_channels()
to handle only xdp queues.
Splat looks like:
BUG: kernel NULL pointer dereference, address: 000000000000002a
#PF: supervisor write access in kernel mode
#PF: error_code(0x0002) - not-present page
PGD 0 P4D 0
Oops: 0002 [#4] PREEMPT SMP NOPTI
RIP: 0010:efx_tx_map_chunk+0x54/0x90 [sfc]
CPU: 2 PID: 0 Comm: swapper/2 Tainted: G D 5.17.0+ #55 e8beeee8289528f11357029357cf
Code: 48 8b 8d a8 01 00 00 48 8d 14 52 4c 8d 2c d0 44 89 e0 48 85 c9 74 0e 44 89 e2 4c 89 f6 48 80
RSP: 0018:ffff92f121e45c60 EFLAGS: 00010297
RIP: 0010:efx_tx_map_chunk+0x54/0x90 [sfc]
RAX: 0000000000000040 RBX: ffff92ea506895c0 RCX: ffffffffc0330870
RDX: 0000000000000001 RSI: 00000001139b10ce RDI: ffff92ea506895c0
RBP: ffffffffc0358a80 R08: 00000001139b110d R09: 0000000000000000
R10: 0000000000000001 R11: ffff92ea414c0088 R12: 0000000000000040
R13: 0000000000000018 R14: 00000001139b10ce R15: ffff92ea506895c0
FS: 0000000000000000(0000) GS:ffff92f121ec0000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
Code: 48 8b 8d a8 01 00 00 48 8d 14 52 4c 8d 2c d0 44 89 e0 48 85 c9 74 0e 44 89 e2 4c 89 f6 48 80
CR2: 000000000000002a CR3: 00000003e6810004 CR4: 00000000007706e0
RSP: 0018:ffff92f121e85c60 EFLAGS: 00010297
PKRU: 55555554
RAX: 0000000000000040 RBX: ffff92ea50689700 RCX: ffffffffc0330870
RDX: 0000000000000001 RSI: 00000001145a90ce RDI: ffff92ea50689700
RBP: ffffffffc0358a80 R08: 00000001145a910d R09: 0000000000000000
R10: 0000000000000001 R11: ffff92ea414c0088 R12: 0000000000000040
R13: 0000000000000018 R14: 00000001145a90ce R15: ffff92ea50689700
FS: 0000000000000000(0000) GS:ffff92f121e80000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 000000000000002a CR3: 00000003e6810005 CR4: 00000000007706e0
PKRU: 55555554
Call Trace:
<IRQ>
efx_xdp_tx_buffers+0x12b/0x3d0 [sfc 84c94b8e32d44d296c17e10a634d3ad454de4ba5]
__efx_rx_packet+0x5c3/0x930 [sfc 84c94b8e32d44d296c17e10a634d3ad454de4ba5]
efx_rx_packet+0x28c/0x2e0 [sfc 84c94b8e32d44d296c17e10a634d3ad454de4ba5]
efx_ef10_ev_process+0x5f8/0xf40 [sfc 84c94b8e32d44d296c17e10a634d3ad454de4ba5]
? enqueue_task_fair+0x95/0x550
efx_poll+0xc4/0x360 [sfc 84c94b8e32d44d296c17e10a634d3ad454de4ba5]
Fixes: 3990a8fffbda ("sfc: allocate channels for XDP tx queues")
Signed-off-by: Taehee Yoo <ap420073@gmail.com>
---
v2: Do not use inline in .c file
drivers/net/ethernet/sfc/efx_channels.c | 146 +++++++++++++-----------
1 file changed, 81 insertions(+), 65 deletions(-)
diff --git a/drivers/net/ethernet/sfc/efx_channels.c b/drivers/net/ethernet/sfc/efx_channels.c
index d6fdcdc530ca..b29ca69aeca5 100644
--- a/drivers/net/ethernet/sfc/efx_channels.c
+++ b/drivers/net/ethernet/sfc/efx_channels.c
@@ -789,6 +789,85 @@ void efx_remove_channels(struct efx_nic *efx)
kfree(efx->xdp_tx_queues);
}
+static int efx_set_xdp_tx_queue(struct efx_nic *efx, int xdp_queue_number,
+ struct efx_tx_queue *tx_queue)
+{
+ if (xdp_queue_number >= efx->xdp_tx_queue_count)
+ return -EINVAL;
+
+ netif_dbg(efx, drv, efx->net_dev,
+ "Channel %u TXQ %u is XDP %u, HW %u\n",
+ tx_queue->channel->channel, tx_queue->label,
+ xdp_queue_number, tx_queue->queue);
+ efx->xdp_tx_queues[xdp_queue_number] = tx_queue;
+ return 0;
+}
+
+static void efx_set_xdp_channels(struct efx_nic *efx)
+{
+ struct efx_tx_queue *tx_queue;
+ struct efx_channel *channel;
+ unsigned int next_queue = 0;
+ int xdp_queue_number = 0;
+ int rc;
+
+ /* We need to mark which channels really have RX and TX
+ * queues, and adjust the TX queue numbers if we have separate
+ * RX-only and TX-only channels.
+ */
+ efx_for_each_channel(channel, efx) {
+ if (channel->channel < efx->tx_channel_offset)
+ continue;
+
+ if (efx_channel_is_xdp_tx(channel)) {
+ efx_for_each_channel_tx_queue(tx_queue, channel) {
+ tx_queue->queue = next_queue++;
+ rc = efx_set_xdp_tx_queue(efx, xdp_queue_number,
+ tx_queue);
+ if (rc == 0)
+ xdp_queue_number++;
+ }
+ } else {
+ efx_for_each_channel_tx_queue(tx_queue, channel) {
+ tx_queue->queue = next_queue++;
+ netif_dbg(efx, drv, efx->net_dev,
+ "Channel %u TXQ %u is HW %u\n",
+ channel->channel, tx_queue->label,
+ tx_queue->queue);
+ }
+
+ /* If XDP is borrowing queues from net stack, it must
+ * use the queue with no csum offload, which is the
+ * first one of the channel
+ * (note: tx_queue_by_type is not initialized yet)
+ */
+ if (efx->xdp_txq_queues_mode ==
+ EFX_XDP_TX_QUEUES_BORROWED) {
+ tx_queue = &channel->tx_queue[0];
+ rc = efx_set_xdp_tx_queue(efx, xdp_queue_number,
+ tx_queue);
+ if (rc == 0)
+ xdp_queue_number++;
+ }
+ }
+ }
+ WARN_ON(efx->xdp_txq_queues_mode == EFX_XDP_TX_QUEUES_DEDICATED &&
+ xdp_queue_number != efx->xdp_tx_queue_count);
+ WARN_ON(efx->xdp_txq_queues_mode != EFX_XDP_TX_QUEUES_DEDICATED &&
+ xdp_queue_number > efx->xdp_tx_queue_count);
+
+ /* If we have more CPUs than assigned XDP TX queues, assign the already
+ * existing queues to the exceeding CPUs
+ */
+ next_queue = 0;
+ while (xdp_queue_number < efx->xdp_tx_queue_count) {
+ tx_queue = efx->xdp_tx_queues[next_queue++];
+ rc = efx_set_xdp_tx_queue(efx, xdp_queue_number, tx_queue);
+ if (rc == 0)
+ xdp_queue_number++;
+ }
+}
+
int efx_realloc_channels(struct efx_nic *efx, u32 rxq_entries, u32 txq_entries)
{
struct efx_channel *other_channel[EFX_MAX_CHANNELS], *channel;
@@ -860,6 +939,7 @@ int efx_realloc_channels(struct efx_nic *efx, u32 rxq_entries, u32 txq_entries)
efx_init_napi_channel(efx->channel[i]);
}
+ efx_set_xdp_channels(efx);
out:
/* Destroy unused channel structures */
for (i = 0; i < efx->n_channels; i++) {
@@ -892,26 +972,9 @@ int efx_realloc_channels(struct efx_nic *efx, u32 rxq_entries, u32 txq_entries)
goto out;
}
-static inline int
-efx_set_xdp_tx_queue(struct efx_nic *efx, int xdp_queue_number,
- struct efx_tx_queue *tx_queue)
-{
- if (xdp_queue_number >= efx->xdp_tx_queue_count)
- return -EINVAL;
-
- netif_dbg(efx, drv, efx->net_dev, "Channel %u TXQ %u is XDP %u, HW %u\n",
- tx_queue->channel->channel, tx_queue->label,
- xdp_queue_number, tx_queue->queue);
- efx->xdp_tx_queues[xdp_queue_number] = tx_queue;
- return 0;
-}
-
int efx_set_channels(struct efx_nic *efx)
{
- struct efx_tx_queue *tx_queue;
struct efx_channel *channel;
- unsigned int next_queue = 0;
- int xdp_queue_number;
int rc;
efx->tx_channel_offset =
@@ -929,61 +992,14 @@ int efx_set_channels(struct efx_nic *efx)
return -ENOMEM;
}
- /* We need to mark which channels really have RX and TX
- * queues, and adjust the TX queue numbers if we have separate
- * RX-only and TX-only channels.
- */
- xdp_queue_number = 0;
efx_for_each_channel(channel, efx) {
if (channel->channel < efx->n_rx_channels)
channel->rx_queue.core_index = channel->channel;
else
channel->rx_queue.core_index = -1;
-
- if (channel->channel >= efx->tx_channel_offset) {
- if (efx_channel_is_xdp_tx(channel)) {
- efx_for_each_channel_tx_queue(tx_queue, channel) {
- tx_queue->queue = next_queue++;
- rc = efx_set_xdp_tx_queue(efx, xdp_queue_number, tx_queue);
- if (rc == 0)
- xdp_queue_number++;
- }
- } else {
- efx_for_each_channel_tx_queue(tx_queue, channel) {
- tx_queue->queue = next_queue++;
- netif_dbg(efx, drv, efx->net_dev, "Channel %u TXQ %u is HW %u\n",
- channel->channel, tx_queue->label,
- tx_queue->queue);
- }
-
- /* If XDP is borrowing queues from net stack, it must use the queue
- * with no csum offload, which is the first one of the channel
- * (note: channel->tx_queue_by_type is not initialized yet)
- */
- if (efx->xdp_txq_queues_mode == EFX_XDP_TX_QUEUES_BORROWED) {
- tx_queue = &channel->tx_queue[0];
- rc = efx_set_xdp_tx_queue(efx, xdp_queue_number, tx_queue);
- if (rc == 0)
- xdp_queue_number++;
- }
- }
- }
}
- WARN_ON(efx->xdp_txq_queues_mode == EFX_XDP_TX_QUEUES_DEDICATED &&
- xdp_queue_number != efx->xdp_tx_queue_count);
- WARN_ON(efx->xdp_txq_queues_mode != EFX_XDP_TX_QUEUES_DEDICATED &&
- xdp_queue_number > efx->xdp_tx_queue_count);
- /* If we have more CPUs than assigned XDP TX queues, assign the already
- * existing queues to the exceeding CPUs
- */
- next_queue = 0;
- while (xdp_queue_number < efx->xdp_tx_queue_count) {
- tx_queue = efx->xdp_tx_queues[next_queue++];
- rc = efx_set_xdp_tx_queue(efx, xdp_queue_number, tx_queue);
- if (rc == 0)
- xdp_queue_number++;
- }
+ efx_set_xdp_channels(efx);
rc = netif_set_real_num_tx_queues(efx->net_dev, efx->n_tx_channels);
if (rc)
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net v2] net: sfc: add missing xdp queue reinitialization
2022-03-30 16:37 [PATCH net v2] net: sfc: add missing xdp queue reinitialization Taehee Yoo
@ 2022-04-01 11:00 ` patchwork-bot+netdevbpf
2022-04-01 11:06 ` Martin Habets
1 sibling, 0 replies; 6+ messages in thread
From: patchwork-bot+netdevbpf @ 2022-04-01 11:00 UTC (permalink / raw)
To: Taehee Yoo
Cc: davem, kuba, pabeni, netdev, bpf, ecree.xilinx, habetsm.xilinx,
ast, daniel, hawk, john.fastabend, cmclachlan
Hello:
This patch was applied to netdev/net.git (master)
by David S. Miller <davem@davemloft.net>:
On Wed, 30 Mar 2022 16:37:03 +0000 you wrote:
> After rx/tx ring buffer size is changed, kernel panic occurs when
> it acts XDP_TX or XDP_REDIRECT.
>
> When tx/rx ring buffer size is changed(ethtool -G), sfc driver
> reallocates and reinitializes rx and tx queues and their buffer
> (tx_queue->buffer).
> But it misses reinitializing xdp queues(efx->xdp_tx_queues).
> So, while it is acting XDP_TX or XDP_REDIRECT, it uses the uninitialized
> tx_queue->buffer.
>
> [...]
Here is the summary with links:
- [net,v2] net: sfc: add missing xdp queue reinitialization
https://git.kernel.org/netdev/net/c/059a47f1da93
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net v2] net: sfc: add missing xdp queue reinitialization
2022-03-30 16:37 [PATCH net v2] net: sfc: add missing xdp queue reinitialization Taehee Yoo
2022-04-01 11:00 ` patchwork-bot+netdevbpf
@ 2022-04-01 11:06 ` Martin Habets
2022-04-01 18:48 ` Taehee Yoo
1 sibling, 1 reply; 6+ messages in thread
From: Martin Habets @ 2022-04-01 11:06 UTC (permalink / raw)
To: Taehee Yoo
Cc: davem, kuba, pabeni, netdev, bpf, ecree.xilinx, ast, daniel,
hawk, john.fastabend, habetsm.xilinx
Hi Taehee,
Thanks for looking into this. Unfortunately efx_realloc_channels()
has turned out to be quite fragile over the years, so I'm
keen to remove it in stead of patching it up all the time.
Could you try the patch below please?
If it works ok for you as well we'll be able to remove
efx_realloc_channels(). The added advantage of this approach
is that the netdev notifiers get informed of the change.
Regards,
Martin Habets <habetsm.xilinx@gmail.com>
---
drivers/net/ethernet/sfc/ethtool.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/sfc/ethtool.c b/drivers/net/ethernet/sfc/ethtool.c
index 48506373721a..8cfbe61737bb 100644
--- a/drivers/net/ethernet/sfc/ethtool.c
+++ b/drivers/net/ethernet/sfc/ethtool.c
@@ -179,6 +179,7 @@ efx_ethtool_set_ringparam(struct net_device *net_dev,
{
struct efx_nic *efx = netdev_priv(net_dev);
u32 txq_entries;
+ int rc = 0;
if (ring->rx_mini_pending || ring->rx_jumbo_pending ||
ring->rx_pending > EFX_MAX_DMAQ_SIZE ||
@@ -198,7 +199,17 @@ efx_ethtool_set_ringparam(struct net_device *net_dev,
"increasing TX queue size to minimum of %u\n",
txq_entries);
- return efx_realloc_channels(efx, ring->rx_pending, txq_entries);
+ /* Apply the new settings */
+ efx->rxq_entries = ring->rx_pending;
+ efx->txq_entries = ring->tx_pending;
+
+ /* Update the datapath with the new settings if the interface is up */
+ if (!efx_check_disabled(efx) && netif_running(efx->net_dev)) {
+ dev_close(net_dev);
+ rc = dev_open(net_dev, NULL);
+ }
+
+ return rc;
}
static void efx_ethtool_get_wol(struct net_device *net_dev,
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net v2] net: sfc: add missing xdp queue reinitialization
2022-04-01 11:06 ` Martin Habets
@ 2022-04-01 18:48 ` Taehee Yoo
2022-04-04 10:36 ` Martin Habets
0 siblings, 1 reply; 6+ messages in thread
From: Taehee Yoo @ 2022-04-01 18:48 UTC (permalink / raw)
To: davem, kuba, pabeni, netdev, bpf, ecree.xilinx, ast, daniel,
hawk, john.fastabend
On 4/1/22 20:06, Martin Habets wrote:
Hi Martin,
Thank you so much for your review!
> Hi Taehee,
>
> Thanks for looking into this. Unfortunately efx_realloc_channels()
> has turned out to be quite fragile over the years, so I'm
> keen to remove it in stead of patching it up all the time.
I agree with you.
efx_realloc_channels() is too complex.
>
> Could you try the patch below please?
> If it works ok for you as well we'll be able to remove
> efx_realloc_channels(). The added advantage of this approach
> is that the netdev notifiers get informed of the change.
I tested your patch and I found a page reference count problem.
How to test:
1. set up XDP_TX
2. traffic on
3. traffic off
4. ring buffer size change
5. loop from 2 to 4.
[ 87.836195][ T72] BUG: Bad page state in process kworker/u16:1
pfn:125445
[ 87.843356][ T72] page:000000003725f642 refcount:-2 mapcount:0
mapping:0000000000000000 index:0x0 pfn:0x125445
[ 87.853783][ T72] flags: 0x200000000000000(node=0|zone=2)
[ 87.859391][ T72] raw: 0200000000000000 dead000000000100
dead000000000122 0000000000000000
[ 87.867928][ T72] raw: 0000000000000000 0000000000000000
fffffffeffffffff 0000000000000000
[ 87.876569][ T72] page dumped because: nonzero _refcount
[ 87.882125][ T72] Modules linked in: af_packet sfc ixgbe mtd
atlantic coretemp mdio hwmon sch_fq_codel msr bpf_prelx
[ 87.895331][ T72] CPU: 0 PID: 72 Comm: kworker/u16:1 Not tainted
5.17.0+ #62 dbf33652f22e5147659e7e2472bb962779c4833
[ 87.906350][ T72] Hardware name: ASUS System Product Name/PRIME
Z690-P D4, BIOS 0603 11/01/2021
[ 87.915360][ T72] Workqueue: netns cleanup_net
[ 87.920087][ T72] Call Trace:
[ 87.923311][ T72] <TASK>
[ 87.926188][ T72] dump_stack_lvl+0x56/0x7b
[ 87.930597][ T72] bad_page.cold.125+0x63/0x93
[ 87.935288][ T72] free_pcppages_bulk+0x63c/0x6f0
[ 87.940232][ T72] free_unref_page+0x8b/0xf0
[ 87.944749][ T72] efx_fini_rx_queue+0x15f/0x210 [sfc
49c5d4f562a40c6a7ed913c25f5bd4e126bcfa4e]
[ 87.953756][ T72] efx_stop_channels+0xef/0x1b0 [sfc
49c5d4f562a40c6a7ed913c25f5bd4e126bcfa4e]
[ 87.962699][ T72] efx_net_stop+0x4d/0x60 [sfc
49c5d4f562a40c6a7ed913c25f5bd4e126bcfa4e]
[ 87.971029][ T72] __dev_close_many+0x8b/0xf0
[ 87.975618][ T72] dev_close_many+0x7d/0x120
[ ... ]
In addition, I would like to share issues that I'm currently looking into:
1. TX DMA error
when interface down/up or ring buffer size changes, TX DMA error would occur
because tx_queue can be used before initialization.
But It will be fixed by the below patch.
static void efx_ethtool_get_wol(struct net_device *net_dev,
diff --git a/drivers/net/ethernet/sfc/tx.c b/drivers/net/ethernet/sfc/tx.c
index d16e031e95f4..6983799e1c05 100644
--- a/drivers/net/ethernet/sfc/tx.c
+++ b/drivers/net/ethernet/sfc/tx.c
@@ -443,6 +443,9 @@ int efx_xdp_tx_buffers(struct efx_nic *efx, int n,
struct xdp_frame **xdpfs,
if (unlikely(!tx_queue))
return -EINVAL;
+ if (!tx_queue->initialised)
+ return -EINVAL;
+
if (efx->xdp_txq_queues_mode != EFX_XDP_TX_QUEUES_DEDICATED)
HARD_TX_LOCK(efx->net_dev, tx_queue->core_txq, cpu);
diff --git a/drivers/net/ethernet/sfc/tx_common.c
b/drivers/net/ethernet/sfc/tx_common.c
index d530cde2b864..9bc8281b7f5b 100644
--- a/drivers/net/ethernet/sfc/tx_common.c
+++ b/drivers/net/ethernet/sfc/tx_common.c
@@ -101,6 +101,8 @@ void efx_fini_tx_queue(struct efx_tx_queue *tx_queue)
netif_dbg(tx_queue->efx, drv, tx_queue->efx->net_dev,
"shutting down TX queue %d\n", tx_queue->queue);
+ tx_queue->initialised = false;
+
if (!tx_queue->buffer)
return;
After your patch, unfortunately, it can't fix ring buffer size change case.
It can fix only interface down/up case.
I will look into this more.
2. Memory leak
There is a memory leak in ring buffer size change logic.
reproducer:
while :
do
ethtool -G <interface name> rx 2048 tx 2048
ethtool -G <interface name> rx 1024 tx 1024
done
Thanks a lot,
Taehee Yoo
>
> Regards,
> Martin Habets <habetsm.xilinx@gmail.com>
>
> ---
> drivers/net/ethernet/sfc/ethtool.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/sfc/ethtool.c
b/drivers/net/ethernet/sfc/ethtool.c
> index 48506373721a..8cfbe61737bb 100644
> --- a/drivers/net/ethernet/sfc/ethtool.c
> +++ b/drivers/net/ethernet/sfc/ethtool.c
> @@ -179,6 +179,7 @@ efx_ethtool_set_ringparam(struct net_device *net_dev,
> {
> struct efx_nic *efx = netdev_priv(net_dev);
> u32 txq_entries;
> + int rc = 0;
>
> if (ring->rx_mini_pending || ring->rx_jumbo_pending ||
> ring->rx_pending > EFX_MAX_DMAQ_SIZE ||
> @@ -198,7 +199,17 @@ efx_ethtool_set_ringparam(struct net_device
*net_dev,
> "increasing TX queue size to minimum of %u\n",
> txq_entries);
>
> - return efx_realloc_channels(efx, ring->rx_pending, txq_entries);
> + /* Apply the new settings */
> + efx->rxq_entries = ring->rx_pending;
> + efx->txq_entries = ring->tx_pending;
> +
> + /* Update the datapath with the new settings if the interface is up */
> + if (!efx_check_disabled(efx) && netif_running(efx->net_dev)) {
> + dev_close(net_dev);
> + rc = dev_open(net_dev, NULL);
> + }
> +
> + return rc;
> }
>
> static void efx_ethtool_get_wol(struct net_device *net_dev,
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net v2] net: sfc: add missing xdp queue reinitialization
2022-04-01 18:48 ` Taehee Yoo
@ 2022-04-04 10:36 ` Martin Habets
2022-04-05 3:16 ` Taehee Yoo
0 siblings, 1 reply; 6+ messages in thread
From: Martin Habets @ 2022-04-04 10:36 UTC (permalink / raw)
To: Taehee Yoo
Cc: davem, kuba, pabeni, netdev, bpf, ecree.xilinx, ast, daniel,
hawk, john.fastabend
Hi Taehee,
On Sat, Apr 02, 2022 at 03:48:14AM +0900, Taehee Yoo wrote:
> On 4/1/22 20:06, Martin Habets wrote:
>
> Hi Martin,
> Thank you so much for your review!
>
> > Hi Taehee,
> >
> > Thanks for looking into this. Unfortunately efx_realloc_channels()
> > has turned out to be quite fragile over the years, so I'm
> > keen to remove it in stead of patching it up all the time.
>
> I agree with you.
> efx_realloc_channels() is too complex.
>
> >
> > Could you try the patch below please?
> > If it works ok for you as well we'll be able to remove
> > efx_realloc_channels(). The added advantage of this approach
> > is that the netdev notifiers get informed of the change.
>
> I tested your patch and I found a page reference count problem.
> How to test:
> 1. set up XDP_TX
> 2. traffic on
> 3. traffic off
> 4. ring buffer size change
> 5. loop from 2 to 4.
>
> [ 87.836195][ T72] BUG: Bad page state in process kworker/u16:1
> pfn:125445
> [ 87.843356][ T72] page:000000003725f642 refcount:-2 mapcount:0
> mapping:0000000000000000 index:0x0 pfn:0x125445
> [ 87.853783][ T72] flags: 0x200000000000000(node=0|zone=2)
>
> [ 87.859391][ T72] raw: 0200000000000000 dead000000000100
> dead000000000122 0000000000000000
> [ 87.867928][ T72] raw: 0000000000000000 0000000000000000
> fffffffeffffffff 0000000000000000
> [ 87.876569][ T72] page dumped because: nonzero _refcount
>
> [ 87.882125][ T72] Modules linked in: af_packet sfc ixgbe mtd atlantic
> coretemp mdio hwmon sch_fq_codel msr bpf_prelx
> [ 87.895331][ T72] CPU: 0 PID: 72 Comm: kworker/u16:1 Not tainted
> 5.17.0+ #62 dbf33652f22e5147659e7e2472bb962779c4833
> [ 87.906350][ T72] Hardware name: ASUS System Product Name/PRIME Z690-P
> D4, BIOS 0603 11/01/2021
> [ 87.915360][ T72] Workqueue: netns cleanup_net
>
> [ 87.920087][ T72] Call Trace:
>
> [ 87.923311][ T72] <TASK>
>
> [ 87.926188][ T72] dump_stack_lvl+0x56/0x7b
>
> [ 87.930597][ T72] bad_page.cold.125+0x63/0x93
>
> [ 87.935288][ T72] free_pcppages_bulk+0x63c/0x6f0
>
> [ 87.940232][ T72] free_unref_page+0x8b/0xf0
>
> [ 87.944749][ T72] efx_fini_rx_queue+0x15f/0x210 [sfc
> 49c5d4f562a40c6a7ed913c25f5bd4e126bcfa4e]
Looks to me like this is in efx_fini_rx_recycle_ring().
It could be a side effect of the memory leak you report below.
If this is in efx_fini_rx_recycle_ring() I'll post a patch for
that soon on a separate thread.
> [ 87.953756][ T72] efx_stop_channels+0xef/0x1b0 [sfc
> 49c5d4f562a40c6a7ed913c25f5bd4e126bcfa4e]
> [ 87.962699][ T72] efx_net_stop+0x4d/0x60 [sfc
> 49c5d4f562a40c6a7ed913c25f5bd4e126bcfa4e]
> [ 87.971029][ T72] __dev_close_many+0x8b/0xf0
>
> [ 87.975618][ T72] dev_close_many+0x7d/0x120
>
> [ ... ]
>
>
> In addition, I would like to share issues that I'm currently looking into:
> 1. TX DMA error
> when interface down/up or ring buffer size changes, TX DMA error would occur
> because tx_queue can be used before initialization.
> But It will be fixed by the below patch.
>
> static void efx_ethtool_get_wol(struct net_device *net_dev,
> diff --git a/drivers/net/ethernet/sfc/tx.c b/drivers/net/ethernet/sfc/tx.c
> index d16e031e95f4..6983799e1c05 100644
> --- a/drivers/net/ethernet/sfc/tx.c
> +++ b/drivers/net/ethernet/sfc/tx.c
> @@ -443,6 +443,9 @@ int efx_xdp_tx_buffers(struct efx_nic *efx, int n,
> struct xdp_frame **xdpfs,
> if (unlikely(!tx_queue))
> return -EINVAL;
>
> + if (!tx_queue->initialised)
> + return -EINVAL;
> +
> if (efx->xdp_txq_queues_mode != EFX_XDP_TX_QUEUES_DEDICATED)
> HARD_TX_LOCK(efx->net_dev, tx_queue->core_txq, cpu);
>
> diff --git a/drivers/net/ethernet/sfc/tx_common.c
> b/drivers/net/ethernet/sfc/tx_common.c
> index d530cde2b864..9bc8281b7f5b 100644
> --- a/drivers/net/ethernet/sfc/tx_common.c
> +++ b/drivers/net/ethernet/sfc/tx_common.c
> @@ -101,6 +101,8 @@ void efx_fini_tx_queue(struct efx_tx_queue *tx_queue)
> netif_dbg(tx_queue->efx, drv, tx_queue->efx->net_dev,
> "shutting down TX queue %d\n", tx_queue->queue);
>
> + tx_queue->initialised = false;
> +
> if (!tx_queue->buffer)
> return;
Looks ok, but xmit_hard should never be called on an interface that
is down. Makes me wonder if we have a seqence issue in our ndo_stop API.
>
> After your patch, unfortunately, it can't fix ring buffer size change case.
> It can fix only interface down/up case.
> I will look into this more.
>
> 2. Memory leak
> There is a memory leak in ring buffer size change logic.
> reproducer:
> while :
> do
> ethtool -G <interface name> rx 2048 tx 2048
> ethtool -G <interface name> rx 1024 tx 1024
> done
Is this with my patch or only with yours?
Thanks a lot for testing this.
Martin
> Thanks a lot,
> Taehee Yoo
>
> >
> > Regards,
> > Martin Habets <habetsm.xilinx@gmail.com>
> >
> > ---
> > drivers/net/ethernet/sfc/ethtool.c | 13 ++++++++++++-
> > 1 file changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/sfc/ethtool.c
> b/drivers/net/ethernet/sfc/ethtool.c
> > index 48506373721a..8cfbe61737bb 100644
> > --- a/drivers/net/ethernet/sfc/ethtool.c
> > +++ b/drivers/net/ethernet/sfc/ethtool.c
> > @@ -179,6 +179,7 @@ efx_ethtool_set_ringparam(struct net_device *net_dev,
> > {
> > struct efx_nic *efx = netdev_priv(net_dev);
> > u32 txq_entries;
> > + int rc = 0;
> >
> > if (ring->rx_mini_pending || ring->rx_jumbo_pending ||
> > ring->rx_pending > EFX_MAX_DMAQ_SIZE ||
> > @@ -198,7 +199,17 @@ efx_ethtool_set_ringparam(struct net_device *net_dev,
> > "increasing TX queue size to minimum of %u\n",
> > txq_entries);
> >
> > - return efx_realloc_channels(efx, ring->rx_pending, txq_entries);
> > + /* Apply the new settings */
> > + efx->rxq_entries = ring->rx_pending;
> > + efx->txq_entries = ring->tx_pending;
> > +
> > + /* Update the datapath with the new settings if the interface is up */
> > + if (!efx_check_disabled(efx) && netif_running(efx->net_dev)) {
> > + dev_close(net_dev);
> > + rc = dev_open(net_dev, NULL);
> > + }
> > +
> > + return rc;
> > }
> >
> > static void efx_ethtool_get_wol(struct net_device *net_dev,
--
Martin Habets <habetsm.xilinx@gmail.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net v2] net: sfc: add missing xdp queue reinitialization
2022-04-04 10:36 ` Martin Habets
@ 2022-04-05 3:16 ` Taehee Yoo
0 siblings, 0 replies; 6+ messages in thread
From: Taehee Yoo @ 2022-04-05 3:16 UTC (permalink / raw)
To: davem, kuba, pabeni, netdev, bpf, ecree.xilinx, ast, daniel,
hawk, john.fastabend
On 4/4/22 19:36, Martin Habets wrote:
Hi Martin,
> Hi Taehee,
>
> On Sat, Apr 02, 2022 at 03:48:14AM +0900, Taehee Yoo wrote:
>> On 4/1/22 20:06, Martin Habets wrote:
>>
>> Hi Martin,
>> Thank you so much for your review!
>>
>>> Hi Taehee,
>>>
>>> Thanks for looking into this. Unfortunately efx_realloc_channels()
>>> has turned out to be quite fragile over the years, so I'm
>>> keen to remove it in stead of patching it up all the time.
>>
>> I agree with you.
>> efx_realloc_channels() is too complex.
>>
>>>
>>> Could you try the patch below please?
>>> If it works ok for you as well we'll be able to remove
>>> efx_realloc_channels(). The added advantage of this approach
>>> is that the netdev notifiers get informed of the change.
>>
>> I tested your patch and I found a page reference count problem.
>> How to test:
>> 1. set up XDP_TX
>> 2. traffic on
>> 3. traffic off
>> 4. ring buffer size change
>> 5. loop from 2 to 4.
>>
>> [ 87.836195][ T72] BUG: Bad page state in process kworker/u16:1
>> pfn:125445
>> [ 87.843356][ T72] page:000000003725f642 refcount:-2 mapcount:0
>> mapping:0000000000000000 index:0x0 pfn:0x125445
>> [ 87.853783][ T72] flags: 0x200000000000000(node=0|zone=2)
>>
>> [ 87.859391][ T72] raw: 0200000000000000 dead000000000100
>> dead000000000122 0000000000000000
>> [ 87.867928][ T72] raw: 0000000000000000 0000000000000000
>> fffffffeffffffff 0000000000000000
>> [ 87.876569][ T72] page dumped because: nonzero _refcount
>>
>> [ 87.882125][ T72] Modules linked in: af_packet sfc ixgbe mtd
atlantic
>> coretemp mdio hwmon sch_fq_codel msr bpf_prelx
>> [ 87.895331][ T72] CPU: 0 PID: 72 Comm: kworker/u16:1 Not tainted
>> 5.17.0+ #62 dbf33652f22e5147659e7e2472bb962779c4833
>> [ 87.906350][ T72] Hardware name: ASUS System Product Name/PRIME
Z690-P
>> D4, BIOS 0603 11/01/2021
>> [ 87.915360][ T72] Workqueue: netns cleanup_net
>>
>> [ 87.920087][ T72] Call Trace:
>>
>> [ 87.923311][ T72] <TASK>
>>
>> [ 87.926188][ T72] dump_stack_lvl+0x56/0x7b
>>
>> [ 87.930597][ T72] bad_page.cold.125+0x63/0x93
>>
>> [ 87.935288][ T72] free_pcppages_bulk+0x63c/0x6f0
>>
>> [ 87.940232][ T72] free_unref_page+0x8b/0xf0
>>
>> [ 87.944749][ T72] efx_fini_rx_queue+0x15f/0x210 [sfc
>> 49c5d4f562a40c6a7ed913c25f5bd4e126bcfa4e]
>
> Looks to me like this is in efx_fini_rx_recycle_ring().
> It could be a side effect of the memory leak you report below.
> If this is in efx_fini_rx_recycle_ring() I'll post a patch for
> that soon on a separate thread.
Thanks for that!
>
>> [ 87.953756][ T72] efx_stop_channels+0xef/0x1b0 [sfc
>> 49c5d4f562a40c6a7ed913c25f5bd4e126bcfa4e]
>> [ 87.962699][ T72] efx_net_stop+0x4d/0x60 [sfc
>> 49c5d4f562a40c6a7ed913c25f5bd4e126bcfa4e]
>> [ 87.971029][ T72] __dev_close_many+0x8b/0xf0
>>
>> [ 87.975618][ T72] dev_close_many+0x7d/0x120
>>
>> [ ... ]
>>
>>
>> In addition, I would like to share issues that I'm currently looking
into:
>> 1. TX DMA error
>> when interface down/up or ring buffer size changes, TX DMA error
would occur
>> because tx_queue can be used before initialization.
>> But It will be fixed by the below patch.
>>
>> static void efx_ethtool_get_wol(struct net_device *net_dev,
>> diff --git a/drivers/net/ethernet/sfc/tx.c
b/drivers/net/ethernet/sfc/tx.c
>> index d16e031e95f4..6983799e1c05 100644
>> --- a/drivers/net/ethernet/sfc/tx.c
>> +++ b/drivers/net/ethernet/sfc/tx.c
>> @@ -443,6 +443,9 @@ int efx_xdp_tx_buffers(struct efx_nic *efx, int n,
>> struct xdp_frame **xdpfs,
>> if (unlikely(!tx_queue))
>> return -EINVAL;
>>
>> + if (!tx_queue->initialised)
>> + return -EINVAL;
>> +
>> if (efx->xdp_txq_queues_mode != EFX_XDP_TX_QUEUES_DEDICATED)
>> HARD_TX_LOCK(efx->net_dev, tx_queue->core_txq, cpu);
>>
>> diff --git a/drivers/net/ethernet/sfc/tx_common.c
>> b/drivers/net/ethernet/sfc/tx_common.c
>> index d530cde2b864..9bc8281b7f5b 100644
>> --- a/drivers/net/ethernet/sfc/tx_common.c
>> +++ b/drivers/net/ethernet/sfc/tx_common.c
>> @@ -101,6 +101,8 @@ void efx_fini_tx_queue(struct efx_tx_queue
*tx_queue)
>> netif_dbg(tx_queue->efx, drv, tx_queue->efx->net_dev,
>> "shutting down TX queue %d\n", tx_queue->queue);
>>
>> + tx_queue->initialised = false;
>> +
>> if (!tx_queue->buffer)
>> return;
>
> Looks ok, but xmit_hard should never be called on an interface that
> is down. Makes me wonder if we have a seqence issue in our ndo_stop API.
>
I think it's an XDP tx_queue specific issue.
Sorry that I didn't provide that information.
When a packet is received and it acts XDP_TX, it calls
efx_xdp_tx_buffers() and it uses xdp tx_queue and sends it to hardware
directly.
So, ->ndo_start_xmit is not called.
And that bug occurs when the first interface up.
->ndo_stop has never called, So I think this is not an issue of ->ndo_stop.
>>
>> After your patch, unfortunately, it can't fix ring buffer size
change case.
>> It can fix only interface down/up case.
>> I will look into this more.
>>
>> 2. Memory leak
>> There is a memory leak in ring buffer size change logic.
>> reproducer:
>> while :
>> do
>> ethtool -G <interface name> rx 2048 tx 2048
>> ethtool -G <interface name> rx 1024 tx 1024
>> done
>
> Is this with my patch or only with yours?
> Thanks a lot for testing this.
>
Memory leak still occurs with your patch(do not use efx_realloc_channels())
Thanks a lot,
Taehee Yoo
> Martin
>
>> Thanks a lot,
>> Taehee Yoo
>>
>>>
>>> Regards,
>>> Martin Habets <habetsm.xilinx@gmail.com>
>>>
>>> ---
>>> drivers/net/ethernet/sfc/ethtool.c | 13 ++++++++++++-
>>> 1 file changed, 12 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/ethernet/sfc/ethtool.c
>> b/drivers/net/ethernet/sfc/ethtool.c
>>> index 48506373721a..8cfbe61737bb 100644
>>> --- a/drivers/net/ethernet/sfc/ethtool.c
>>> +++ b/drivers/net/ethernet/sfc/ethtool.c
>>> @@ -179,6 +179,7 @@ efx_ethtool_set_ringparam(struct net_device
*net_dev,
>>> {
>>> struct efx_nic *efx = netdev_priv(net_dev);
>>> u32 txq_entries;
>>> + int rc = 0;
>>>
>>> if (ring->rx_mini_pending || ring->rx_jumbo_pending ||
>>> ring->rx_pending > EFX_MAX_DMAQ_SIZE ||
>>> @@ -198,7 +199,17 @@ efx_ethtool_set_ringparam(struct net_device
*net_dev,
>>> "increasing TX queue size to minimum of %u\n",
>>> txq_entries);
>>>
>>> - return efx_realloc_channels(efx, ring->rx_pending, txq_entries);
>>> + /* Apply the new settings */
>>> + efx->rxq_entries = ring->rx_pending;
>>> + efx->txq_entries = ring->tx_pending;
>>> +
>>> + /* Update the datapath with the new settings if the interface is
up */
>>> + if (!efx_check_disabled(efx) && netif_running(efx->net_dev)) {
>>> + dev_close(net_dev);
>>> + rc = dev_open(net_dev, NULL);
>>> + }
>>> +
>>> + return rc;
>>> }
>>>
>>> static void efx_ethtool_get_wol(struct net_device *net_dev,
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-04-05 3:16 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-30 16:37 [PATCH net v2] net: sfc: add missing xdp queue reinitialization Taehee Yoo
2022-04-01 11:00 ` patchwork-bot+netdevbpf
2022-04-01 11:06 ` Martin Habets
2022-04-01 18:48 ` Taehee Yoo
2022-04-04 10:36 ` Martin Habets
2022-04-05 3:16 ` Taehee Yoo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).