Skip to contentSkip to conversation

mlxsw: Fixes

PATCH Applied Petr Machata 4 participants 12 messages Oct 25, 2024 – Oct 31, 2024

Subsystems (5)
Networking Network drivers Kselftest IPv4/IPv6 +1 more subsystems
lore
Share & tools
lore

Petr Machata

author maintainer
#0 lore raw

cover mlxsw: Fixes

In this patchset:

- Tx header should be pushed for each packet which is transmitted via Spectrum ASICs. Patch #1 adds a missing call to skb_cow_head() to make sure that there is both enough room to push the Tx header and that the SKB header is not cloned and can be modified.

- Commit b5b60bb491b2 ("mlxsw: pci: Use page pool for Rx buffers allocation") converted mlxsw to use page pool for Rx buffers allocation. Sync for CPU and for device should be done for Rx pages. In patches #2 and #3, add the missing calls to sync pages for, respectively, CPU and the device.

- Patch #4 then fixes a bug to IPv6 GRE forwarding offload. Patch #5 adds a generic forwarding test that fails with mlxsw ports prior to the fix.

Amit Cohen (3):
mlxsw: spectrum_ptp: Add missing verification before pushing Tx header
mlxsw: pci: Sync Rx buffers for CPU
mlxsw: pci: Sync Rx buffers for device

Ido Schimmel (2):
mlxsw: spectrum_ipip: Fix memory leak when changing remote IPv6
address
selftests: forwarding: Add IPv6 GRE remote change tests

 drivers/net/ethernet/mellanox/mlxsw/pci.c     | 25 ++++--
 .../ethernet/mellanox/mlxsw/spectrum_ipip.c   | 26 +++++-
 .../ethernet/mellanox/mlxsw/spectrum_ptp.c    |  7 ++
 .../selftests/net/forwarding/ip6gre_flat.sh   | 14 ++++
 .../net/forwarding/ip6gre_flat_key.sh         | 14 ++++
 .../net/forwarding/ip6gre_flat_keys.sh        | 14 ++++
 .../selftests/net/forwarding/ip6gre_hier.sh   | 14 ++++
 .../net/forwarding/ip6gre_hier_key.sh         | 14 ++++
 .../net/forwarding/ip6gre_hier_keys.sh        | 14 ++++
 .../selftests/net/forwarding/ip6gre_lib.sh    | 80 +++++++++++++++++++
 10 files changed, 212 insertions(+), 10 deletions(-)
Signature · 1 line
2.45.0
author maintainer
#1 lore raw

PATCH 1/5 mlxsw: spectrum_ptp: Add missing verification before pushing Tx header

From: Amit Cohen <[email protected]>

Tx header should be pushed for each packet which is transmitted via Spectrum ASICs. The cited commit moved the call to skb_cow_head() from mlxsw_sp_port_xmit() to functions which handle Tx header.

In case that mlxsw_sp->ptp_ops->txhdr_construct() is used to handle Tx header, and txhdr_construct() is mlxsw_sp_ptp_txhdr_construct(), there is no call for skb_cow_head() before pushing Tx header size to SKB. This flow is relevant for Spectrum-1 and Spectrum-4, for PTP packets.

Add the missing call to skb_cow_head() to make sure that there is both enough room to push the Tx header and that the SKB header is not cloned and can be modified.

An additional set will be sent to net-next to centralize the handling of the Tx header by pushing it to every packet just before transmission.

Cc Richard Cochran <[email protected]>
Fixes 24157bc69f45 ("mlxsw: Send PTP packets as data packets to overcome a limitation")
Signed-off-by Amit Cohen <[email protected]>
Signed-off-by Petr Machata <[email protected]>
---
 drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.c | 7 +++++++
 1 file changed, 7 insertions(+)
drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.c +7 -0
index 5b174cb95eb8..d94081c7658e 100644
@@ -16,6 +16,7 @@
 #include "spectrum.h"
 #include "spectrum_ptp.h"
 #include "core.h"
+#include "txheader.h"
 
 #define MLXSW_SP1_PTP_CLOCK_CYCLES_SHIFT	29
 #define MLXSW_SP1_PTP_CLOCK_FREQ_KHZ		156257 /* 6.4nSec */

@@ -1684,6 +1685,12 @@ int mlxsw_sp_ptp_txhdr_construct(struct mlxsw_core *mlxsw_core,
 				 struct sk_buff *skb,
 				 const struct mlxsw_tx_info *tx_info)
 {
+	if (skb_cow_head(skb, MLXSW_TXHDR_LEN)) {
+		this_cpu_inc(mlxsw_sp_port->pcpu_stats->tx_dropped);
+		dev_kfree_skb_any(skb);
+		return -ENOMEM;
+	}
+
 	mlxsw_sp_txhdr_construct(skb, tx_info);
 	return 0;
 }
Signature · 1 line
2.45.0
author maintainer
#2 lore raw

PATCH 2/5 mlxsw: pci: Sync Rx buffers for CPU

In reply to Petr Machata

From: Amit Cohen <[email protected]>

When Rx packet is received, drivers should sync the pages for CPU, to ensure the CPU reads the data written by the device and not stale data from its cache.

Add the missing sync call in Rx path, sync the actual length of data for each fragment.

Cc Jiri Pirko <[email protected]>
Fixes b5b60bb491b2 ("mlxsw: pci: Use page pool for Rx buffers allocation")
Signed-off-by Amit Cohen <[email protected]>
Reviewed-by Ido Schimmel <[email protected]>
Signed-off-by Petr Machata <[email protected]>
---
 drivers/net/ethernet/mellanox/mlxsw/pci.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)
drivers/net/ethernet/mellanox/mlxsw/pci.c +15 -7
index 060e5b939211..2320a5f323b4 100644
@@ -389,15 +389,27 @@ static void mlxsw_pci_wqe_frag_unmap(struct mlxsw_pci *mlxsw_pci, char *wqe,
 	dma_unmap_single(&pdev->dev, mapaddr, frag_len, direction);
 }
 
-static struct sk_buff *mlxsw_pci_rdq_build_skb(struct page *pages[],
+static struct sk_buff *mlxsw_pci_rdq_build_skb(struct mlxsw_pci_queue *q,
+					       struct page *pages[],
 					       u16 byte_count)
 {
+	struct mlxsw_pci_queue *cq = q->u.rdq.cq;
 	unsigned int linear_data_size;
+	struct page_pool *page_pool;
 	struct sk_buff *skb;
 	int page_index = 0;
 	bool linear_only;
 	void *data;
 
+	linear_only = byte_count + MLXSW_PCI_RX_BUF_SW_OVERHEAD <= PAGE_SIZE;
+	linear_data_size = linear_only ? byte_count :
+					 PAGE_SIZE -
+					 MLXSW_PCI_RX_BUF_SW_OVERHEAD;
+
+	page_pool = cq->u.cq.page_pool;
+	page_pool_dma_sync_for_cpu(page_pool, pages[page_index],
+				   MLXSW_PCI_SKB_HEADROOM, linear_data_size);
+
 	data = page_address(pages[page_index]);
 	net_prefetch(data);
 

@@ -405,11 +417,6 @@ static struct sk_buff *mlxsw_pci_rdq_build_skb(struct page *pages[],
 	if (unlikely(!skb))
 		return ERR_PTR(-ENOMEM);
 
-	linear_only = byte_count + MLXSW_PCI_RX_BUF_SW_OVERHEAD <= PAGE_SIZE;
-	linear_data_size = linear_only ? byte_count :
-					 PAGE_SIZE -
-					 MLXSW_PCI_RX_BUF_SW_OVERHEAD;
-
 	skb_reserve(skb, MLXSW_PCI_SKB_HEADROOM);
 	skb_put(skb, linear_data_size);
 

@@ -425,6 +432,7 @@ static struct sk_buff *mlxsw_pci_rdq_build_skb(struct page *pages[],
 
 		page = pages[page_index];
 		frag_size = min(byte_count, PAGE_SIZE);
+		page_pool_dma_sync_for_cpu(page_pool, page, 0, frag_size);
 		skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags,
 				page, 0, frag_size, PAGE_SIZE);
 		byte_count -= frag_size;

@@ -760,7 +768,7 @@ static void mlxsw_pci_cqe_rdq_handle(struct mlxsw_pci *mlxsw_pci,
 	if (err)
 		goto out;
 
-	skb = mlxsw_pci_rdq_build_skb(pages, byte_count);
+	skb = mlxsw_pci_rdq_build_skb(q, pages, byte_count);
 	if (IS_ERR(skb)) {
 		dev_err_ratelimited(&pdev->dev, "Failed to build skb for RDQ\n");
 		mlxsw_pci_rdq_pages_recycle(q, pages, num_sg_entries);
Signature · 1 line
2.45.0
#3 lore raw

Re: [PATCH net 2/5] mlxsw: pci: Sync Rx buffers for CPU

From: Petr Machata <[email protected]>
Date: Fri, 25 Oct 2024 16:26:26 +0200

Quoted 5 lines

From: Amit Cohen <[email protected]>

When Rx packet is received, drivers should sync the pages for CPU, to ensure the CPU reads the data written by the device and not stale data from its cache.

[...]

Quoted 7 lines
drivers/net/ethernet/mellanox/mlxsw/pci.c +3 -1
-static struct sk_buff *mlxsw_pci_rdq_build_skb(struct page *pages[],
+static struct sk_buff *mlxsw_pci_rdq_build_skb(struct mlxsw_pci_queue *q,
+					       struct page *pages[],
 					       u16 byte_count)
 {
+	struct mlxsw_pci_queue *cq = q->u.rdq.cq;
 	unsigned int linear_data_size;

drivers/net/ethernet/mellanox/mlxsw/pci.c static void mlxsw_pci_wqe_frag_unmap(struct mlxsw_pci *mlxsw_pci, char *wqe, · line 407

Quoted 10 lines
drivers/net/ethernet/mellanox/mlxsw/pci.c +5 -0
+	struct page_pool *page_pool;
 	struct sk_buff *skb;
 	int page_index = 0;
 	bool linear_only;
 	void *data;
 
+	linear_only = byte_count + MLXSW_PCI_RX_BUF_SW_OVERHEAD <= PAGE_SIZE;
+	linear_data_size = linear_only ? byte_count :
+					 PAGE_SIZE -
+					 MLXSW_PCI_RX_BUF_SW_OVERHEAD;

Maybe reformat the line while at it?

	linear_data_size = linear_only ? byte_count :
			   PAGE_SIZE - MLXSW_PCI_RX_BUF_SW_OVERHEAD;

drivers/net/ethernet/mellanox/mlxsw/pci.c static void mlxsw_pci_wqe_frag_unmap(struct mlxsw_pci *mlxsw_pci, char *wqe, · line 411

Quoted 4 lines
drivers/net/ethernet/mellanox/mlxsw/pci.c +4 -0
+
+	page_pool = cq->u.cq.page_pool;
+	page_pool_dma_sync_for_cpu(page_pool, pages[page_index],
+				   MLXSW_PCI_SKB_HEADROOM, linear_data_size);

page_pool_dma_sync_for_cpu() already skips the headroom:

	dma_sync_single_range_for_cpu(pool->p.dev,
				      offset + pool->p.offset, ...

Since your pool->p.offset is MLXSW_PCI_SKB_HEADROOM, I believe you need to pass 0 here.

drivers/net/ethernet/mellanox/mlxsw/pci.c static void mlxsw_pci_wqe_frag_unmap(struct mlxsw_pci *mlxsw_pci, char *wqe, · line 414

Quoted 3 lines
drivers/net/ethernet/mellanox/mlxsw/pci.c +1 -0
+
 	data = page_address(pages[page_index]);
 	net_prefetch(data);

Thanks,
Olek

#4 lore raw

RE: [PATCH net 2/5] mlxsw: pci: Sync Rx buffers for CPU

Quoted 52 lines

-----Original Message-----
From: Alexander Lobakin <[email protected]>
Sent: Friday, 25 October 2024 18:00
To: Petr Machata <[email protected]>; Amit Cohen <[email protected]>
Cc: [email protected]; Andrew Lunn <[email protected]>; David S. Miller <[email protected]>; Eric Dumazet <[email protected]>; Jakub Kicinski <[email protected]>; Paolo Abeni <[email protected]>; Simon Horman <[email protected]>; Danielle Ratson <[email protected]>; Ido Schimmel <[email protected]>; mlxsw <[email protected]>; Jiri Pirko <[email protected]>
Subject: Re: [PATCH net 2/5] mlxsw: pci: Sync Rx buffers for CPU

From: Petr Machata <[email protected]>
Date: Fri, 25 Oct 2024 16:26:26 +0200

Quoted 5 lines

From: Amit Cohen <[email protected]>

When Rx packet is received, drivers should sync the pages for CPU, to ensure the CPU reads the data written by the device and not stale data from its cache.

[...]

Quoted 7 lines
drivers/net/ethernet/mellanox/mlxsw/pci.c +3 -1
-static struct sk_buff *mlxsw_pci_rdq_build_skb(struct page *pages[],
+static struct sk_buff *mlxsw_pci_rdq_build_skb(struct mlxsw_pci_queue *q,
+					       struct page *pages[],
 					       u16 byte_count)
 {
+	struct mlxsw_pci_queue *cq = q->u.rdq.cq;
 	unsigned int linear_data_size;

drivers/net/ethernet/mellanox/mlxsw/pci.c static void mlxsw_pci_wqe_frag_unmap(struct mlxsw_pci *mlxsw_pci, char *wqe, · line 407

Quoted 10 lines
drivers/net/ethernet/mellanox/mlxsw/pci.c +5 -0
+	struct page_pool *page_pool;
 	struct sk_buff *skb;
 	int page_index = 0;
 	bool linear_only;
 	void *data;
 
+	linear_only = byte_count + MLXSW_PCI_RX_BUF_SW_OVERHEAD <= PAGE_SIZE;
+	linear_data_size = linear_only ? byte_count :
+					 PAGE_SIZE -
+					 MLXSW_PCI_RX_BUF_SW_OVERHEAD;

Maybe reformat the line while at it?

	linear_data_size = linear_only ? byte_count :
			   PAGE_SIZE - MLXSW_PCI_RX_BUF_SW_OVERHEAD;

drivers/net/ethernet/mellanox/mlxsw/pci.c static void mlxsw_pci_wqe_frag_unmap(struct mlxsw_pci *mlxsw_pci, char *wqe, · line 411

Quoted 4 lines
drivers/net/ethernet/mellanox/mlxsw/pci.c +4 -0
+
+	page_pool = cq->u.cq.page_pool;
+	page_pool_dma_sync_for_cpu(page_pool, pages[page_index],
+				   MLXSW_PCI_SKB_HEADROOM, linear_data_size);

page_pool_dma_sync_for_cpu() already skips the headroom:

Quoted 5 lines
	dma_sync_single_range_for_cpu(pool->p.dev,
				      offset + pool->p.offset, ...

Since your pool->p.offset is MLXSW_PCI_SKB_HEADROOM, I believe you need to pass 0 here.

Our pool->p.offset is zero.
We use the page pool to allocate buffers for scatter/gather entries.
Only the first entry saves headroom for software usage, so only for the first buffer of the packet we pass headroom to page_pool_dma_sync_for_cpu().

Quoted 7 lines

drivers/net/ethernet/mellanox/mlxsw/pci.c static void mlxsw_pci_wqe_frag_unmap(struct mlxsw_pci *mlxsw_pci, char *wqe, · line 414

Quoted 4 lines
drivers/net/ethernet/mellanox/mlxsw/pci.c +1 -0
+
 	data = page_address(pages[page_index]);
 	net_prefetch(data);

Thanks,
Olek

author maintainer
#5 lore raw

PATCH 3/5 mlxsw: pci: Sync Rx buffers for device

In reply to Petr Machata

From: Amit Cohen <[email protected]>

Non-coherent architectures, like ARM, may require invalidating caches before the device can use the DMA mapped memory, which means that before posting pages to device, drivers should sync the memory for device.

Sync for device can be configured as page pool responsibility. Set the relevant flag and define max_len for sync.

Cc Jiri Pirko <[email protected]>
Fixes b5b60bb491b2 ("mlxsw: pci: Use page pool for Rx buffers allocation")
Signed-off-by Amit Cohen <[email protected]>
Reviewed-by Ido Schimmel <[email protected]>
Signed-off-by Petr Machata <[email protected]>
---
 drivers/net/ethernet/mellanox/mlxsw/pci.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/net/ethernet/mellanox/mlxsw/pci.c +2 -1
index 2320a5f323b4..d6f37456fb31 100644
@@ -996,12 +996,13 @@ static int mlxsw_pci_cq_page_pool_init(struct mlxsw_pci_queue *q,
 	if (cq_type != MLXSW_PCI_CQ_RDQ)
 		return 0;
 
-	pp_params.flags = PP_FLAG_DMA_MAP;
+	pp_params.flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV;
 	pp_params.pool_size = MLXSW_PCI_WQE_COUNT * mlxsw_pci->num_sg_entries;
 	pp_params.nid = dev_to_node(&mlxsw_pci->pdev->dev);
 	pp_params.dev = &mlxsw_pci->pdev->dev;
 	pp_params.napi = &q->u.cq.napi;
 	pp_params.dma_dir = DMA_FROM_DEVICE;
+	pp_params.max_len = PAGE_SIZE;
 
 	page_pool = page_pool_create(&pp_params);
 	if (IS_ERR(page_pool))
Signature · 1 line
2.45.0
#6 lore raw

Re: [PATCH net 3/5] mlxsw: pci: Sync Rx buffers for device

From: Petr Machata <[email protected]>
Date: Fri, 25 Oct 2024 16:26:27 +0200

Quoted 24 lines

From: Amit Cohen <[email protected]>

Non-coherent architectures, like ARM, may require invalidating caches before the device can use the DMA mapped memory, which means that before posting pages to device, drivers should sync the memory for device.

Sync for device can be configured as page pool responsibility. Set the relevant flag and define max_len for sync.

Cc Jiri Pirko <[email protected]>
Fixes b5b60bb491b2 ("mlxsw: pci: Use page pool for Rx buffers allocation")
Signed-off-by Amit Cohen <[email protected]>
Reviewed-by Ido Schimmel <[email protected]>
Signed-off-by Petr Machata <[email protected]>
---
 drivers/net/ethernet/mellanox/mlxsw/pci.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/net/ethernet/mellanox/mlxsw/pci.c +0 -0
index 2320a5f323b4..d6f37456fb31 100644
@@ -996,12 +996,13 @@ static int mlxsw_pci_cq_page_pool_init(struct mlxsw_pci_queue *q,
 	if (cq_type != MLXSW_PCI_CQ_RDQ)

drivers/net/ethernet/mellanox/mlxsw/pci.c static int mlxsw_pci_cq_page_pool_init(struct mlxsw_pci_queue *q, · line 1005

Quoted 10 lines
drivers/net/ethernet/mellanox/mlxsw/pci.c +2 -1
 		return 0;
 
-	pp_params.flags = PP_FLAG_DMA_MAP;
+	pp_params.flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV;
 	pp_params.pool_size = MLXSW_PCI_WQE_COUNT * mlxsw_pci->num_sg_entries;
 	pp_params.nid = dev_to_node(&mlxsw_pci->pdev->dev);
 	pp_params.dev = &mlxsw_pci->pdev->dev;
 	pp_params.napi = &q->u.cq.napi;
 	pp_params.dma_dir = DMA_FROM_DEVICE;
+	pp_params.max_len = PAGE_SIZE;

max_len is the maximum HW-writable area of a buffer. Headroom and tailroom must be excluded. In your case

	pp_params.max_len = PAGE_SIZE - MLXSW_PCI_RX_BUF_SW_OVERHEAD;

drivers/net/ethernet/mellanox/mlxsw/pci.c static int mlxsw_pci_cq_page_pool_init(struct mlxsw_pci_queue *q, · line 1008

Quoted 3 lines
drivers/net/ethernet/mellanox/mlxsw/pci.c +0 -0
 	page_pool = page_pool_create(&pp_params);
 	if (IS_ERR(page_pool))

Thanks,
Olek

#7 lore raw

RE: [PATCH net 3/5] mlxsw: pci: Sync Rx buffers for device

Quoted 52 lines

-----Original Message-----
From: Alexander Lobakin <[email protected]>
Sent: Friday, 25 October 2024 18:03
To: Petr Machata <[email protected]>; Amit Cohen <[email protected]>
Cc: [email protected]; Andrew Lunn <[email protected]>; David S. Miller <[email protected]>; Eric Dumazet <[email protected]>; Jakub Kicinski <[email protected]>; Paolo Abeni <[email protected]>; Simon Horman <[email protected]>; Danielle Ratson <[email protected]>; Ido Schimmel <[email protected]>; mlxsw <[email protected]>; Jiri Pirko <[email protected]>
Subject: Re: [PATCH net 3/5] mlxsw: pci: Sync Rx buffers for device

From: Petr Machata <[email protected]>
Date: Fri, 25 Oct 2024 16:26:27 +0200

Quoted 24 lines

From: Amit Cohen <[email protected]>

Non-coherent architectures, like ARM, may require invalidating caches before the device can use the DMA mapped memory, which means that before posting pages to device, drivers should sync the memory for device.

Sync for device can be configured as page pool responsibility. Set the relevant flag and define max_len for sync.

Cc Jiri Pirko <[email protected]>
Fixes b5b60bb491b2 ("mlxsw: pci: Use page pool for Rx buffers allocation")
Signed-off-by Amit Cohen <[email protected]>
Reviewed-by Ido Schimmel <[email protected]>
Signed-off-by Petr Machata <[email protected]>
---
 drivers/net/ethernet/mellanox/mlxsw/pci.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/net/ethernet/mellanox/mlxsw/pci.c +0 -0
index 2320a5f323b4..d6f37456fb31 100644
@@ -996,12 +996,13 @@ static int mlxsw_pci_cq_page_pool_init(struct mlxsw_pci_queue *q,
 	if (cq_type != MLXSW_PCI_CQ_RDQ)

drivers/net/ethernet/mellanox/mlxsw/pci.c static int mlxsw_pci_cq_page_pool_init(struct mlxsw_pci_queue *q, · line 1005

Quoted 10 lines
drivers/net/ethernet/mellanox/mlxsw/pci.c +2 -1
 		return 0;
 
-	pp_params.flags = PP_FLAG_DMA_MAP;
+	pp_params.flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV;
 	pp_params.pool_size = MLXSW_PCI_WQE_COUNT * mlxsw_pci->num_sg_entries;
 	pp_params.nid = dev_to_node(&mlxsw_pci->pdev->dev);
 	pp_params.dev = &mlxsw_pci->pdev->dev;
 	pp_params.napi = &q->u.cq.napi;
 	pp_params.dma_dir = DMA_FROM_DEVICE;
+	pp_params.max_len = PAGE_SIZE;

max_len is the maximum HW-writable area of a buffer. Headroom and tailroom must be excluded. In your case

Quoted 2 lines
	pp_params.max_len = PAGE_SIZE - MLXSW_PCI_RX_BUF_SW_OVERHEAD;

mlxsw driver uses fragmented buffers and the page pool is used to allocate the buffers for all scatter/gather entries.
For each packet, the HW-writable area of a buffer of the *first* entry is 'PAGE_SIZE - MLXSW_PCI_RX_BUF_SW_OVERHEAD', but for other entries we map PAGE_SIZE to HW. That's why we set page pool to sync PAGE_SIZE and use offset=0.

Quoted 6 lines

drivers/net/ethernet/mellanox/mlxsw/pci.c static int mlxsw_pci_cq_page_pool_init(struct mlxsw_pci_queue *q, · line 1008

Quoted 3 lines
drivers/net/ethernet/mellanox/mlxsw/pci.c +0 -0
 	page_pool = page_pool_create(&pp_params);
 	if (IS_ERR(page_pool))

Thanks,
Olek

#8 lore raw

Re: [PATCH net 3/5] mlxsw: pci: Sync Rx buffers for device

From: Amit Cohen <[email protected]>
Date: Sun, 27 Oct 2024 06:51:00 +0000

Quoted 52 lines
Quoted 52 lines

-----Original Message-----
From: Alexander Lobakin <[email protected]>
Sent: Friday, 25 October 2024 18:03
To: Petr Machata <[email protected]>; Amit Cohen <[email protected]>
Cc: [email protected]; Andrew Lunn <[email protected]>; David S. Miller <[email protected]>; Eric Dumazet <[email protected]>; Jakub Kicinski <[email protected]>; Paolo Abeni <[email protected]>; Simon Horman <[email protected]>; Danielle Ratson <[email protected]>; Ido Schimmel <[email protected]>; mlxsw <[email protected]>; Jiri Pirko <[email protected]>
Subject: Re: [PATCH net 3/5] mlxsw: pci: Sync Rx buffers for device

From: Petr Machata <[email protected]>
Date: Fri, 25 Oct 2024 16:26:27 +0200

Quoted 24 lines

From: Amit Cohen <[email protected]>

Non-coherent architectures, like ARM, may require invalidating caches before the device can use the DMA mapped memory, which means that before posting pages to device, drivers should sync the memory for device.

Sync for device can be configured as page pool responsibility. Set the relevant flag and define max_len for sync.

Cc Jiri Pirko <[email protected]>
Fixes b5b60bb491b2 ("mlxsw: pci: Use page pool for Rx buffers allocation")
Signed-off-by Amit Cohen <[email protected]>
Reviewed-by Ido Schimmel <[email protected]>
Signed-off-by Petr Machata <[email protected]>
---
 drivers/net/ethernet/mellanox/mlxsw/pci.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/net/ethernet/mellanox/mlxsw/pci.c +0 -0
index 2320a5f323b4..d6f37456fb31 100644
@@ -996,12 +996,13 @@ static int mlxsw_pci_cq_page_pool_init(struct mlxsw_pci_queue *q,
 	if (cq_type != MLXSW_PCI_CQ_RDQ)

drivers/net/ethernet/mellanox/mlxsw/pci.c static int mlxsw_pci_cq_page_pool_init(struct mlxsw_pci_queue *q, · line 1005

Quoted 10 lines
drivers/net/ethernet/mellanox/mlxsw/pci.c +2 -1
 		return 0;
 
-	pp_params.flags = PP_FLAG_DMA_MAP;
+	pp_params.flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV;
 	pp_params.pool_size = MLXSW_PCI_WQE_COUNT * mlxsw_pci->num_sg_entries;
 	pp_params.nid = dev_to_node(&mlxsw_pci->pdev->dev);
 	pp_params.dev = &mlxsw_pci->pdev->dev;
 	pp_params.napi = &q->u.cq.napi;
 	pp_params.dma_dir = DMA_FROM_DEVICE;
+	pp_params.max_len = PAGE_SIZE;

max_len is the maximum HW-writable area of a buffer. Headroom and tailroom must be excluded. In your case

Quoted 6 lines
Quoted 2 lines
	pp_params.max_len = PAGE_SIZE - MLXSW_PCI_RX_BUF_SW_OVERHEAD;

mlxsw driver uses fragmented buffers and the page pool is used to allocate the buffers for all scatter/gather entries.
For each packet, the HW-writable area of a buffer of the *first* entry is 'PAGE_SIZE - MLXSW_PCI_RX_BUF_SW_OVERHEAD', but for other entries we map PAGE_SIZE to HW. That's why we set page pool to sync PAGE_SIZE and use offset=0.

Ooops, I didn't notice this particular configuration has offset == 0, sorry.

Thanks,
Olek

author maintainer
#9 lore raw

PATCH 4/5 mlxsw: spectrum_ipip: Fix memory leak when changing remote IPv6 address

In reply to Petr Machata

From: Ido Schimmel <[email protected]>

The device stores IPv6 addresses that are used for encapsulation in linear memory that is managed by the driver.

Changing the remote address of an ip6gre net device never worked properly, but since cited commit the following reproducer [1] would result in a warning [2] and a memory leak [3]. The problem is that the new remote address is never added by the driver to its hash table (and therefore the device) and the old address is never removed from it.

Fix by programming the new address when the configuration of the ip6gre net device changes and removing the old one. If the address did not change, then the above would result in increasing the reference count of the address and then decreasing it.

[1]

 # ip link add name bla up type ip6gre local 2001:db8:1::1 remote 2001:db8:2::1 tos inherit ttl inherit
 # ip link set dev bla type ip6gre remote 2001:db8:3::1
 # ip link del dev bla
 # devlink dev reload pci/0000:01:00.0

[2]

WARNING: CPU: 0 PID: 1682 at drivers/net/ethernet/mellanox/mlxsw/spectrum.c:3002 mlxsw_sp_ipv6_addr_put+0x140/0x1d0
Modules linked in:
CPU: 0 UID: 0 PID: 1682 Comm: ip Not tainted 6.12.0-rc3-custom-g86b5b55bc835 #151
Hardware name: Nvidia SN5600/VMOD0013, BIOS 5.13 05/31/2023
RIP: 0010:mlxsw_sp_ipv6_addr_put+0x140/0x1d0
[...]
Call Trace:
 <TASK>
 mlxsw_sp_router_netdevice_event+0x55f/0x1240
 notifier_call_chain+0x5a/0xd0
 call_netdevice_notifiers_info+0x39/0x90
 unregister_netdevice_many_notify+0x63e/0x9d0
 rtnl_dellink+0x16b/0x3a0
 rtnetlink_rcv_msg+0x142/0x3f0
 netlink_rcv_skb+0x50/0x100
 netlink_unicast+0x242/0x390
 netlink_sendmsg+0x1de/0x420
 ____sys_sendmsg+0x2bd/0x320
 ___sys_sendmsg+0x9a/0xe0
 __sys_sendmsg+0x7a/0xd0
 do_syscall_64+0x9e/0x1a0
 entry_SYSCALL_64_after_hwframe+0x77/0x7f

[3]
unreferenced object 0xffff898081f597a0 (size 32):
comm "ip", pid 1626, jiffies 4294719324
hex dump (first 32 bytes):

    20 01 0d b8 00 02 00 00 00 00 00 00 00 00 00 01   ...............
    21 49 61 83 80 89 ff ff 00 00 00 00 01 00 00 00  !Ia.............
  backtrace (crc fd9be911):
    [<00000000df89c55d>] __kmalloc_cache_noprof+0x1da/0x260
    [<00000000ff2a1ddb>] mlxsw_sp_ipv6_addr_kvdl_index_get+0x281/0x340
    [<000000009ddd445d>] mlxsw_sp_router_netdevice_event+0x47b/0x1240
    [<00000000743e7757>] notifier_call_chain+0x5a/0xd0
    [<000000007c7b9e13>] call_netdevice_notifiers_info+0x39/0x90
    [<000000002509645d>] register_netdevice+0x5f7/0x7a0
    [<00000000c2e7d2a9>] ip6gre_newlink_common.isra.0+0x65/0x130
    [<0000000087cd6d8d>] ip6gre_newlink+0x72/0x120
    [<000000004df7c7cc>] rtnl_newlink+0x471/0xa20
    [<0000000057ed632a>] rtnetlink_rcv_msg+0x142/0x3f0
    [<0000000032e0d5b5>] netlink_rcv_skb+0x50/0x100
    [<00000000908bca63>] netlink_unicast+0x242/0x390
    [<00000000cdbe1c87>] netlink_sendmsg+0x1de/0x420
    [<0000000011db153e>] ____sys_sendmsg+0x2bd/0x320
    [<000000003b6d53eb>] ___sys_sendmsg+0x9a/0xe0
    [<00000000cae27c62>] __sys_sendmsg+0x7a/0xd0
Fixes cf42911523e0 ("mlxsw: spectrum_ipip: Use common hash table for IPv6 address mapping")
Reported-by Maksym Yaremchuk <[email protected]>
Signed-off-by Ido Schimmel <[email protected]>
Reviewed-by Petr Machata <[email protected]>
Signed-off-by Petr Machata <[email protected]>
---
 .../ethernet/mellanox/mlxsw/spectrum_ipip.c   | 26 +++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)
drivers/net/ethernet/mellanox/mlxsw/spectrum_ipip.c +24 -2
index d761a1235994..7ea798a4949e 100644
@@ -481,11 +481,33 @@ mlxsw_sp_ipip_ol_netdev_change_gre6(struct mlxsw_sp *mlxsw_sp,
 				    struct mlxsw_sp_ipip_entry *ipip_entry,
 				    struct netlink_ext_ack *extack)
 {
+	u32 new_kvdl_index, old_kvdl_index = ipip_entry->dip_kvdl_index;
+	struct in6_addr old_addr6 = ipip_entry->parms.daddr.addr6;
 	struct mlxsw_sp_ipip_parms new_parms;
+	int err;
 
 	new_parms = mlxsw_sp_ipip_netdev_parms_init_gre6(ipip_entry->ol_dev);
-	return mlxsw_sp_ipip_ol_netdev_change_gre(mlxsw_sp, ipip_entry,
-						  &new_parms, extack);
+
+	err = mlxsw_sp_ipv6_addr_kvdl_index_get(mlxsw_sp,
+						&new_parms.daddr.addr6,
+						&new_kvdl_index);
+	if (err)
+		return err;
+	ipip_entry->dip_kvdl_index = new_kvdl_index;
+
+	err = mlxsw_sp_ipip_ol_netdev_change_gre(mlxsw_sp, ipip_entry,
+						 &new_parms, extack);
+	if (err)
+		goto err_change_gre;
+
+	mlxsw_sp_ipv6_addr_put(mlxsw_sp, &old_addr6);
+
+	return 0;
+
+err_change_gre:
+	ipip_entry->dip_kvdl_index = old_kvdl_index;
+	mlxsw_sp_ipv6_addr_put(mlxsw_sp, &new_parms.daddr.addr6);
+	return err;
 }
 
 static int
Signature · 1 line
2.45.0
author maintainer
#10 lore raw

PATCH 5/5 selftests: forwarding: Add IPv6 GRE remote change tests

In reply to Petr Machata

From: Ido Schimmel <[email protected]>

Test that after changing the remote address of an ip6gre net device traffic is forwarded as expected. Test with both flat and hierarchical topologies and with and without an input / output keys.

Signed-off-by Ido Schimmel <[email protected]>
Reviewed-by Petr Machata <[email protected]>
Signed-off-by Petr Machata <[email protected]>
---
 .../selftests/net/forwarding/ip6gre_flat.sh   | 14 ++++
 .../net/forwarding/ip6gre_flat_key.sh         | 14 ++++
 .../net/forwarding/ip6gre_flat_keys.sh        | 14 ++++
 .../selftests/net/forwarding/ip6gre_hier.sh   | 14 ++++
 .../net/forwarding/ip6gre_hier_key.sh         | 14 ++++
 .../net/forwarding/ip6gre_hier_keys.sh        | 14 ++++
 .../selftests/net/forwarding/ip6gre_lib.sh    | 80 +++++++++++++++++++
 7 files changed, 164 insertions(+)
tools/testing/selftests/net/forwarding/ip6gre_flat.sh +14 -0
index 96c97064f2d3..becc7c3fc809 100755
@@ -8,6 +8,7 @@
 ALL_TESTS="
 	gre_flat
 	gre_mtu_change
+	gre_flat_remote_change
 "
 
 NUM_NETIFS=6

@@ -44,6 +45,19 @@ gre_mtu_change()
 	test_mtu_change
 }
 
+gre_flat_remote_change()
+{
+	flat_remote_change
+
+	test_traffic_ip4ip6 "GRE flat IPv4-in-IPv6 (new remote)"
+	test_traffic_ip6ip6 "GRE flat IPv6-in-IPv6 (new remote)"
+
+	flat_remote_restore
+
+	test_traffic_ip4ip6 "GRE flat IPv4-in-IPv6 (old remote)"
+	test_traffic_ip6ip6 "GRE flat IPv6-in-IPv6 (old remote)"
+}
+
 cleanup()
 {
 	pre_cleanup
tools/testing/selftests/net/forwarding/ip6gre_flat_key.sh +14 -0
index ff9fb0db9bd1..e5335116a2fd 100755
@@ -8,6 +8,7 @@
 ALL_TESTS="
 	gre_flat
 	gre_mtu_change
+	gre_flat_remote_change
 "
 
 NUM_NETIFS=6

@@ -44,6 +45,19 @@ gre_mtu_change()
 	test_mtu_change
 }
 
+gre_flat_remote_change()
+{
+	flat_remote_change
+
+	test_traffic_ip4ip6 "GRE flat IPv4-in-IPv6 with key (new remote)"
+	test_traffic_ip6ip6 "GRE flat IPv6-in-IPv6 with key (new remote)"
+
+	flat_remote_restore
+
+	test_traffic_ip4ip6 "GRE flat IPv4-in-IPv6 with key (old remote)"
+	test_traffic_ip6ip6 "GRE flat IPv6-in-IPv6 with key (old remote)"
+}
+
 cleanup()
 {
 	pre_cleanup
tools/testing/selftests/net/forwarding/ip6gre_flat_keys.sh +14 -0
index 12c138785242..7e0cbfdefab0 100755
@@ -8,6 +8,7 @@
 ALL_TESTS="
 	gre_flat
 	gre_mtu_change
+	gre_flat_remote_change
 "
 
 NUM_NETIFS=6

@@ -44,6 +45,19 @@ gre_mtu_change()
 	test_mtu_change	gre
 }
 
+gre_flat_remote_change()
+{
+	flat_remote_change
+
+	test_traffic_ip4ip6 "GRE flat IPv4-in-IPv6 with ikey/okey (new remote)"
+	test_traffic_ip6ip6 "GRE flat IPv6-in-IPv6 with ikey/okey (new remote)"
+
+	flat_remote_restore
+
+	test_traffic_ip4ip6 "GRE flat IPv4-in-IPv6 with ikey/okey (old remote)"
+	test_traffic_ip6ip6 "GRE flat IPv6-in-IPv6 with ikey/okey (old remote)"
+}
+
 cleanup()
 {
 	pre_cleanup
tools/testing/selftests/net/forwarding/ip6gre_hier.sh +14 -0
index 83b55c30a5c3..e0844495f3d1 100755
@@ -8,6 +8,7 @@
 ALL_TESTS="
 	gre_hier
 	gre_mtu_change
+	gre_hier_remote_change
 "
 
 NUM_NETIFS=6

@@ -44,6 +45,19 @@ gre_mtu_change()
 	test_mtu_change gre
 }
 
+gre_hier_remote_change()
+{
+	hier_remote_change
+
+	test_traffic_ip4ip6 "GRE hierarchical IPv4-in-IPv6 (new remote)"
+	test_traffic_ip6ip6 "GRE hierarchical IPv6-in-IPv6 (new remote)"
+
+	hier_remote_restore
+
+	test_traffic_ip4ip6 "GRE hierarchical IPv4-in-IPv6 (old remote)"
+	test_traffic_ip6ip6 "GRE hierarchical IPv6-in-IPv6 (old remote)"
+}
+
 cleanup()
 {
 	pre_cleanup
tools/testing/selftests/net/forwarding/ip6gre_hier_key.sh +14 -0
index 256607916d92..741bc9c928eb 100755
@@ -8,6 +8,7 @@
 ALL_TESTS="
 	gre_hier
 	gre_mtu_change
+	gre_hier_remote_change
 "
 
 NUM_NETIFS=6

@@ -44,6 +45,19 @@ gre_mtu_change()
 	test_mtu_change gre
 }
 
+gre_hier_remote_change()
+{
+	hier_remote_change
+
+	test_traffic_ip4ip6 "GRE hierarchical IPv4-in-IPv6 with key (new remote)"
+	test_traffic_ip6ip6 "GRE hierarchical IPv6-in-IPv6 with key (new remote)"
+
+	hier_remote_restore
+
+	test_traffic_ip4ip6 "GRE hierarchical IPv4-in-IPv6 with key (old remote)"
+	test_traffic_ip6ip6 "GRE hierarchical IPv6-in-IPv6 with key (old remote)"
+}
+
 cleanup()
 {
 	pre_cleanup
tools/testing/selftests/net/forwarding/ip6gre_hier_keys.sh +14 -0
index ad1bcd6334a8..ad9eab4b1367 100755
@@ -8,6 +8,7 @@
 ALL_TESTS="
 	gre_hier
 	gre_mtu_change
+	gre_hier_remote_change
 "
 
 NUM_NETIFS=6

@@ -44,6 +45,19 @@ gre_mtu_change()
 	test_mtu_change gre
 }
 
+gre_hier_remote_change()
+{
+	hier_remote_change
+
+	test_traffic_ip4ip6 "GRE hierarchical IPv4-in-IPv6 with ikey/okey (new remote)"
+	test_traffic_ip6ip6 "GRE hierarchical IPv6-in-IPv6 with ikey/okey (new remote)"
+
+	hier_remote_restore
+
+	test_traffic_ip4ip6 "GRE hierarchical IPv4-in-IPv6 with ikey/okey (old remote)"
+	test_traffic_ip6ip6 "GRE hierarchical IPv6-in-IPv6 with ikey/okey (old remote)"
+}
+
 cleanup()
 {
 	pre_cleanup
tools/testing/selftests/net/forwarding/ip6gre_lib.sh +80 -0

83 lines not on this page · raw message

Signature · 1 line
2.45.0
CI Applied
#11 lore raw
In reply to Petr Machata
This series was applied to netdev/net.git (main) by Jakub Kicinski <[email protected]>:

Hello:

This series was applied to netdev/net.git (main)
by Jakub Kicinski <[email protected]>:

Quoted 8 lines · On Fri, 25 Oct 2024 16:26:24 +0200 you wrote:

In this patchset:

- Tx header should be pushed for each packet which is transmitted via Spectrum ASICs. Patch #1 adds a missing call to skb_cow_head() to make sure that there is both enough room to push the Tx header and that the SKB header is not cloned and can be modified.

[...]

Here is the summary with links:
- [net,1/5] mlxsw: spectrum_ptp: Add missing verification before pushing Tx header

    https://git.kernel.org/netdev/net/c/0a66e5582b51
  - [net,2/5] mlxsw: pci: Sync Rx buffers for CPU
    https://git.kernel.org/netdev/net/c/15f73e601a9c
  - [net,3/5] mlxsw: pci: Sync Rx buffers for device
    https://git.kernel.org/netdev/net/c/d0fbdc3ae9ec
  - [net,4/5] mlxsw: spectrum_ipip: Fix memory leak when changing remote IPv6 address
    https://git.kernel.org/netdev/net/c/12ae97c531fc
  - [net,5/5] selftests: forwarding: Add IPv6 GRE remote change tests
    https://git.kernel.org/netdev/net/c/d7bd61fa0222

You are awesome, thank you!

Signature · 2 lines
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html