[RFC PATCH net-next v2 0/9] bnxt: implement queue api

[RFC PATCH net-next v2 0/9] bnxt: implement queue api

[David Wei]

Both TCP devmem and io_uring ZC Rx need a way to change the page pool
for an Rx queue and reset it. As discussed in [1], until netdev core
takes a greater role in owning queue mem and ndo_open()/stop() calls
the queue API to alloc/free queue mem, we will have the driver
allocate queue mem in netdev_queue_mgmt_ops.

Rather than keeping the queue restart function in netmem, move it to
netdev core in netdev_rx_queue.h, since io_uring will also need to call
this as well. In the future, we'll have another API function that
changes the page pool memory provider for a given queue, then restarts
it.

The bnxt implementation allocates new queue mem and descriptors for an
rx queue before stopping the queue. Then, before starting the queue, the
queue mem and descriptors are swapped. Finally, the old queue mem and
descriptors are freed.

Individual patches go into more detail about how this is done for bnxt.
But from a high level:

1. Clone the bnxt_rx_ring_info for the specified rx queue
2. Allocate queue mem and descriptors into this clone
3. Stop the queue
4. Swap the queue mem and descriptors around, such that the clone how
   has the old queue mem and descriptors
5. Start the queue
6. Free the clone, which frees the old queue mem and descriptors

This patches compiled but is so far untested as I don't have access to
FW that supports individual rx queue reset. I'm sending it as an RFC to
hopefully get some early feedback while I get a test environment set up.

David Wei (8):
  bnxt: implement queue api
  netdev: add netdev_rx_queue_restart()
  bnxt: refactor bnxt_{alloc,free}_one_rx_ring()
  bnxt: refactor bnxt_{alloc,free}_one_tpa_info()
  bnxt: add __bnxt_init_rx_ring_struct() helper
  bnxt: add helpers for allocating rx ring mem
  bnxt: alloc rx ring mem first before reset
  bnxt: swap rx ring mem during queue_stop()

Mina Almasry (1):
  queue_api: define queue api

 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 410 ++++++++++++++++++----
 drivers/net/ethernet/broadcom/bnxt/bnxt.h |   2 +
 include/linux/netdevice.h                 |   3 +
 include/net/netdev_queues.h               |  27 ++
 include/net/netdev_rx_queue.h             |   3 +
 net/core/Makefile                         |   1 +
 net/core/netdev_rx_queue.c                |  58 +++
 7 files changed, 443 insertions(+), 61 deletions(-)
 create mode 100644 net/core/netdev_rx_queue.c

-- 
2.43.0

[RFC PATCH net-next v2 1/9] queue_api: define queue api

[David Wei]

From: Mina Almasry <almasrymina@google.com>

This API enables the net stack to reset the queues used for devmem TCP.

Signed-off-by: Mina Almasry <almasrymina@google.com>
Signed-off-by: David Wei <dw@davidwei.uk>
---
 include/linux/netdevice.h   |  3 +++
 include/net/netdev_queues.h | 27 +++++++++++++++++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index f849e7d110ed..6a58ec73c5e8 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1957,6 +1957,7 @@ enum netdev_reg_state {
  *	@sysfs_rx_queue_group:	Space for optional per-rx queue attributes
  *	@rtnl_link_ops:	Rtnl_link_ops
  *	@stat_ops:	Optional ops for queue-aware statistics
+ *	@queue_mgmt_ops:	Optional ops for queue management
  *
  *	@gso_max_size:	Maximum size of generic segmentation offload
  *	@tso_max_size:	Device (as in HW) limit on the max TSO request size
@@ -2340,6 +2341,8 @@ struct net_device {
 
 	const struct netdev_stat_ops *stat_ops;
 
+	const struct netdev_queue_mgmt_ops *queue_mgmt_ops;
+
 	/* for setting kernel sock attribute on TCP connection setup */
 #define GSO_MAX_SEGS		65535u
 #define GSO_LEGACY_MAX_SIZE	65536u
diff --git a/include/net/netdev_queues.h b/include/net/netdev_queues.h
index 1ec408585373..58042957c39f 100644
--- a/include/net/netdev_queues.h
+++ b/include/net/netdev_queues.h
@@ -60,6 +60,33 @@ struct netdev_stat_ops {
 			       struct netdev_queue_stats_tx *tx);
 };
 
+/**
+ * struct netdev_queue_mgmt_ops - netdev ops for queue management
+ *
+ * @ndo_queue_mem_alloc: Allocate memory for an RX queue. The memory returned
+ *			 in the form of a void* can be passed to
+ *			 ndo_queue_mem_free() for freeing or to ndo_queue_start
+ *			 to create an RX queue with this memory.
+ *
+ * @ndo_queue_mem_free:	Free memory from an RX queue.
+ *
+ * @ndo_queue_start:	Start an RX queue at the specified index.
+ *
+ * @ndo_queue_stop:	Stop the RX queue at the specified index.
+ */
+struct netdev_queue_mgmt_ops {
+       void *                  (*ndo_queue_mem_alloc)(struct net_device *dev,
+                                                      int idx);
+       void                    (*ndo_queue_mem_free)(struct net_device *dev,
+                                                     void *queue_mem);
+       int                     (*ndo_queue_start)(struct net_device *dev,
+                                                  int idx,
+                                                  void *queue_mem);
+       int                     (*ndo_queue_stop)(struct net_device *dev,
+                                                 int idx,
+                                                 void **out_queue_mem);
+};
+
 /**
  * DOC: Lockless queue stopping / waking helpers.
  *
-- 
2.43.0

Re: [RFC PATCH net-next v2 1/9] queue_api: define queue api

[Simon Horman]

On Wed, May 01, 2024 at 09:54:02PM -0700, David Wei wrote:
> From: Mina Almasry <almasrymina@google.com>
> 
> This API enables the net stack to reset the queues used for devmem TCP.
> 
> Signed-off-by: Mina Almasry <almasrymina@google.com>
> Signed-off-by: David Wei <dw@davidwei.uk>

...

> diff --git a/include/net/netdev_queues.h b/include/net/netdev_queues.h
> index 1ec408585373..58042957c39f 100644
> --- a/include/net/netdev_queues.h
> +++ b/include/net/netdev_queues.h
> @@ -60,6 +60,33 @@ struct netdev_stat_ops {
>  			       struct netdev_queue_stats_tx *tx);
>  };
>  
> +/**
> + * struct netdev_queue_mgmt_ops - netdev ops for queue management
> + *
> + * @ndo_queue_mem_alloc: Allocate memory for an RX queue. The memory returned
> + *			 in the form of a void* can be passed to
> + *			 ndo_queue_mem_free() for freeing or to ndo_queue_start
> + *			 to create an RX queue with this memory.
> + *
> + * @ndo_queue_mem_free:	Free memory from an RX queue.
> + *
> + * @ndo_queue_start:	Start an RX queue at the specified index.
> + *
> + * @ndo_queue_stop:	Stop the RX queue at the specified index.
> + */
> +struct netdev_queue_mgmt_ops {
> +       void *                  (*ndo_queue_mem_alloc)(struct net_device *dev,
> +                                                      int idx);
> +       void                    (*ndo_queue_mem_free)(struct net_device *dev,
> +                                                     void *queue_mem);
> +       int                     (*ndo_queue_start)(struct net_device *dev,
> +                                                  int idx,
> +                                                  void *queue_mem);
> +       int                     (*ndo_queue_stop)(struct net_device *dev,
> +                                                 int idx,
> +                                                 void **out_queue_mem);

Nit: The indentation (before the return types) should use tabs rather than
     spaces. And I'm not sure I see the value of the large indentation after
     the return types. Basically, I suggest this:

	void * (*ndo_queue_mem_alloc)(struct net_device *dev, int idx);
	void   (*ndo_queue_mem_free)(struct net_device *dev, void *queue_mem);
	int    (*ndo_queue_start)(struct net_device *dev, int idx,
				  void *queue_mem);
	int    (*ndo_queue_stop)(struct net_device *dev, int idx,
				 void **out_queue_mem);

> +};
> +
>  /**
>   * DOC: Lockless queue stopping / waking helpers.
>   *
> -- 
> 2.43.0
> 
> 

Re: [RFC PATCH net-next v2 1/9] queue_api: define queue api

[David Wei]

On 2024-05-04 05:11, Simon Horman wrote:
> On Wed, May 01, 2024 at 09:54:02PM -0700, David Wei wrote:
>> From: Mina Almasry <almasrymina@google.com>

...

>> +struct netdev_queue_mgmt_ops {
>> +       void *                  (*ndo_queue_mem_alloc)(struct net_device *dev,
>> +                                                      int idx);
>> +       void                    (*ndo_queue_mem_free)(struct net_device *dev,
>> +                                                     void *queue_mem);
>> +       int                     (*ndo_queue_start)(struct net_device *dev,
>> +                                                  int idx,
>> +                                                  void *queue_mem);
>> +       int                     (*ndo_queue_stop)(struct net_device *dev,
>> +                                                 int idx,
>> +                                                 void **out_queue_mem);
> 
> Nit: The indentation (before the return types) should use tabs rather than
>      spaces. And I'm not sure I see the value of the large indentation after
>      the return types. Basically, I suggest this:
> 
> 	void * (*ndo_queue_mem_alloc)(struct net_device *dev, int idx);
> 	void   (*ndo_queue_mem_free)(struct net_device *dev, void *queue_mem);
> 	int    (*ndo_queue_start)(struct net_device *dev, int idx,
> 				  void *queue_mem);
> 	int    (*ndo_queue_stop)(struct net_device *dev, int idx,
> 				 void **out_queue_mem);
> 

Hi Simon, this patch came from Shailend and Mina which I applied to this
patchset. We'll make sure it's formatted properly once we send a
non-RFC. Thanks.

>> +};
>> +
>>  /**
>>   * DOC: Lockless queue stopping / waking helpers.
>>   *
>> -- 
>> 2.43.0
>>
>>

[RFC PATCH net-next v2 2/9] bnxt: implement queue api

[David Wei]

Implement the bare minimum queue API for bnxt. I'm essentially breaking
up the existing bnxt_rx_ring_reset() into two steps:

1. bnxt_queue_stop()
2. bnxt_queue_start()

The other two ndos are left as no-ops for now, so the queue mem is
allocated after the queue has been stopped. Doing this before stopping
the queue is a lot more work, so I'm looking for some feedback first.

Signed-off-by: David Wei <dw@davidwei.uk>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 62 +++++++++++++++++++++++
 1 file changed, 62 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 06b7a963bbbd..788c87271eb1 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -14810,6 +14810,67 @@ static const struct netdev_stat_ops bnxt_stat_ops = {
 	.get_base_stats		= bnxt_get_base_stats,
 };
 
+static void *bnxt_queue_mem_alloc(struct net_device *dev, int idx)
+{
+	struct bnxt *bp = netdev_priv(dev);
+
+	return &bp->rx_ring[idx];
+}
+
+static void bnxt_queue_mem_free(struct net_device *dev, void *qmem)
+{
+}
+
+static int bnxt_queue_start(struct net_device *dev, int idx, void *qmem)
+{
+	struct bnxt_rx_ring_info *rxr = qmem;
+	struct bnxt *bp = netdev_priv(dev);
+
+	bnxt_alloc_one_rx_ring(bp, idx);
+
+	if (bp->flags & BNXT_FLAG_AGG_RINGS)
+		bnxt_db_write(bp, &rxr->rx_agg_db, rxr->rx_agg_prod);
+	bnxt_db_write(bp, &rxr->rx_db, rxr->rx_prod);
+
+	if (bp->flags & BNXT_FLAG_TPA)
+		bnxt_set_tpa(bp, true);
+
+	return 0;
+}
+
+static int bnxt_queue_stop(struct net_device *dev, int idx, void **out_qmem)
+{
+	struct bnxt *bp = netdev_priv(dev);
+	struct bnxt_rx_ring_info *rxr;
+	struct bnxt_cp_ring_info *cpr;
+	int rc;
+
+	rc = bnxt_hwrm_rx_ring_reset(bp, idx);
+	if (rc)
+		return rc;
+
+	bnxt_free_one_rx_ring_skbs(bp, idx);
+	rxr = &bp->rx_ring[idx];
+	rxr->rx_prod = 0;
+	rxr->rx_agg_prod = 0;
+	rxr->rx_sw_agg_prod = 0;
+	rxr->rx_next_cons = 0;
+
+	cpr = &rxr->bnapi->cp_ring;
+	cpr->sw_stats.rx.rx_resets++;
+
+	*out_qmem = rxr;
+
+	return 0;
+}
+
+static const struct netdev_queue_mgmt_ops bnxt_queue_mgmt_ops = {
+	.ndo_queue_mem_alloc	= bnxt_queue_mem_alloc,
+	.ndo_queue_mem_free	= bnxt_queue_mem_free,
+	.ndo_queue_start	= bnxt_queue_start,
+	.ndo_queue_stop		= bnxt_queue_stop,
+};
+
 static void bnxt_remove_one(struct pci_dev *pdev)
 {
 	struct net_device *dev = pci_get_drvdata(pdev);
@@ -15275,6 +15336,7 @@ static int bnxt_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	dev->stat_ops = &bnxt_stat_ops;
 	dev->watchdog_timeo = BNXT_TX_TIMEOUT;
 	dev->ethtool_ops = &bnxt_ethtool_ops;
+	dev->queue_mgmt_ops = &bnxt_queue_mgmt_ops;
 	pci_set_drvdata(pdev, dev);
 
 	rc = bnxt_alloc_hwrm_resources(bp);
-- 
2.43.0

Re: [RFC PATCH net-next v2 2/9] bnxt: implement queue api

[Mina Almasry]

On Wed, May 1, 2024 at 9:54 PM David Wei <dw@davidwei.uk> wrote:
>
> Implement the bare minimum queue API for bnxt. I'm essentially breaking
> up the existing bnxt_rx_ring_reset() into two steps:
>
> 1. bnxt_queue_stop()
> 2. bnxt_queue_start()
>
> The other two ndos are left as no-ops for now, so the queue mem is
> allocated after the queue has been stopped. Doing this before stopping
> the queue is a lot more work, so I'm looking for some feedback first.
>
> Signed-off-by: David Wei <dw@davidwei.uk>
> ---
>  drivers/net/ethernet/broadcom/bnxt/bnxt.c | 62 +++++++++++++++++++++++
>  1 file changed, 62 insertions(+)
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index 06b7a963bbbd..788c87271eb1 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -14810,6 +14810,67 @@ static const struct netdev_stat_ops bnxt_stat_ops = {
>         .get_base_stats         = bnxt_get_base_stats,
>  };
>
> +static void *bnxt_queue_mem_alloc(struct net_device *dev, int idx)
> +{
> +       struct bnxt *bp = netdev_priv(dev);
> +
> +       return &bp->rx_ring[idx];
> +}
> +
> +static void bnxt_queue_mem_free(struct net_device *dev, void *qmem)
> +{
> +}
> +
> +static int bnxt_queue_start(struct net_device *dev, int idx, void *qmem)
> +{
> +       struct bnxt_rx_ring_info *rxr = qmem;
> +       struct bnxt *bp = netdev_priv(dev);
> +
> +       bnxt_alloc_one_rx_ring(bp, idx);
> +
> +       if (bp->flags & BNXT_FLAG_AGG_RINGS)
> +               bnxt_db_write(bp, &rxr->rx_agg_db, rxr->rx_agg_prod);
> +       bnxt_db_write(bp, &rxr->rx_db, rxr->rx_prod);
> +
> +       if (bp->flags & BNXT_FLAG_TPA)
> +               bnxt_set_tpa(bp, true);
> +
> +       return 0;
> +}
> +
> +static int bnxt_queue_stop(struct net_device *dev, int idx, void **out_qmem)
> +{
> +       struct bnxt *bp = netdev_priv(dev);
> +       struct bnxt_rx_ring_info *rxr;
> +       struct bnxt_cp_ring_info *cpr;
> +       int rc;
> +
> +       rc = bnxt_hwrm_rx_ring_reset(bp, idx);
> +       if (rc)
> +               return rc;
> +
> +       bnxt_free_one_rx_ring_skbs(bp, idx);
> +       rxr = &bp->rx_ring[idx];
> +       rxr->rx_prod = 0;
> +       rxr->rx_agg_prod = 0;
> +       rxr->rx_sw_agg_prod = 0;
> +       rxr->rx_next_cons = 0;
> +
> +       cpr = &rxr->bnapi->cp_ring;
> +       cpr->sw_stats.rx.rx_resets++;
> +
> +       *out_qmem = rxr;

Oh, I'm not sure you can do this, no?

rxr is a stack variable, it goes away after the function returns, no?

You have to kzalloc sizeof(struct bnext_rx_ring_info), no?

Other than that, the code looks very similar to what we do for GVE,
and good to me.

> +
> +       return 0;
> +}
> +
> +static const struct netdev_queue_mgmt_ops bnxt_queue_mgmt_ops = {
> +       .ndo_queue_mem_alloc    = bnxt_queue_mem_alloc,
> +       .ndo_queue_mem_free     = bnxt_queue_mem_free,
> +       .ndo_queue_start        = bnxt_queue_start,
> +       .ndo_queue_stop         = bnxt_queue_stop,
> +};
> +
>  static void bnxt_remove_one(struct pci_dev *pdev)
>  {
>         struct net_device *dev = pci_get_drvdata(pdev);
> @@ -15275,6 +15336,7 @@ static int bnxt_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>         dev->stat_ops = &bnxt_stat_ops;
>         dev->watchdog_timeo = BNXT_TX_TIMEOUT;
>         dev->ethtool_ops = &bnxt_ethtool_ops;
> +       dev->queue_mgmt_ops = &bnxt_queue_mgmt_ops;
>         pci_set_drvdata(pdev, dev);
>
>         rc = bnxt_alloc_hwrm_resources(bp);
> --
> 2.43.0
>


-- 
Thanks,
Mina

Re: [RFC PATCH net-next v2 2/9] bnxt: implement queue api

[David Wei]

On 2024-05-02 10:23, Mina Almasry wrote:
> On Wed, May 1, 2024 at 9:54 PM David Wei <dw@davidwei.uk> wrote:

...

>> +
>> +static int bnxt_queue_stop(struct net_device *dev, int idx, void **out_qmem)
>> +{
>> +       struct bnxt *bp = netdev_priv(dev);
>> +       struct bnxt_rx_ring_info *rxr;
>> +       struct bnxt_cp_ring_info *cpr;
>> +       int rc;
>> +
>> +       rc = bnxt_hwrm_rx_ring_reset(bp, idx);
>> +       if (rc)
>> +               return rc;
>> +
>> +       bnxt_free_one_rx_ring_skbs(bp, idx);
>> +       rxr = &bp->rx_ring[idx];
>> +       rxr->rx_prod = 0;
>> +       rxr->rx_agg_prod = 0;
>> +       rxr->rx_sw_agg_prod = 0;
>> +       rxr->rx_next_cons = 0;
>> +
>> +       cpr = &rxr->bnapi->cp_ring;
>> +       cpr->sw_stats.rx.rx_resets++;
>> +
>> +       *out_qmem = rxr;
> 
> Oh, I'm not sure you can do this, no?
> 
> rxr is a stack variable, it goes away after the function returns, no?

Not for bnxt, where rx_ring is a dynamically allocated array.

In later patches I will change how the ndos are implemented. Next series
I'll squash these intermediate patches that are now useless.

> 
> You have to kzalloc sizeof(struct bnext_rx_ring_info), no?
> 
> Other than that, the code looks very similar to what we do for GVE,
> and good to me.

Thanks.

> 
>> +
>> +       return 0;
>> +}
>> +
>> +static const struct netdev_queue_mgmt_ops bnxt_queue_mgmt_ops = {
>> +       .ndo_queue_mem_alloc    = bnxt_queue_mem_alloc,
>> +       .ndo_queue_mem_free     = bnxt_queue_mem_free,
>> +       .ndo_queue_start        = bnxt_queue_start,
>> +       .ndo_queue_stop         = bnxt_queue_stop,
>> +};
>> +
>>  static void bnxt_remove_one(struct pci_dev *pdev)
>>  {
>>         struct net_device *dev = pci_get_drvdata(pdev);
>> @@ -15275,6 +15336,7 @@ static int bnxt_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>>         dev->stat_ops = &bnxt_stat_ops;
>>         dev->watchdog_timeo = BNXT_TX_TIMEOUT;
>>         dev->ethtool_ops = &bnxt_ethtool_ops;
>> +       dev->queue_mgmt_ops = &bnxt_queue_mgmt_ops;
>>         pci_set_drvdata(pdev, dev);
>>
>>         rc = bnxt_alloc_hwrm_resources(bp);
>> --
>> 2.43.0
>>
> 
> 

[RFC PATCH net-next v2 3/9] netdev: add netdev_rx_queue_restart()

[David Wei]

From: Mina Almasry <almasrymina@google.com>

Add netdev_rx_queue_restart() function to netdev_rx_queue.h. This is
taken from Mina's work in [1] with a slight modification of taking
rtnl_lock() during the queue stop and start ops.

For bnxt specifically, if the firmware doesn't support
BNXT_RST_RING_SP_EVENT, then ndo_queue_stop() returns -EOPNOTSUPP and
the whole restart fails. Unlike bnxt_rx_ring_reset(), there is no
attempt to reset the whole device.

[1]: https://lore.kernel.org/linux-kernel/20240403002053.2376017-6-almasrymina@google.com/#t

Signed-off-by: David Wei <dw@davidwei.uk>
---
 include/net/netdev_rx_queue.h |  3 ++
 net/core/Makefile             |  1 +
 net/core/netdev_rx_queue.c    | 58 +++++++++++++++++++++++++++++++++++
 3 files changed, 62 insertions(+)
 create mode 100644 net/core/netdev_rx_queue.c

diff --git a/include/net/netdev_rx_queue.h b/include/net/netdev_rx_queue.h
index aa1716fb0e53..e78ca52d67fb 100644
--- a/include/net/netdev_rx_queue.h
+++ b/include/net/netdev_rx_queue.h
@@ -54,4 +54,7 @@ get_netdev_rx_queue_index(struct netdev_rx_queue *queue)
 	return index;
 }
 #endif
+
+int netdev_rx_queue_restart(struct net_device *dev, unsigned int rxq);
+
 #endif
diff --git a/net/core/Makefile b/net/core/Makefile
index 21d6fbc7e884..f2aa63c167a3 100644
--- a/net/core/Makefile
+++ b/net/core/Makefile
@@ -19,6 +19,7 @@ obj-$(CONFIG_NETDEV_ADDR_LIST_TEST) += dev_addr_lists_test.o
 
 obj-y += net-sysfs.o
 obj-y += hotdata.o
+obj-y += netdev_rx_queue.o
 obj-$(CONFIG_PAGE_POOL) += page_pool.o page_pool_user.o
 obj-$(CONFIG_PROC_FS) += net-procfs.o
 obj-$(CONFIG_NET_PKTGEN) += pktgen.o
diff --git a/net/core/netdev_rx_queue.c b/net/core/netdev_rx_queue.c
new file mode 100644
index 000000000000..9633fb36f6d1
--- /dev/null
+++ b/net/core/netdev_rx_queue.c
@@ -0,0 +1,58 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#include <linux/netdevice.h>
+#include <net/netdev_queues.h>
+#include <net/netdev_rx_queue.h>
+
+int netdev_rx_queue_restart(struct net_device *dev, unsigned int rxq)
+{
+	void *new_mem;
+	void *old_mem;
+	int err;
+
+	if (!dev->queue_mgmt_ops->ndo_queue_stop ||
+	    !dev->queue_mgmt_ops->ndo_queue_mem_free ||
+	    !dev->queue_mgmt_ops->ndo_queue_mem_alloc ||
+	    !dev->queue_mgmt_ops->ndo_queue_start)
+		return -EOPNOTSUPP;
+
+	new_mem = dev->queue_mgmt_ops->ndo_queue_mem_alloc(dev, rxq);
+	if (!new_mem)
+		return -ENOMEM;
+
+	rtnl_lock();
+	err = dev->queue_mgmt_ops->ndo_queue_stop(dev, rxq, &old_mem);
+	if (err)
+		goto err_free_new_mem;
+
+	err = dev->queue_mgmt_ops->ndo_queue_start(dev, rxq, new_mem);
+	if (err)
+		goto err_start_queue;
+	rtnl_unlock();
+
+	dev->queue_mgmt_ops->ndo_queue_mem_free(dev, old_mem);
+
+	return 0;
+
+err_start_queue:
+	/* Restarting the queue with old_mem should be successful as we haven't
+	 * changed any of the queue configuration, and there is not much we can
+	 * do to recover from a failure here.
+	 *
+	 * WARN if the we fail to recover the old rx queue, and at least free
+	 * old_mem so we don't also leak that.
+	 */
+	if (dev->queue_mgmt_ops->ndo_queue_start(dev, rxq, old_mem)) {
+		WARN(1,
+		     "Failed to restart old queue in error path. RX queue %d may be unhealthy.",
+		     rxq);
+		dev->queue_mgmt_ops->ndo_queue_mem_free(dev, &old_mem);
+	}
+
+err_free_new_mem:
+	dev->queue_mgmt_ops->ndo_queue_mem_free(dev, new_mem);
+	rtnl_unlock();
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(netdev_rx_queue_restart);
-- 
2.43.0

Re: [RFC PATCH net-next v2 3/9] netdev: add netdev_rx_queue_restart()

[Mina Almasry]

On Wed, May 1, 2024 at 9:54 PM David Wei <dw@davidwei.uk> wrote:
>
> From: Mina Almasry <almasrymina@google.com>
>
> Add netdev_rx_queue_restart() function to netdev_rx_queue.h. This is
> taken from Mina's work in [1] with a slight modification of taking
> rtnl_lock() during the queue stop and start ops.
>
> For bnxt specifically, if the firmware doesn't support
> BNXT_RST_RING_SP_EVENT, then ndo_queue_stop() returns -EOPNOTSUPP and
> the whole restart fails. Unlike bnxt_rx_ring_reset(), there is no
> attempt to reset the whole device.
>
> [1]: https://lore.kernel.org/linux-kernel/20240403002053.2376017-6-almasrymina@google.com/#t
>
> Signed-off-by: David Wei <dw@davidwei.uk>
> ---
>  include/net/netdev_rx_queue.h |  3 ++
>  net/core/Makefile             |  1 +
>  net/core/netdev_rx_queue.c    | 58 +++++++++++++++++++++++++++++++++++
>  3 files changed, 62 insertions(+)
>  create mode 100644 net/core/netdev_rx_queue.c
>
> diff --git a/include/net/netdev_rx_queue.h b/include/net/netdev_rx_queue.h
> index aa1716fb0e53..e78ca52d67fb 100644
> --- a/include/net/netdev_rx_queue.h
> +++ b/include/net/netdev_rx_queue.h
> @@ -54,4 +54,7 @@ get_netdev_rx_queue_index(struct netdev_rx_queue *queue)
>         return index;
>  }
>  #endif
> +
> +int netdev_rx_queue_restart(struct net_device *dev, unsigned int rxq);
> +
>  #endif
> diff --git a/net/core/Makefile b/net/core/Makefile
> index 21d6fbc7e884..f2aa63c167a3 100644
> --- a/net/core/Makefile
> +++ b/net/core/Makefile
> @@ -19,6 +19,7 @@ obj-$(CONFIG_NETDEV_ADDR_LIST_TEST) += dev_addr_lists_test.o
>
>  obj-y += net-sysfs.o
>  obj-y += hotdata.o
> +obj-y += netdev_rx_queue.o
>  obj-$(CONFIG_PAGE_POOL) += page_pool.o page_pool_user.o
>  obj-$(CONFIG_PROC_FS) += net-procfs.o
>  obj-$(CONFIG_NET_PKTGEN) += pktgen.o
> diff --git a/net/core/netdev_rx_queue.c b/net/core/netdev_rx_queue.c
> new file mode 100644
> index 000000000000..9633fb36f6d1
> --- /dev/null
> +++ b/net/core/netdev_rx_queue.c
> @@ -0,0 +1,58 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#include <linux/netdevice.h>
> +#include <net/netdev_queues.h>
> +#include <net/netdev_rx_queue.h>
> +
> +int netdev_rx_queue_restart(struct net_device *dev, unsigned int rxq)
> +{
> +       void *new_mem;
> +       void *old_mem;
> +       int err;
> +
> +       if (!dev->queue_mgmt_ops->ndo_queue_stop ||
> +           !dev->queue_mgmt_ops->ndo_queue_mem_free ||
> +           !dev->queue_mgmt_ops->ndo_queue_mem_alloc ||
> +           !dev->queue_mgmt_ops->ndo_queue_start)
> +               return -EOPNOTSUPP;
> +
> +       new_mem = dev->queue_mgmt_ops->ndo_queue_mem_alloc(dev, rxq);
> +       if (!new_mem)
> +               return -ENOMEM;
> +
> +       rtnl_lock();
> +       err = dev->queue_mgmt_ops->ndo_queue_stop(dev, rxq, &old_mem);
> +       if (err)
> +               goto err_free_new_mem;
> +
> +       err = dev->queue_mgmt_ops->ndo_queue_start(dev, rxq, new_mem);
> +       if (err)
> +               goto err_start_queue;
> +       rtnl_unlock();
> +
> +       dev->queue_mgmt_ops->ndo_queue_mem_free(dev, old_mem);
> +
> +       return 0;
> +
> +err_start_queue:
> +       /* Restarting the queue with old_mem should be successful as we haven't
> +        * changed any of the queue configuration, and there is not much we can
> +        * do to recover from a failure here.
> +        *
> +        * WARN if the we fail to recover the old rx queue, and at least free
> +        * old_mem so we don't also leak that.
> +        */
> +       if (dev->queue_mgmt_ops->ndo_queue_start(dev, rxq, old_mem)) {
> +               WARN(1,
> +                    "Failed to restart old queue in error path. RX queue %d may be unhealthy.",
> +                    rxq);
> +               dev->queue_mgmt_ops->ndo_queue_mem_free(dev, &old_mem);
> +       }
> +
> +err_free_new_mem:
> +       dev->queue_mgmt_ops->ndo_queue_mem_free(dev, new_mem);
> +       rtnl_unlock();
> +
> +       return err;
> +}

The function looks good to me. It's very similar to what we are doing with GVE.

> +EXPORT_SYMBOL_GPL(netdev_rx_queue_restart);

I would still prefer not to export this, unless necessary, and it
seems it's not at the moment (we only need to call it from core net
and core io uring which doesn't need an export).

Unexporting later, as far as my primitive understanding goes, is maybe
tricky because it may break out of tree drivers that decided to call
this. I don't feel strongly about unexporting, but someone else may.

-- 
Thanks,
Mina

Re: [RFC PATCH net-next v2 3/9] netdev: add netdev_rx_queue_restart()

[David Wei]

On 2024-05-02 10:27, Mina Almasry wrote:
> On Wed, May 1, 2024 at 9:54 PM David Wei <dw@davidwei.uk> wrote:
>>

...

>> +int netdev_rx_queue_restart(struct net_device *dev, unsigned int rxq)
>> +{
>> +       void *new_mem;
>> +       void *old_mem;
>> +       int err;
>> +
>> +       if (!dev->queue_mgmt_ops->ndo_queue_stop ||
>> +           !dev->queue_mgmt_ops->ndo_queue_mem_free ||
>> +           !dev->queue_mgmt_ops->ndo_queue_mem_alloc ||
>> +           !dev->queue_mgmt_ops->ndo_queue_start)
>> +               return -EOPNOTSUPP;
>> +
>> +       new_mem = dev->queue_mgmt_ops->ndo_queue_mem_alloc(dev, rxq);
>> +       if (!new_mem)
>> +               return -ENOMEM;
>> +
>> +       rtnl_lock();
>> +       err = dev->queue_mgmt_ops->ndo_queue_stop(dev, rxq, &old_mem);
>> +       if (err)
>> +               goto err_free_new_mem;
>> +
>> +       err = dev->queue_mgmt_ops->ndo_queue_start(dev, rxq, new_mem);
>> +       if (err)
>> +               goto err_start_queue;
>> +       rtnl_unlock();
>> +
>> +       dev->queue_mgmt_ops->ndo_queue_mem_free(dev, old_mem);
>> +
>> +       return 0;
>> +
>> +err_start_queue:
>> +       /* Restarting the queue with old_mem should be successful as we haven't
>> +        * changed any of the queue configuration, and there is not much we can
>> +        * do to recover from a failure here.
>> +        *
>> +        * WARN if the we fail to recover the old rx queue, and at least free
>> +        * old_mem so we don't also leak that.
>> +        */
>> +       if (dev->queue_mgmt_ops->ndo_queue_start(dev, rxq, old_mem)) {
>> +               WARN(1,
>> +                    "Failed to restart old queue in error path. RX queue %d may be unhealthy.",
>> +                    rxq);
>> +               dev->queue_mgmt_ops->ndo_queue_mem_free(dev, &old_mem);
>> +       }
>> +
>> +err_free_new_mem:
>> +       dev->queue_mgmt_ops->ndo_queue_mem_free(dev, new_mem);
>> +       rtnl_unlock();
>> +
>> +       return err;
>> +}
> 
> The function looks good to me. It's very similar to what we are doing with GVE.
> 
>> +EXPORT_SYMBOL_GPL(netdev_rx_queue_restart);
> 
> I would still prefer not to export this, unless necessary, and it
> seems it's not at the moment (we only need to call it from core net
> and core io uring which doesn't need an export).
> 
> Unexporting later, as far as my primitive understanding goes, is maybe
> tricky because it may break out of tree drivers that decided to call
> this. I don't feel strongly about unexporting, but someone else may.

Sorry, I didn't mean to ignore you, I forgot to do it. :(

I'll change it for the next series.

Re: [RFC PATCH net-next v2 3/9] netdev: add netdev_rx_queue_restart()

[Simon Horman]

On Wed, May 01, 2024 at 09:54:04PM -0700, David Wei wrote:
> From: Mina Almasry <almasrymina@google.com>
> 
> Add netdev_rx_queue_restart() function to netdev_rx_queue.h. This is
> taken from Mina's work in [1] with a slight modification of taking
> rtnl_lock() during the queue stop and start ops.
> 
> For bnxt specifically, if the firmware doesn't support
> BNXT_RST_RING_SP_EVENT, then ndo_queue_stop() returns -EOPNOTSUPP and
> the whole restart fails. Unlike bnxt_rx_ring_reset(), there is no
> attempt to reset the whole device.
> 
> [1]: https://lore.kernel.org/linux-kernel/20240403002053.2376017-6-almasrymina@google.com/#t
> 
> Signed-off-by: David Wei <dw@davidwei.uk>

nit: Mina's From line is above, but there is no corresponding Signed-off-by
     line here.

> ---
>  include/net/netdev_rx_queue.h |  3 ++
>  net/core/Makefile             |  1 +
>  net/core/netdev_rx_queue.c    | 58 +++++++++++++++++++++++++++++++++++
>  3 files changed, 62 insertions(+)
>  create mode 100644 net/core/netdev_rx_queue.c
> 
> diff --git a/include/net/netdev_rx_queue.h b/include/net/netdev_rx_queue.h
> index aa1716fb0e53..e78ca52d67fb 100644
> --- a/include/net/netdev_rx_queue.h
> +++ b/include/net/netdev_rx_queue.h
> @@ -54,4 +54,7 @@ get_netdev_rx_queue_index(struct netdev_rx_queue *queue)
>  	return index;
>  }
>  #endif
> +
> +int netdev_rx_queue_restart(struct net_device *dev, unsigned int rxq);
> +
>  #endif
> diff --git a/net/core/Makefile b/net/core/Makefile
> index 21d6fbc7e884..f2aa63c167a3 100644
> --- a/net/core/Makefile
> +++ b/net/core/Makefile
> @@ -19,6 +19,7 @@ obj-$(CONFIG_NETDEV_ADDR_LIST_TEST) += dev_addr_lists_test.o
>  
>  obj-y += net-sysfs.o
>  obj-y += hotdata.o
> +obj-y += netdev_rx_queue.o
>  obj-$(CONFIG_PAGE_POOL) += page_pool.o page_pool_user.o
>  obj-$(CONFIG_PROC_FS) += net-procfs.o
>  obj-$(CONFIG_NET_PKTGEN) += pktgen.o
> diff --git a/net/core/netdev_rx_queue.c b/net/core/netdev_rx_queue.c
> new file mode 100644
> index 000000000000..9633fb36f6d1
> --- /dev/null
> +++ b/net/core/netdev_rx_queue.c
> @@ -0,0 +1,58 @@
> +/* SPDX-License-Identifier: GPL-2.0 */

nit: my understanding is that, as a .c file, the correct SPDX format is:

// SPDX-License-Identifier: GPL-2.0

...

Re: [RFC PATCH net-next v2 3/9] netdev: add netdev_rx_queue_restart()

[David Wei]

On 2024-05-04 05:20, Simon Horman wrote:
> On Wed, May 01, 2024 at 09:54:04PM -0700, David Wei wrote:
>> From: Mina Almasry <almasrymina@google.com>
>>
>> Add netdev_rx_queue_restart() function to netdev_rx_queue.h. This is
>> taken from Mina's work in [1] with a slight modification of taking
>> rtnl_lock() during the queue stop and start ops.
>>
>> For bnxt specifically, if the firmware doesn't support
>> BNXT_RST_RING_SP_EVENT, then ndo_queue_stop() returns -EOPNOTSUPP and
>> the whole restart fails. Unlike bnxt_rx_ring_reset(), there is no
>> attempt to reset the whole device.
>>
>> [1]: https://lore.kernel.org/linux-kernel/20240403002053.2376017-6-almasrymina@google.com/#t
>>
>> Signed-off-by: David Wei <dw@davidwei.uk>
> 
> nit: Mina's From line is above, but there is no corresponding Signed-off-by
>      line here.

This patch isn't a clean cherry pick, I pulled the core logic of
netdev_rx_queue_restart() from the middle of another patch. In these
cases should I be manually adding Signed-off-by tag?

...

>> diff --git a/net/core/netdev_rx_queue.c b/net/core/netdev_rx_queue.c
>> new file mode 100644
>> index 000000000000..9633fb36f6d1
>> --- /dev/null
>> +++ b/net/core/netdev_rx_queue.c
>> @@ -0,0 +1,58 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
> 
> nit: my understanding is that, as a .c file, the correct SPDX format is:
> 
> // SPDX-License-Identifier: GPL-2.0

Thanks, I will fix this.

> 
> ...

Re: [RFC PATCH net-next v2 3/9] netdev: add netdev_rx_queue_restart()

[Simon Horman]

On Sun, May 05, 2024 at 05:41:14PM -0700, David Wei wrote:
> On 2024-05-04 05:20, Simon Horman wrote:
> > On Wed, May 01, 2024 at 09:54:04PM -0700, David Wei wrote:
> >> From: Mina Almasry <almasrymina@google.com>
> >>
> >> Add netdev_rx_queue_restart() function to netdev_rx_queue.h. This is
> >> taken from Mina's work in [1] with a slight modification of taking
> >> rtnl_lock() during the queue stop and start ops.
> >>
> >> For bnxt specifically, if the firmware doesn't support
> >> BNXT_RST_RING_SP_EVENT, then ndo_queue_stop() returns -EOPNOTSUPP and
> >> the whole restart fails. Unlike bnxt_rx_ring_reset(), there is no
> >> attempt to reset the whole device.
> >>
> >> [1]: https://lore.kernel.org/linux-kernel/20240403002053.2376017-6-almasrymina@google.com/#t
> >>
> >> Signed-off-by: David Wei <dw@davidwei.uk>
> > 
> > nit: Mina's From line is above, but there is no corresponding Signed-off-by
> >      line here.
> 
> This patch isn't a clean cherry pick, I pulled the core logic of
> netdev_rx_queue_restart() from the middle of another patch. In these
> cases should I be manually adding Signed-off-by tag?

As you asked:

I think if the patch is materially Mina's work - lets say more than 80% -
then a From line and a Signed-off-by tag is appropriate. N.B. this
implies Mina supplied a Signed-off-by tag at some point.

Otherwise I think it's fine to drop both the From line and Signed-off-by tag.
And as a courtesy acknowledge Mina's work some other way.

e.g. based on work by Mina Almasry <almasrymina@google.com>

But perhaps it's as well to as Mina what he thinks :)

...

Re: [RFC PATCH net-next v2 3/9] netdev: add netdev_rx_queue_restart()

[Mina Almasry]

On Tue, May 7, 2024 at 9:47 AM Simon Horman <horms@kernel.org> wrote:
>
> On Sun, May 05, 2024 at 05:41:14PM -0700, David Wei wrote:
> > On 2024-05-04 05:20, Simon Horman wrote:
> > > On Wed, May 01, 2024 at 09:54:04PM -0700, David Wei wrote:
> > >> From: Mina Almasry <almasrymina@google.com>
> > >>
> > >> Add netdev_rx_queue_restart() function to netdev_rx_queue.h. This is
> > >> taken from Mina's work in [1] with a slight modification of taking
> > >> rtnl_lock() during the queue stop and start ops.
> > >>
> > >> For bnxt specifically, if the firmware doesn't support
> > >> BNXT_RST_RING_SP_EVENT, then ndo_queue_stop() returns -EOPNOTSUPP and
> > >> the whole restart fails. Unlike bnxt_rx_ring_reset(), there is no
> > >> attempt to reset the whole device.
> > >>
> > >> [1]: https://lore.kernel.org/linux-kernel/20240403002053.2376017-6-almasrymina@google.com/#t
> > >>
> > >> Signed-off-by: David Wei <dw@davidwei.uk>
> > >
> > > nit: Mina's From line is above, but there is no corresponding Signed-off-by
> > >      line here.
> >
> > This patch isn't a clean cherry pick, I pulled the core logic of
> > netdev_rx_queue_restart() from the middle of another patch. In these
> > cases should I be manually adding Signed-off-by tag?
>
> As you asked:
>
> I think if the patch is materially Mina's work - lets say more than 80% -
> then a From line and a Signed-off-by tag is appropriate. N.B. this
> implies Mina supplied a Signed-off-by tag at some point.
>
> Otherwise I think it's fine to drop both the From line and Signed-off-by tag.
> And as a courtesy acknowledge Mina's work some other way.
>
> e.g. based on work by Mina Almasry <almasrymina@google.com>
>
> But perhaps it's as well to as Mina what he thinks :)
>

I'm fine with whatever here. This work is mostly off of Jakub's design
anyway. Either Signed-off-by or Suggested-by or 'based on work by' is
fine with me.

However from the other thread, it looks like David is delegating me to
send follow up versions of this, possibly with the Devmem TCP series,
which works better for me anyway :D

-- 
Thanks,
Mina

[RFC PATCH net-next v2 4/9] bnxt: refactor bnxt_{alloc,free}_one_rx_ring()

[David Wei]

Refactor bnxt_free_one_rx_ring_skbs() and bnxt_alloc_one_rx_ring() to
take the rx ring directly as a parameter instead of an index. This makes
the functions usable with an rx ring that is allocated outside of
bp->rx_ring.

Signed-off-by: David Wei <dw@davidwei.uk>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 788c87271eb1..7b20303f3b7d 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -3307,9 +3307,8 @@ static void bnxt_free_tx_skbs(struct bnxt *bp)
 	}
 }
 
-static void bnxt_free_one_rx_ring_skbs(struct bnxt *bp, int ring_nr)
+static void bnxt_free_one_rx_ring_skbs(struct bnxt *bp, struct bnxt_rx_ring_info *rxr)
 {
-	struct bnxt_rx_ring_info *rxr = &bp->rx_ring[ring_nr];
 	struct pci_dev *pdev = bp->pdev;
 	struct bnxt_tpa_idx_map *map;
 	int i, max_idx, max_agg_idx;
@@ -3389,7 +3388,7 @@ static void bnxt_free_rx_skbs(struct bnxt *bp)
 		return;
 
 	for (i = 0; i < bp->rx_nr_rings; i++)
-		bnxt_free_one_rx_ring_skbs(bp, i);
+		bnxt_free_one_rx_ring_skbs(bp, &bp->rx_ring[i]);
 }
 
 static void bnxt_free_skbs(struct bnxt *bp)
@@ -4051,9 +4050,8 @@ static void bnxt_init_rxbd_pages(struct bnxt_ring_struct *ring, u32 type)
 	}
 }
 
-static int bnxt_alloc_one_rx_ring(struct bnxt *bp, int ring_nr)
+static int bnxt_alloc_one_rx_ring(struct bnxt *bp, struct bnxt_rx_ring_info *rxr)
 {
-	struct bnxt_rx_ring_info *rxr = &bp->rx_ring[ring_nr];
 	struct net_device *dev = bp->dev;
 	u32 prod;
 	int i;
@@ -4062,7 +4060,7 @@ static int bnxt_alloc_one_rx_ring(struct bnxt *bp, int ring_nr)
 	for (i = 0; i < bp->rx_ring_size; i++) {
 		if (bnxt_alloc_rx_data(bp, rxr, prod, GFP_KERNEL)) {
 			netdev_warn(dev, "init'ed rx ring %d with %d/%d skbs only\n",
-				    ring_nr, i, bp->rx_ring_size);
+				    rxr->bnapi->index, i, bp->rx_ring_size);
 			break;
 		}
 		prod = NEXT_RX(prod);
@@ -4076,7 +4074,7 @@ static int bnxt_alloc_one_rx_ring(struct bnxt *bp, int ring_nr)
 	for (i = 0; i < bp->rx_agg_ring_size; i++) {
 		if (bnxt_alloc_rx_page(bp, rxr, prod, GFP_KERNEL)) {
 			netdev_warn(dev, "init'ed rx ring %d with %d/%d pages only\n",
-				    ring_nr, i, bp->rx_ring_size);
+				    rxr->bnapi->index, i, bp->rx_ring_size);
 			break;
 		}
 		prod = NEXT_RX_AGG(prod);
@@ -4135,7 +4133,7 @@ static int bnxt_init_one_rx_ring(struct bnxt *bp, int ring_nr)
 		bnxt_init_rxbd_pages(ring, type);
 	}
 
-	return bnxt_alloc_one_rx_ring(bp, ring_nr);
+	return bnxt_alloc_one_rx_ring(bp, rxr);
 }
 
 static void bnxt_init_cp_rings(struct bnxt *bp)
@@ -13238,13 +13236,13 @@ static void bnxt_rx_ring_reset(struct bnxt *bp)
 			bnxt_reset_task(bp, true);
 			break;
 		}
-		bnxt_free_one_rx_ring_skbs(bp, i);
+		bnxt_free_one_rx_ring_skbs(bp, rxr);
 		rxr->rx_prod = 0;
 		rxr->rx_agg_prod = 0;
 		rxr->rx_sw_agg_prod = 0;
 		rxr->rx_next_cons = 0;
 		rxr->bnapi->in_reset = false;
-		bnxt_alloc_one_rx_ring(bp, i);
+		bnxt_alloc_one_rx_ring(bp, rxr);
 		cpr = &rxr->bnapi->cp_ring;
 		cpr->sw_stats.rx.rx_resets++;
 		if (bp->flags & BNXT_FLAG_AGG_RINGS)
@@ -14826,7 +14824,7 @@ static int bnxt_queue_start(struct net_device *dev, int idx, void *qmem)
 	struct bnxt_rx_ring_info *rxr = qmem;
 	struct bnxt *bp = netdev_priv(dev);
 
-	bnxt_alloc_one_rx_ring(bp, idx);
+	bnxt_alloc_one_rx_ring(bp, rxr);
 
 	if (bp->flags & BNXT_FLAG_AGG_RINGS)
 		bnxt_db_write(bp, &rxr->rx_agg_db, rxr->rx_agg_prod);
@@ -14849,8 +14847,8 @@ static int bnxt_queue_stop(struct net_device *dev, int idx, void **out_qmem)
 	if (rc)
 		return rc;
 
-	bnxt_free_one_rx_ring_skbs(bp, idx);
 	rxr = &bp->rx_ring[idx];
+	bnxt_free_one_rx_ring_skbs(bp, rxr);
 	rxr->rx_prod = 0;
 	rxr->rx_agg_prod = 0;
 	rxr->rx_sw_agg_prod = 0;
-- 
2.43.0

[RFC PATCH net-next v2 5/9] bnxt: refactor bnxt_{alloc,free}_one_tpa_info()

[David Wei]

Refactor the allocation of each rx ring's tpa_info in
bnxt_alloc_tpa_info() out into a standalone function
__bnxt_alloc_one_tpa_info().

In case of allocation failures during bnxt_alloc_tpa_info(), clean up
in-place.

Change bnxt_free_tpa_info() to free a single rx ring passed in as a
parameter. This makes bnxt_free_rx_rings() more symmetrical.

Signed-off-by: David Wei <dw@davidwei.uk>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 95 +++++++++++++++--------
 1 file changed, 62 insertions(+), 33 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 7b20303f3b7d..bda49e7f6c3d 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -3500,29 +3500,66 @@ static int bnxt_alloc_ring(struct bnxt *bp, struct bnxt_ring_mem_info *rmem)
 	return 0;
 }
 
-static void bnxt_free_tpa_info(struct bnxt *bp)
+static void bnxt_free_tpa_info(struct bnxt *bp, struct bnxt_rx_ring_info *rxr)
 {
-	int i, j;
+	int i;
 
-	for (i = 0; i < bp->rx_nr_rings; i++) {
-		struct bnxt_rx_ring_info *rxr = &bp->rx_ring[i];
+	kfree(rxr->rx_tpa_idx_map);
+	rxr->rx_tpa_idx_map = NULL;
+	if (rxr->rx_tpa) {
+		for (i = 0; i < bp->max_tpa; i++) {
+			kfree(rxr->rx_tpa[i].agg_arr);
+			rxr->rx_tpa[i].agg_arr = NULL;
+		}
+	}
+	kfree(rxr->rx_tpa);
+	rxr->rx_tpa = NULL;
+}
 
-		kfree(rxr->rx_tpa_idx_map);
-		rxr->rx_tpa_idx_map = NULL;
-		if (rxr->rx_tpa) {
-			for (j = 0; j < bp->max_tpa; j++) {
-				kfree(rxr->rx_tpa[j].agg_arr);
-				rxr->rx_tpa[j].agg_arr = NULL;
-			}
+static int __bnxt_alloc_one_tpa_info(struct bnxt *bp, struct bnxt_rx_ring_info *rxr)
+{
+	struct rx_agg_cmp *agg;
+	int i, rc;
+
+	rxr->rx_tpa = kcalloc(bp->max_tpa, sizeof(struct bnxt_tpa_info),
+				GFP_KERNEL);
+	if (!rxr->rx_tpa)
+		return -ENOMEM;
+
+	if (!(bp->flags & BNXT_FLAG_CHIP_P5_PLUS))
+		return 0;
+
+	for (i = 0; i < bp->max_tpa; i++) {
+		agg = kcalloc(MAX_SKB_FRAGS, sizeof(*agg), GFP_KERNEL);
+		if (!agg) {
+			rc = -ENOMEM;
+			goto err_free;
 		}
-		kfree(rxr->rx_tpa);
-		rxr->rx_tpa = NULL;
+		rxr->rx_tpa[i].agg_arr = agg;
+	}
+	rxr->rx_tpa_idx_map = kzalloc(sizeof(*rxr->rx_tpa_idx_map),
+					GFP_KERNEL);
+	if (!rxr->rx_tpa_idx_map) {
+		rc = -ENOMEM;
+		goto err_free;
 	}
+
+	return 0;
+
+err_free:
+	while(i--) {
+		kfree(rxr->rx_tpa[i].agg_arr);
+		rxr->rx_tpa[i].agg_arr = NULL;
+	}
+	kfree(rxr->rx_tpa);
+	rxr->rx_tpa = NULL;
+
+	return rc;
 }
 
 static int bnxt_alloc_tpa_info(struct bnxt *bp)
 {
-	int i, j;
+	int i, rc;
 
 	bp->max_tpa = MAX_TPA;
 	if (bp->flags & BNXT_FLAG_CHIP_P5_PLUS) {
@@ -3533,27 +3570,18 @@ static int bnxt_alloc_tpa_info(struct bnxt *bp)
 
 	for (i = 0; i < bp->rx_nr_rings; i++) {
 		struct bnxt_rx_ring_info *rxr = &bp->rx_ring[i];
-		struct rx_agg_cmp *agg;
-
-		rxr->rx_tpa = kcalloc(bp->max_tpa, sizeof(struct bnxt_tpa_info),
-				      GFP_KERNEL);
-		if (!rxr->rx_tpa)
-			return -ENOMEM;
 
-		if (!(bp->flags & BNXT_FLAG_CHIP_P5_PLUS))
-			continue;
-		for (j = 0; j < bp->max_tpa; j++) {
-			agg = kcalloc(MAX_SKB_FRAGS, sizeof(*agg), GFP_KERNEL);
-			if (!agg)
-				return -ENOMEM;
-			rxr->rx_tpa[j].agg_arr = agg;
-		}
-		rxr->rx_tpa_idx_map = kzalloc(sizeof(*rxr->rx_tpa_idx_map),
-					      GFP_KERNEL);
-		if (!rxr->rx_tpa_idx_map)
-			return -ENOMEM;
+		rc = __bnxt_alloc_one_tpa_info(bp, rxr);
+		if (rc)
+			goto err_free;
 	}
 	return 0;
+
+err_free:
+	while (i--)
+		bnxt_free_tpa_info(bp, &bp->rx_ring[i]);
+
+	return rc;
 }
 
 static void bnxt_free_rx_rings(struct bnxt *bp)
@@ -3563,11 +3591,12 @@ static void bnxt_free_rx_rings(struct bnxt *bp)
 	if (!bp->rx_ring)
 		return;
 
-	bnxt_free_tpa_info(bp);
 	for (i = 0; i < bp->rx_nr_rings; i++) {
 		struct bnxt_rx_ring_info *rxr = &bp->rx_ring[i];
 		struct bnxt_ring_struct *ring;
 
+		bnxt_free_tpa_info(bp, rxr);
+
 		if (rxr->xdp_prog)
 			bpf_prog_put(rxr->xdp_prog);
 
-- 
2.43.0

Re: [RFC PATCH net-next v2 5/9] bnxt: refactor bnxt_{alloc,free}_one_tpa_info()

[Simon Horman]

On Wed, May 01, 2024 at 09:54:06PM -0700, David Wei wrote:
> Refactor the allocation of each rx ring's tpa_info in
> bnxt_alloc_tpa_info() out into a standalone function
> __bnxt_alloc_one_tpa_info().
> 
> In case of allocation failures during bnxt_alloc_tpa_info(), clean up
> in-place.
> 
> Change bnxt_free_tpa_info() to free a single rx ring passed in as a
> parameter. This makes bnxt_free_rx_rings() more symmetrical.
> 
> Signed-off-by: David Wei <dw@davidwei.uk>

Hi David,

Some minor nits flagged by

./scripts/checkpatch.pl --codespell --max-line-length=80 --strict

> ---
>  drivers/net/ethernet/broadcom/bnxt/bnxt.c | 95 +++++++++++++++--------
>  1 file changed, 62 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c

...

> +static int __bnxt_alloc_one_tpa_info(struct bnxt *bp, struct bnxt_rx_ring_info *rxr)
> +{

Please consider limiting Networking code to 80 columns wide where
it can be trivially achieved.

In this case, perhaps:

static int __bnxt_alloc_one_tpa_info(struct bnxt *bp,
				     struct bnxt_rx_ring_info *rxr)

> +	struct rx_agg_cmp *agg;
> +	int i, rc;
> +
> +	rxr->rx_tpa = kcalloc(bp->max_tpa, sizeof(struct bnxt_tpa_info),
> +				GFP_KERNEL);

The indentation here is not quite right.

	rxr->rx_tpa = kcalloc(bp->max_tpa, sizeof(struct bnxt_tpa_info),
			      GFP_KERNEL);

> +	if (!rxr->rx_tpa)
> +		return -ENOMEM;
> +
> +	if (!(bp->flags & BNXT_FLAG_CHIP_P5_PLUS))
> +		return 0;
> +
> +	for (i = 0; i < bp->max_tpa; i++) {
> +		agg = kcalloc(MAX_SKB_FRAGS, sizeof(*agg), GFP_KERNEL);
> +		if (!agg) {
> +			rc = -ENOMEM;
> +			goto err_free;
>  		}
> -		kfree(rxr->rx_tpa);
> -		rxr->rx_tpa = NULL;
> +		rxr->rx_tpa[i].agg_arr = agg;
> +	}
> +	rxr->rx_tpa_idx_map = kzalloc(sizeof(*rxr->rx_tpa_idx_map),
> +					GFP_KERNEL);
> +	if (!rxr->rx_tpa_idx_map) {
> +		rc = -ENOMEM;
> +		goto err_free;
>  	}
> +
> +	return 0;
> +
> +err_free:
> +	while(i--) {

Space before '(' here please.

> +		kfree(rxr->rx_tpa[i].agg_arr);
> +		rxr->rx_tpa[i].agg_arr = NULL;
> +	}
> +	kfree(rxr->rx_tpa);
> +	rxr->rx_tpa = NULL;
> +
> +	return rc;
>  }

...

Re: [RFC PATCH net-next v2 5/9] bnxt: refactor bnxt_{alloc,free}_one_tpa_info()

[David Wei]

On 2024-05-04 05:30, Simon Horman wrote:
> On Wed, May 01, 2024 at 09:54:06PM -0700, David Wei wrote:
>> Refactor the allocation of each rx ring's tpa_info in
>> bnxt_alloc_tpa_info() out into a standalone function
>> __bnxt_alloc_one_tpa_info().
>>
>> In case of allocation failures during bnxt_alloc_tpa_info(), clean up
>> in-place.
>>
>> Change bnxt_free_tpa_info() to free a single rx ring passed in as a
>> parameter. This makes bnxt_free_rx_rings() more symmetrical.
>>
>> Signed-off-by: David Wei <dw@davidwei.uk>
> 
> Hi David,
> 
> Some minor nits flagged by
> 
> ./scripts/checkpatch.pl --codespell --max-line-length=80 --strict

I didn't run through the usual checks because this is an RFC. I'll fix
it for the next series, thanks.

> 
>> ---
>>  drivers/net/ethernet/broadcom/bnxt/bnxt.c | 95 +++++++++++++++--------
>>  1 file changed, 62 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> 
> ...
> 
>> +static int __bnxt_alloc_one_tpa_info(struct bnxt *bp, struct bnxt_rx_ring_info *rxr)
>> +{
> 
> Please consider limiting Networking code to 80 columns wide where
> it can be trivially achieved.
> 
> In this case, perhaps:
> 
> static int __bnxt_alloc_one_tpa_info(struct bnxt *bp,
> 				     struct bnxt_rx_ring_info *rxr)
> 
>> +	struct rx_agg_cmp *agg;
>> +	int i, rc;
>> +
>> +	rxr->rx_tpa = kcalloc(bp->max_tpa, sizeof(struct bnxt_tpa_info),
>> +				GFP_KERNEL);
> 
> The indentation here is not quite right.
> 
> 	rxr->rx_tpa = kcalloc(bp->max_tpa, sizeof(struct bnxt_tpa_info),
> 			      GFP_KERNEL);
> 
>> +	if (!rxr->rx_tpa)
>> +		return -ENOMEM;
>> +
>> +	if (!(bp->flags & BNXT_FLAG_CHIP_P5_PLUS))
>> +		return 0;
>> +
>> +	for (i = 0; i < bp->max_tpa; i++) {
>> +		agg = kcalloc(MAX_SKB_FRAGS, sizeof(*agg), GFP_KERNEL);
>> +		if (!agg) {
>> +			rc = -ENOMEM;
>> +			goto err_free;
>>  		}
>> -		kfree(rxr->rx_tpa);
>> -		rxr->rx_tpa = NULL;
>> +		rxr->rx_tpa[i].agg_arr = agg;
>> +	}
>> +	rxr->rx_tpa_idx_map = kzalloc(sizeof(*rxr->rx_tpa_idx_map),
>> +					GFP_KERNEL);
>> +	if (!rxr->rx_tpa_idx_map) {
>> +		rc = -ENOMEM;
>> +		goto err_free;
>>  	}
>> +
>> +	return 0;
>> +
>> +err_free:
>> +	while(i--) {
> 
> Space before '(' here please.
> 
>> +		kfree(rxr->rx_tpa[i].agg_arr);
>> +		rxr->rx_tpa[i].agg_arr = NULL;
>> +	}
>> +	kfree(rxr->rx_tpa);
>> +	rxr->rx_tpa = NULL;
>> +
>> +	return rc;
>>  }
> 
> ...

[RFC PATCH net-next v2 6/9] bnxt: add __bnxt_init_rx_ring_struct() helper

[David Wei]

Move the initialisation of rx ring and rx agg ring structs into a helper
function.

Signed-off-by: David Wei <dw@davidwei.uk>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 44 +++++++++++++----------
 1 file changed, 26 insertions(+), 18 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index bda49e7f6c3d..b0a8d14b7319 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -3997,6 +3997,31 @@ static int bnxt_alloc_cp_rings(struct bnxt *bp)
 	return 0;
 }
 
+static void __bnxt_init_rx_ring_struct(struct bnxt *bp,
+				       struct bnxt_rx_ring_info *rxr)
+{
+	struct bnxt_ring_mem_info *rmem;
+	struct bnxt_ring_struct *ring;
+
+	ring = &rxr->rx_ring_struct;
+	rmem = &ring->ring_mem;
+	rmem->nr_pages = bp->rx_nr_pages;
+	rmem->page_size = HW_RXBD_RING_SIZE;
+	rmem->pg_arr = (void **)rxr->rx_desc_ring;
+	rmem->dma_arr = rxr->rx_desc_mapping;
+	rmem->vmem_size = SW_RXBD_RING_SIZE * bp->rx_nr_pages;
+	rmem->vmem = (void **)&rxr->rx_buf_ring;
+
+	ring = &rxr->rx_agg_ring_struct;
+	rmem = &ring->ring_mem;
+	rmem->nr_pages = bp->rx_agg_nr_pages;
+	rmem->page_size = HW_RXBD_RING_SIZE;
+	rmem->pg_arr = (void **)rxr->rx_agg_desc_ring;
+	rmem->dma_arr = rxr->rx_agg_desc_mapping;
+	rmem->vmem_size = SW_RXBD_AGG_RING_SIZE * bp->rx_agg_nr_pages;
+	rmem->vmem = (void **)&rxr->rx_agg_ring;
+}
+
 static void bnxt_init_ring_struct(struct bnxt *bp)
 {
 	int i, j;
@@ -4024,24 +4049,7 @@ static void bnxt_init_ring_struct(struct bnxt *bp)
 		rxr = bnapi->rx_ring;
 		if (!rxr)
 			goto skip_rx;
-
-		ring = &rxr->rx_ring_struct;
-		rmem = &ring->ring_mem;
-		rmem->nr_pages = bp->rx_nr_pages;
-		rmem->page_size = HW_RXBD_RING_SIZE;
-		rmem->pg_arr = (void **)rxr->rx_desc_ring;
-		rmem->dma_arr = rxr->rx_desc_mapping;
-		rmem->vmem_size = SW_RXBD_RING_SIZE * bp->rx_nr_pages;
-		rmem->vmem = (void **)&rxr->rx_buf_ring;
-
-		ring = &rxr->rx_agg_ring_struct;
-		rmem = &ring->ring_mem;
-		rmem->nr_pages = bp->rx_agg_nr_pages;
-		rmem->page_size = HW_RXBD_RING_SIZE;
-		rmem->pg_arr = (void **)rxr->rx_agg_desc_ring;
-		rmem->dma_arr = rxr->rx_agg_desc_mapping;
-		rmem->vmem_size = SW_RXBD_AGG_RING_SIZE * bp->rx_agg_nr_pages;
-		rmem->vmem = (void **)&rxr->rx_agg_ring;
+		__bnxt_init_rx_ring_struct(bp, rxr);
 
 skip_rx:
 		bnxt_for_each_napi_tx(j, bnapi, txr) {
-- 
2.43.0

[RFC PATCH net-next v2 7/9] bnxt: add helpers for allocating rx ring mem

[David Wei]

Add several helper functions for allocating rx ring memory. These are
mostly taken from existing functions, but with unnecessary bits stripped
out such that only allocations are done.

Signed-off-by: David Wei <dw@davidwei.uk>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 87 +++++++++++++++++++++++
 1 file changed, 87 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index b0a8d14b7319..21c1a7cb70ab 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -14845,6 +14845,93 @@ static const struct netdev_stat_ops bnxt_stat_ops = {
 	.get_base_stats		= bnxt_get_base_stats,
 };
 
+static int __bnxt_alloc_rx_desc_ring(struct pci_dev *pdev, struct bnxt_ring_mem_info *rmem)
+{
+	int i, rc;
+
+	for (i = 0; i < rmem->nr_pages; i++) {
+		rmem->pg_arr[i] = dma_alloc_coherent(&pdev->dev,
+						     rmem->page_size,
+						     &rmem->dma_arr[i],
+						     GFP_KERNEL);
+		if (!rmem->pg_arr[i]) {
+			rc = -ENOMEM;
+			goto err_free;
+		}
+	}
+
+	return 0;
+
+err_free:
+	while (i--) {
+		dma_free_coherent(&pdev->dev, rmem->page_size,
+				  rmem->pg_arr[i], rmem->dma_arr[i]);
+		rmem->pg_arr[i] = NULL;
+	}
+	return rc;
+}
+
+static int bnxt_alloc_rx_ring_struct(struct bnxt *bp, struct bnxt_ring_struct *ring)
+{
+	struct bnxt_ring_mem_info *rmem;
+	int rc;
+
+	rmem = &ring->ring_mem;
+	rc = __bnxt_alloc_rx_desc_ring(bp->pdev, rmem);
+	if (rc)
+		return rc;
+
+	*rmem->vmem = vzalloc(rmem->vmem_size);
+	if (!(*rmem->vmem)) {
+		rc = -ENOMEM;
+		goto err_free;
+	}
+
+	return 0;
+
+err_free:
+	bnxt_free_ring(bp, rmem);
+	return rc;
+}
+
+static int bnxt_alloc_rx_agg_bmap(struct bnxt *bp, struct bnxt_rx_ring_info *rxr)
+{
+	u16 mem_size;
+
+	rxr->rx_agg_bmap_size = bp->rx_agg_ring_mask + 1;
+	mem_size = rxr->rx_agg_bmap_size / 8;
+	rxr->rx_agg_bmap = kzalloc(mem_size, GFP_KERNEL);
+	if (!rxr->rx_agg_bmap)
+		return -ENOMEM;
+
+	return 0;
+}
+
+static void bnxt_init_rx_ring_rxbd_pages(struct bnxt *bp, struct bnxt_rx_ring_info *rxr)
+{
+	struct bnxt_ring_struct *ring;
+	u32 type;
+
+	type = (bp->rx_buf_use_size << RX_BD_LEN_SHIFT) |
+		RX_BD_TYPE_RX_PACKET_BD | RX_BD_FLAGS_EOP;
+
+	if (NET_IP_ALIGN == 2)
+		type |= RX_BD_FLAGS_SOP;
+
+	ring = &rxr->rx_ring_struct;
+	ring->fw_ring_id = INVALID_HW_RING_ID;
+	bnxt_init_rxbd_pages(ring, type);
+
+	ring = &rxr->rx_agg_ring_struct;
+	ring->fw_ring_id = INVALID_HW_RING_ID;
+	if ((bp->flags & BNXT_FLAG_AGG_RINGS)) {
+		type = ((u32)BNXT_RX_PAGE_SIZE << RX_BD_LEN_SHIFT) |
+			RX_BD_TYPE_RX_AGG_BD | RX_BD_FLAGS_SOP;
+
+		bnxt_init_rxbd_pages(ring, type);
+	}
+}
+
 static void *bnxt_queue_mem_alloc(struct net_device *dev, int idx)
 {
 	struct bnxt *bp = netdev_priv(dev);
-- 
2.43.0

Re: [RFC PATCH net-next v2 7/9] bnxt: add helpers for allocating rx ring mem

[Simon Horman]

On Wed, May 01, 2024 at 09:54:08PM -0700, David Wei wrote:
> Add several helper functions for allocating rx ring memory. These are
> mostly taken from existing functions, but with unnecessary bits stripped
> out such that only allocations are done.
> 
> Signed-off-by: David Wei <dw@davidwei.uk>
> ---
>  drivers/net/ethernet/broadcom/bnxt/bnxt.c | 87 +++++++++++++++++++++++
>  1 file changed, 87 insertions(+)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index b0a8d14b7319..21c1a7cb70ab 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -14845,6 +14845,93 @@ static const struct netdev_stat_ops bnxt_stat_ops = {
>  	.get_base_stats		= bnxt_get_base_stats,
>  };
>  
> +static int __bnxt_alloc_rx_desc_ring(struct pci_dev *pdev, struct bnxt_ring_mem_info *rmem)
> +{
> +	int i, rc;
> +
> +	for (i = 0; i < rmem->nr_pages; i++) {
> +		rmem->pg_arr[i] = dma_alloc_coherent(&pdev->dev,
> +						     rmem->page_size,
> +						     &rmem->dma_arr[i],
> +						     GFP_KERNEL);
> +		if (!rmem->pg_arr[i]) {
> +			rc = -ENOMEM;
> +			goto err_free;
> +		}
> +	}
> +
> +	return 0;
> +
> +err_free:
> +	while (i--) {
> +		dma_free_coherent(&pdev->dev, rmem->page_size,
> +				  rmem->pg_arr[i], rmem->dma_arr[i]);
> +		rmem->pg_arr[i] = NULL;
> +	}
> +	return rc;
> +}
> +
> +static int bnxt_alloc_rx_ring_struct(struct bnxt *bp, struct bnxt_ring_struct *ring)

Hi David,

W=1 builds fail because this and other functions introduced by
this patch are unused. I agree that it is nice to split up changes
into discrete patches. But in this case the change isn't really discrete.
So perhaps it is best to add helper functions in the same patch
where they are first used.

Re: [RFC PATCH net-next v2 7/9] bnxt: add helpers for allocating rx ring mem

[David Wei]

On 2024-05-04 05:34, Simon Horman wrote:
> On Wed, May 01, 2024 at 09:54:08PM -0700, David Wei wrote:
>> Add several helper functions for allocating rx ring memory. These are
>> mostly taken from existing functions, but with unnecessary bits stripped
>> out such that only allocations are done.
>>
>> Signed-off-by: David Wei <dw@davidwei.uk>
>> ---
>>  drivers/net/ethernet/broadcom/bnxt/bnxt.c | 87 +++++++++++++++++++++++
>>  1 file changed, 87 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>> index b0a8d14b7319..21c1a7cb70ab 100644
>> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>> @@ -14845,6 +14845,93 @@ static const struct netdev_stat_ops bnxt_stat_ops = {
>>  	.get_base_stats		= bnxt_get_base_stats,
>>  };
>>  
>> +static int __bnxt_alloc_rx_desc_ring(struct pci_dev *pdev, struct bnxt_ring_mem_info *rmem)
>> +{
>> +	int i, rc;
>> +
>> +	for (i = 0; i < rmem->nr_pages; i++) {
>> +		rmem->pg_arr[i] = dma_alloc_coherent(&pdev->dev,
>> +						     rmem->page_size,
>> +						     &rmem->dma_arr[i],
>> +						     GFP_KERNEL);
>> +		if (!rmem->pg_arr[i]) {
>> +			rc = -ENOMEM;
>> +			goto err_free;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +
>> +err_free:
>> +	while (i--) {
>> +		dma_free_coherent(&pdev->dev, rmem->page_size,
>> +				  rmem->pg_arr[i], rmem->dma_arr[i]);
>> +		rmem->pg_arr[i] = NULL;
>> +	}
>> +	return rc;
>> +}
>> +
>> +static int bnxt_alloc_rx_ring_struct(struct bnxt *bp, struct bnxt_ring_struct *ring)
> 
> Hi David,
> 
> W=1 builds fail because this and other functions introduced by
> this patch are unused. I agree that it is nice to split up changes
> into discrete patches. But in this case the change isn't really discrete.
> So perhaps it is best to add helper functions in the same patch
> where they are first used.

Okay, I'll squash it with the patch that uses these functions. Thanks.

[RFC PATCH net-next v2 8/9] bnxt: alloc rx ring mem first before reset

[David Wei]

Implement allocating rx ring mem in bnxt_queue_mem_alloc(). This is done
by duplicating the existing rx ring entirely, then allocating new memory
into this clone as most functions take rx ring as an argument.

I've identified the following memory that gets allocated:

* rx_desc_ring
  * separate allocation per hw page
* rx_buf_ring
* rx_agg_desc_ring
* rx_agg_ring
* rx_agg_bmap
* rx_tpa
* rx_tpa_idx_map

So, zero the ring heads, alloc the rings, then call
bnxt_alloc_one_rx_ring() to fills in the descriptors.

It's interesting that struct bnxt_ring_mem_info points to addresses of
stack allocated arrays within struct bnxt_rx_ring_info, instead of the
heap allocated queue memory. __bnxt_init_rx_ring_struct() is first
called to reconfigure these ptrs for the clone.

The hardware is only aware of the pg_tbl, which we do not touch. In the
coming patches after an rx ring has been quiesced, we'll swap the bits
that are dynamically allocated then update the dma mappings in pg_tbl.
Then the hardware is none the wiser!

Signed-off-by: David Wei <dw@davidwei.uk>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 78 ++++++++++++++++++++++-
 drivers/net/ethernet/broadcom/bnxt/bnxt.h |  2 +
 2 files changed, 79 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 21c1a7cb70ab..d848b9ceabf0 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -14934,13 +14934,89 @@ static void bnxt_init_rx_ring_rxbd_pages(struct bnxt *bp, struct bnxt_rx_ring_in
 
 static void *bnxt_queue_mem_alloc(struct net_device *dev, int idx)
 {
+	struct bnxt_rx_ring_info *rxr, *clone;
 	struct bnxt *bp = netdev_priv(dev);
+	int rc;
+
+	rxr = &bp->rx_ring[idx];
+	clone = kmemdup(rxr, sizeof(*rxr), GFP_KERNEL);
+	if (!clone)
+		return ERR_PTR(-ENOMEM);
+
+	clone->rx_prod = 0;
+	clone->rx_agg_prod = 0;
+	clone->rx_sw_agg_prod = 0;
+	clone->rx_next_cons = 0;
+
+	__bnxt_init_rx_ring_struct(bp, clone);
+
+	rc = bnxt_alloc_rx_page_pool(bp, clone, rxr->page_pool->p.nid);
+	if (rc)
+		goto err_free_clone;
+
+	rc = bnxt_alloc_rx_ring_struct(bp, &clone->rx_ring_struct);
+	if (rc)
+		goto err_free_page_pool;
 
-	return &bp->rx_ring[idx];
+	if (bp->flags & BNXT_FLAG_AGG_RINGS) {
+		rc = bnxt_alloc_rx_ring_struct(bp, &clone->rx_agg_ring_struct);
+		if (rc)
+			goto err_free_rx_ring;
+
+		rc = bnxt_alloc_rx_agg_bmap(bp, clone);
+		if (rc)
+			goto err_free_rx_agg_ring;
+	}
+
+	if (bp->flags & BNXT_FLAG_TPA) {
+		rc = __bnxt_alloc_one_tpa_info(bp, clone);
+		if (rc)
+			goto err_free_rx_agg_bmap;
+	}
+
+	bnxt_init_rx_ring_rxbd_pages(bp, clone);
+	bnxt_alloc_one_rx_ring(bp, clone);
+
+	rxr->rplc = clone;
+
+	return clone;
+
+err_free_rx_agg_bmap:
+	kfree(clone->rx_agg_bmap);
+err_free_rx_agg_ring:
+	bnxt_free_ring(bp, &clone->rx_agg_ring_struct.ring_mem);
+err_free_rx_ring:
+	bnxt_free_ring(bp, &clone->rx_ring_struct.ring_mem);
+err_free_page_pool:
+	page_pool_destroy(clone->page_pool);
+	rxr->page_pool = NULL;
+err_free_clone:
+	kfree(clone);
+
+	return ERR_PTR(rc);
 }
 
 static void bnxt_queue_mem_free(struct net_device *dev, void *qmem)
 {
+	struct bnxt_rx_ring_info *rxr = qmem;
+	struct bnxt *bp = netdev_priv(dev);
+	struct bnxt_ring_struct *ring;
+
+	bnxt_free_tpa_info(bp, rxr);
+
+	page_pool_destroy(rxr->page_pool);
+	rxr->page_pool = NULL;
+
+	kfree(rxr->rx_agg_bmap);
+	rxr->rx_agg_bmap = NULL;
+
+	ring = &rxr->rx_ring_struct;
+	bnxt_free_ring(bp, &ring->ring_mem);
+
+	ring = &rxr->rx_agg_ring_struct;
+	bnxt_free_ring(bp, &ring->ring_mem);
+
+	kfree(rxr);
 }
 
 static int bnxt_queue_start(struct net_device *dev, int idx, void *qmem)
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
index ad57ef051798..ce2aa48911bb 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
@@ -1076,6 +1076,8 @@ struct bnxt_rx_ring_info {
 	struct bnxt_ring_struct	rx_agg_ring_struct;
 	struct xdp_rxq_info	xdp_rxq;
 	struct page_pool	*page_pool;
+
+	struct bnxt_rx_ring_info	*rplc;
 };
 
 struct bnxt_rx_sw_stats {
-- 
2.43.0

[RFC PATCH net-next v2 9/9] bnxt: swap rx ring mem during queue_stop()

[David Wei]

After stopping an rx ring in bnxt_queue_stop(), swap the preallocated
ring memory into the existing rx ring that the hardware is aware of. As
discussed in the last patch, the hardware ring is associated with the
address of static arrays in struct bnxt_rx_ring_info. For example:

struct bnxt_rx_ring_info              struct bnxt_ring_mem_info

struct rx_bd *rx_desc_ring[MAX]   <-> void **pg_arr
struct bnxt_sw_rx_bd *rx_buf_ring <-> void **vmem

The pg_tbl that is registered w/ the hardware via HWRM contains an array
of dma mappings to the pg_arr above. We can't touch this association
during reset, so can't simply swap the ring and its clone directly.

Instead, swap the ring memory only then update the pg_tbl. Functionally
it should be the same as the existing bnxt_rx_ring_reset(), except the
allocations happen before resetting the ring using a clone struct
bnxt_rx_ring_info.

Signed-off-by: David Wei <dw@davidwei.uk>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 54 +++++++++++++++++------
 1 file changed, 41 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index d848b9ceabf0..4dd4aa0911b1 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -15024,8 +15024,6 @@ static int bnxt_queue_start(struct net_device *dev, int idx, void *qmem)
 	struct bnxt_rx_ring_info *rxr = qmem;
 	struct bnxt *bp = netdev_priv(dev);
 
-	bnxt_alloc_one_rx_ring(bp, rxr);
-
 	if (bp->flags & BNXT_FLAG_AGG_RINGS)
 		bnxt_db_write(bp, &rxr->rx_agg_db, rxr->rx_agg_prod);
 	bnxt_db_write(bp, &rxr->rx_db, rxr->rx_prod);
@@ -15038,26 +15036,56 @@ static int bnxt_queue_start(struct net_device *dev, int idx, void *qmem)
 
 static int bnxt_queue_stop(struct net_device *dev, int idx, void **out_qmem)
 {
+	struct bnxt_rx_ring_info *orig, *rplc;
 	struct bnxt *bp = netdev_priv(dev);
-	struct bnxt_rx_ring_info *rxr;
+	struct bnxt_ring_mem_info *rmem;
 	struct bnxt_cp_ring_info *cpr;
-	int rc;
+	int i, rc;
 
 	rc = bnxt_hwrm_rx_ring_reset(bp, idx);
 	if (rc)
 		return rc;
 
-	rxr = &bp->rx_ring[idx];
-	bnxt_free_one_rx_ring_skbs(bp, rxr);
-	rxr->rx_prod = 0;
-	rxr->rx_agg_prod = 0;
-	rxr->rx_sw_agg_prod = 0;
-	rxr->rx_next_cons = 0;
-
-	cpr = &rxr->bnapi->cp_ring;
+	/* HW ring is registered w/ the original bnxt_rx_ring_info so we cannot
+	 * do a direct swap between orig and rplc. Instead, swap the
+	 * dynamically allocated queue memory and then update pg_tbl.
+	 */
+	orig = &bp->rx_ring[idx];
+	rplc = orig->rplc;
+
+	swap(orig->rx_prod, rplc->rx_prod);
+	swap(orig->rx_agg_prod, rplc->rx_agg_prod);
+	swap(orig->rx_sw_agg_prod, rplc->rx_sw_agg_prod);
+	swap(orig->rx_next_cons, rplc->rx_next_cons);
+
+	for (i = 0; i < MAX_RX_PAGES; i++) {
+		swap(orig->rx_desc_ring[i], rplc->rx_desc_ring[i]);
+		swap(orig->rx_desc_mapping[i], rplc->rx_desc_mapping[i]);
+
+		swap(orig->rx_agg_desc_ring[i], rplc->rx_agg_desc_ring[i]);
+		swap(orig->rx_agg_desc_mapping[i], rplc->rx_agg_desc_mapping[i]);
+	}
+	swap(orig->rx_buf_ring, rplc->rx_buf_ring);
+	swap(orig->rx_agg_ring, rplc->rx_agg_ring);
+	swap(orig->rx_agg_bmap, rplc->rx_agg_bmap);
+	swap(orig->rx_agg_bmap_size, rplc->rx_agg_bmap_size);
+	swap(orig->rx_tpa, rplc->rx_tpa);
+	swap(orig->rx_tpa_idx_map, rplc->rx_tpa_idx_map);
+	swap(orig->page_pool, rplc->page_pool);
+
+	rmem = &orig->rx_ring_struct.ring_mem;
+	for (i = 0; i < rmem->nr_pages; i++)
+		rmem->pg_tbl[i] = cpu_to_le64(rmem->dma_arr[i]);
+
+	rmem = &rplc->rx_ring_struct.ring_mem;
+	for (i = 0; i < rmem->nr_pages; i++)
+		rmem->pg_tbl[i] = cpu_to_le64(rmem->dma_arr[i]);
+
+	cpr = &orig->bnapi->cp_ring;
 	cpr->sw_stats.rx.rx_resets++;
 
-	*out_qmem = rxr;
+	*out_qmem = rplc;
+	orig->rplc = NULL;
 
 	return 0;
 }
-- 
2.43.0