[PATCH net-next 0/3] eth: fbnic: support ring size configuration

[PATCH net-next 0/3] eth: fbnic: support ring size configuration

[Jakub Kicinski]

Support ethtool -g / -G and a couple other small tweaks.

Jakub Kicinski (3):
  eth: fbnic: link NAPIs to page pools
  eth: fbnic: fix typo in compile assert
  eth: fbnic: support ring size configuration

 drivers/net/ethernet/meta/fbnic/fbnic_txrx.h  |  13 +++
 .../net/ethernet/meta/fbnic/fbnic_ethtool.c   | 109 ++++++++++++++++++
 drivers/net/ethernet/meta/fbnic/fbnic_txrx.c  |   6 +-
 3 files changed, 126 insertions(+), 2 deletions(-)

-- 
2.48.1

[PATCH net-next 1/3] eth: fbnic: link NAPIs to page pools

[Jakub Kicinski]

The lifetime of page pools is tied to NAPI instances,
and they are destroyed before NAPI is deleted.
It's safe to link them up.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 drivers/net/ethernet/meta/fbnic/fbnic_txrx.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
index aba4c65974ee..2d2d41c6891b 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
@@ -1316,7 +1316,9 @@ static int fbnic_alloc_nv_page_pool(struct fbnic_net *fbn,
 		.dev = nv->dev,
 		.dma_dir = DMA_BIDIRECTIONAL,
 		.offset = 0,
-		.max_len = PAGE_SIZE
+		.max_len = PAGE_SIZE,
+		.napi	= &nv->napi,
+		.netdev	= fbn->netdev,
 	};
 	struct page_pool *pp;
 
-- 
2.48.1

[PATCH net-next 2/3] eth: fbnic: fix typo in compile assert

[Jakub Kicinski]

We should be validating the Rx count on the Rx struct,
not the Tx struct. There is no real change here, rx_stats
and tx_stats are instances of the same struct.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 drivers/net/ethernet/meta/fbnic/fbnic_txrx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
index 2d2d41c6891b..ac11389a764c 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
@@ -1221,7 +1221,7 @@ void fbnic_aggregate_ring_rx_counters(struct fbnic_net *fbn,
 	fbn->rx_stats.rx.csum_complete += stats->rx.csum_complete;
 	fbn->rx_stats.rx.csum_none += stats->rx.csum_none;
 	/* Remember to add new stats here */
-	BUILD_BUG_ON(sizeof(fbn->tx_stats.rx) / 8 != 3);
+	BUILD_BUG_ON(sizeof(fbn->rx_stats.rx) / 8 != 3);
 }
 
 void fbnic_aggregate_ring_tx_counters(struct fbnic_net *fbn,
-- 
2.48.1

[PATCH net-next 3/3] eth: fbnic: support ring size configuration

[Jakub Kicinski]

Support ethtool -g / -G. Leverage the code added for -l / -L
to alloc / stop / start / free.

Check parameters against HW min/max but also our own min/max.
Min HW queue is 16 entries, we can't deal with TWQs that small
because of the queue waking logic. Add similar contraint on RCQ
for symmetry.

We need 3 sizes on Rx, as the NIC does header-data split two separate
buffer pools:
  (1) head page ring    - how many empty pages we post for headers
  (2) payload page ring - how many empty pages we post for payloads
  (3) completion ring   - where NIC produces the Rx descriptors

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
v0.2:
 - trim unused defines
 - add comment
 - add info to commit msg
 - add extack
---
 drivers/net/ethernet/meta/fbnic/fbnic_txrx.h  |  13 +++
 .../net/ethernet/meta/fbnic/fbnic_ethtool.c   | 109 ++++++++++++++++++
 2 files changed, 122 insertions(+)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.h b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.h
index 54368dc22328..f46616af41ea 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.h
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.h
@@ -24,9 +24,22 @@ struct fbnic_net;
 #define FBNIC_TX_DESC_WAKEUP	(FBNIC_MAX_SKB_DESC * 2)
 #define FBNIC_TX_DESC_MIN	roundup_pow_of_two(FBNIC_TX_DESC_WAKEUP)
 
+/* To receive the worst case packet we need:
+ *	1 descriptor for primary metadata
+ *	+ 1 descriptor for optional metadata
+ *	+ 1 descriptor for headers
+ *	+ 4 descriptors for payload
+ */
+#define FBNIC_MAX_RX_PKT_DESC	7
+#define FBNIC_RX_DESC_MIN	roundup_pow_of_two(FBNIC_MAX_RX_PKT_DESC * 2)
+
 #define FBNIC_MAX_TXQS			128u
 #define FBNIC_MAX_RXQS			128u
 
+/* These apply to TWQs, TCQ, RCQ */
+#define FBNIC_QUEUE_SIZE_MIN		16u
+#define FBNIC_QUEUE_SIZE_MAX		SZ_64K
+
 #define FBNIC_TXQ_SIZE_DEFAULT		1024
 #define FBNIC_HPQ_SIZE_DEFAULT		256
 #define FBNIC_PPQ_SIZE_DEFAULT		256
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c b/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
index c1477aad98a0..0a751a2aaf73 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
@@ -191,6 +191,113 @@ static int fbnic_set_coalesce(struct net_device *netdev,
 	return 0;
 }
 
+static void
+fbnic_get_ringparam(struct net_device *netdev, struct ethtool_ringparam *ring,
+		    struct kernel_ethtool_ringparam *kernel_ring,
+		    struct netlink_ext_ack *extack)
+{
+	struct fbnic_net *fbn = netdev_priv(netdev);
+
+	ring->rx_max_pending = FBNIC_QUEUE_SIZE_MAX;
+	ring->rx_mini_max_pending = FBNIC_QUEUE_SIZE_MAX;
+	ring->rx_jumbo_max_pending = FBNIC_QUEUE_SIZE_MAX;
+	ring->tx_max_pending = FBNIC_QUEUE_SIZE_MAX;
+
+	ring->rx_pending = fbn->rcq_size;
+	ring->rx_mini_pending = fbn->hpq_size;
+	ring->rx_jumbo_pending = fbn->ppq_size;
+	ring->tx_pending = fbn->txq_size;
+}
+
+static void fbnic_set_rings(struct fbnic_net *fbn,
+			    struct ethtool_ringparam *ring)
+{
+	fbn->rcq_size = ring->rx_pending;
+	fbn->hpq_size = ring->rx_mini_pending;
+	fbn->ppq_size = ring->rx_jumbo_pending;
+	fbn->txq_size = ring->tx_pending;
+}
+
+static int
+fbnic_set_ringparam(struct net_device *netdev, struct ethtool_ringparam *ring,
+		    struct kernel_ethtool_ringparam *kernel_ring,
+		    struct netlink_ext_ack *extack)
+
+{
+	struct fbnic_net *fbn = netdev_priv(netdev);
+	struct fbnic_net *clone;
+	int err;
+
+	ring->rx_pending	= roundup_pow_of_two(ring->rx_pending);
+	ring->rx_mini_pending	= roundup_pow_of_two(ring->rx_mini_pending);
+	ring->rx_jumbo_pending	= roundup_pow_of_two(ring->rx_jumbo_pending);
+	ring->tx_pending	= roundup_pow_of_two(ring->tx_pending);
+
+	/* These are absolute minimums allowing the device and driver to operate
+	 * but not necessarily guarantee reasonable performance. Settings below
+	 * Rx queue size of 128 and BDQs smaller than 64 are likely suboptimal
+	 * at best.
+	 */
+	if (ring->rx_pending < max(FBNIC_QUEUE_SIZE_MIN, FBNIC_RX_DESC_MIN) ||
+	    ring->rx_mini_pending < FBNIC_QUEUE_SIZE_MIN ||
+	    ring->rx_jumbo_pending < FBNIC_QUEUE_SIZE_MIN ||
+	    ring->tx_pending < max(FBNIC_QUEUE_SIZE_MIN, FBNIC_TX_DESC_MIN)) {
+		NL_SET_ERR_MSG_MOD(extack, "requested ring size too small");
+		return -EINVAL;
+	}
+
+	if (!netif_running(netdev)) {
+		fbnic_set_rings(fbn, ring);
+		return 0;
+	}
+
+	clone = fbnic_clone_create(fbn);
+	if (!clone)
+		return -ENOMEM;
+
+	fbnic_set_rings(clone, ring);
+
+	err = fbnic_alloc_napi_vectors(clone);
+	if (err)
+		goto err_free_clone;
+
+	err = fbnic_alloc_resources(clone);
+	if (err)
+		goto err_free_napis;
+
+	fbnic_down_noidle(fbn);
+	err = fbnic_wait_all_queues_idle(fbn->fbd, true);
+	if (err)
+		goto err_start_stack;
+
+	err = fbnic_set_netif_queues(clone);
+	if (err)
+		goto err_start_stack;
+
+	/* Nothing can fail past this point */
+	fbnic_flush(fbn);
+
+	fbnic_clone_swap(fbn, clone);
+
+	fbnic_up(fbn);
+
+	fbnic_free_resources(clone);
+	fbnic_free_napi_vectors(clone);
+	fbnic_clone_free(clone);
+
+	return 0;
+
+err_start_stack:
+	fbnic_flush(fbn);
+	fbnic_up(fbn);
+	fbnic_free_resources(clone);
+err_free_napis:
+	fbnic_free_napi_vectors(clone);
+err_free_clone:
+	fbnic_clone_free(clone);
+	return err;
+}
+
 static void fbnic_get_strings(struct net_device *dev, u32 sset, u8 *data)
 {
 	int i;
@@ -1351,6 +1458,8 @@ static const struct ethtool_ops fbnic_ethtool_ops = {
 	.get_regs		= fbnic_get_regs,
 	.get_coalesce		= fbnic_get_coalesce,
 	.set_coalesce		= fbnic_set_coalesce,
+	.get_ringparam		= fbnic_get_ringparam,
+	.set_ringparam		= fbnic_set_ringparam,
 	.get_strings		= fbnic_get_strings,
 	.get_ethtool_stats	= fbnic_get_ethtool_stats,
 	.get_sset_count		= fbnic_get_sset_count,
-- 
2.48.1

Re: [PATCH net-next 1/3] eth: fbnic: link NAPIs to page pools

[Joe Damato]

On Thu, Mar 06, 2025 at 06:51:48AM -0800, Jakub Kicinski wrote:
> The lifetime of page pools is tied to NAPI instances,
> and they are destroyed before NAPI is deleted.
> It's safe to link them up.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  drivers/net/ethernet/meta/fbnic/fbnic_txrx.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>

Acked-by: Joe Damato <jdamato@fastly.com>

Re: [PATCH net-next 2/3] eth: fbnic: fix typo in compile assert

[Joe Damato]

On Thu, Mar 06, 2025 at 06:51:49AM -0800, Jakub Kicinski wrote:
> We should be validating the Rx count on the Rx struct,
> not the Tx struct. There is no real change here, rx_stats
> and tx_stats are instances of the same struct.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  drivers/net/ethernet/meta/fbnic/fbnic_txrx.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>

Good catch.

Acked-by: Joe Damato <jdamato@fastly.com>

Re: [PATCH net-next 3/3] eth: fbnic: support ring size configuration

[Joe Damato]

On Thu, Mar 06, 2025 at 06:51:50AM -0800, Jakub Kicinski wrote:
> Support ethtool -g / -G. Leverage the code added for -l / -L
> to alloc / stop / start / free.
> 
> Check parameters against HW min/max but also our own min/max.
> Min HW queue is 16 entries, we can't deal with TWQs that small
> because of the queue waking logic. Add similar contraint on RCQ
> for symmetry.
> 
> We need 3 sizes on Rx, as the NIC does header-data split two separate
> buffer pools:
>   (1) head page ring    - how many empty pages we post for headers
>   (2) payload page ring - how many empty pages we post for payloads
>   (3) completion ring   - where NIC produces the Rx descriptors
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> v0.2:
>  - trim unused defines
>  - add comment
>  - add info to commit msg
>  - add extack
> ---
>  drivers/net/ethernet/meta/fbnic/fbnic_txrx.h  |  13 +++
>  .../net/ethernet/meta/fbnic/fbnic_ethtool.c   | 109 ++++++++++++++++++
>  2 files changed, 122 insertions(+)

Looked at this for a bit and read a bit more of fbnic. I obviously
know nothing about this device but nothing jumped out to me while
reading the patch:

Acked-by: Joe Damato <jdamato@fastly.com>

Re: [PATCH net-next 0/3] eth: fbnic: support ring size configuration

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu,  6 Mar 2025 06:51:47 -0800 you wrote:
> Support ethtool -g / -G and a couple other small tweaks.
> 
> Jakub Kicinski (3):
>   eth: fbnic: link NAPIs to page pools
>   eth: fbnic: fix typo in compile assert
>   eth: fbnic: support ring size configuration
> 
> [...]

Here is the summary with links:
  - [net-next,1/3] eth: fbnic: link NAPIs to page pools
    https://git.kernel.org/netdev/net-next/c/c1aacad30614
  - [net-next,2/3] eth: fbnic: fix typo in compile assert
    https://git.kernel.org/netdev/net-next/c/bfb522f347df
  - [net-next,3/3] eth: fbnic: support ring size configuration
    (no matching commit)

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html