[PATCH net 0/2] pds_core: fixes for adminq overflow

[PATCH net 0/2] pds_core: fixes for adminq overflow

[Shannon Nelson]

With recent stress testing we found we could break things by
running a large number of VF client requests at the same time.
Brett takes care of this with a couple of fixes to our adminq
handling to add a tighter limit and a retry mechanism.

Brett Creeley (2):
  pds_core: Prevent possible adminq overflow/stuck condition
  pds_core: Add a retry mechanism when the adminq is full

 drivers/net/ethernet/amd/pds_core/adminq.c | 22 ++++++++++++++++++----
 drivers/net/ethernet/amd/pds_core/core.c   |  5 +----
 drivers/net/ethernet/amd/pds_core/core.h   |  2 +-
 include/linux/pds/pds_core_if.h            |  2 +-
 4 files changed, 21 insertions(+), 10 deletions(-)

-- 
2.17.1

[PATCH net 1/2] pds_core: Prevent possible adminq overflow/stuck condition

[Shannon Nelson]

From: Brett Creeley <brett.creeley@amd.com>

The pds_core's adminq is protected by the adminq_lock, which prevents
more than 1 command to be posted onto it at any one time. This makes it
so the client drivers cannot simultaneously post adminq commands.
However, the completions happen in a different context, which means
multiple adminq commands can be posted sequentially and all waiting
on completion.

On the FW side, the backing adminq request queue is only 16 entries
long and the retry mechanism and/or overflow/stuck prevention is
lacking. This can cause the adminq to get stuck, so commands are no
longer processed and completions are no longer sent by the FW.

As an initial fix, prevent more than 16 outstanding adminq commands so
there's no way to cause the adminq from getting stuck. This works
because the backing adminq request queue will never have more than 16
pending adminq commands, so it will never overflow. This is done by
reducing the adminq depth to 16.

This is just the first step to fix this issue because there are already
devices being used. Moving forward a new capability bit will be defined
and set if the FW can gracefully handle the host driver/device having a
deeper adminq.

Fixes: 792d36ccc163 ("pds_core: Clean up init/uninit flows to be more readable")
Signed-off-by: Brett Creeley <brett.creeley@amd.com>
Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
---
 drivers/net/ethernet/amd/pds_core/core.c | 5 +----
 drivers/net/ethernet/amd/pds_core/core.h | 2 +-
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/amd/pds_core/core.c b/drivers/net/ethernet/amd/pds_core/core.c
index 536635e57727..4830292d5f87 100644
--- a/drivers/net/ethernet/amd/pds_core/core.c
+++ b/drivers/net/ethernet/amd/pds_core/core.c
@@ -325,10 +325,7 @@ static int pdsc_core_init(struct pdsc *pdsc)
 	size_t sz;
 	int err;
 
-	/* Scale the descriptor ring length based on number of CPUs and VFs */
-	numdescs = max_t(int, PDSC_ADMINQ_MIN_LENGTH, num_online_cpus());
-	numdescs += 2 * pci_sriov_get_totalvfs(pdsc->pdev);
-	numdescs = roundup_pow_of_two(numdescs);
+	numdescs = PDSC_ADMINQ_MAX_LENGTH;
 	err = pdsc_qcq_alloc(pdsc, PDS_CORE_QTYPE_ADMINQ, 0, "adminq",
 			     PDS_CORE_QCQ_F_CORE | PDS_CORE_QCQ_F_INTR,
 			     numdescs,
diff --git a/drivers/net/ethernet/amd/pds_core/core.h b/drivers/net/ethernet/amd/pds_core/core.h
index 14522d6d5f86..543097983bf6 100644
--- a/drivers/net/ethernet/amd/pds_core/core.h
+++ b/drivers/net/ethernet/amd/pds_core/core.h
@@ -16,7 +16,7 @@
 
 #define PDSC_WATCHDOG_SECS	5
 #define PDSC_QUEUE_NAME_MAX_SZ  16
-#define PDSC_ADMINQ_MIN_LENGTH	16	/* must be a power of two */
+#define PDSC_ADMINQ_MAX_LENGTH	16	/* must be a power of two */
 #define PDSC_NOTIFYQ_LENGTH	64	/* must be a power of two */
 #define PDSC_TEARDOWN_RECOVERY	false
 #define PDSC_TEARDOWN_REMOVING	true
-- 
2.17.1

[PATCH net 2/2] pds_core: Add a retry mechanism when the adminq is full

[Shannon Nelson]

From: Brett Creeley <brett.creeley@amd.com>

If the adminq is full, the driver reports failure when trying to post
new adminq commands. This is a bit aggressive and unexpected because
technically the adminq post didn't fail in this case, it was just full.
To harden this path add support for a bounded retry mechanism.

It's possible some commands take longer than expected, maybe hundreds
of milliseconds or seconds due to other processing on the device side,
so to further reduce the chance of failure due to adminq full increase
the PDS_CORE_DEVCMD_TIMEOUT from 5 to 10 seconds.

The caller of pdsc_adminq_post() may still see -EAGAIN reported if the
space in the adminq never freed up. In this case they can choose to
call the function again or fail. For now, no callers will retry.

Fixes: 01ba61b55b20 ("pds_core: Add adminq processing and commands")
Signed-off-by: Brett Creeley <brett.creeley@amd.com>
Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
---
 drivers/net/ethernet/amd/pds_core/adminq.c | 22 ++++++++++++++++++----
 include/linux/pds/pds_core_if.h            |  2 +-
 2 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/amd/pds_core/adminq.c b/drivers/net/ethernet/amd/pds_core/adminq.c
index c83a0a80d533..387de1712827 100644
--- a/drivers/net/ethernet/amd/pds_core/adminq.c
+++ b/drivers/net/ethernet/amd/pds_core/adminq.c
@@ -181,7 +181,10 @@ static int __pdsc_adminq_post(struct pdsc *pdsc,
 	else
 		avail -= q->head_idx + 1;
 	if (!avail) {
-		ret = -ENOSPC;
+		if (!pdsc_is_fw_running(pdsc))
+			ret = -ENXIO;
+		else
+			ret = -EAGAIN;
 		goto err_out_unlock;
 	}
 
@@ -251,14 +254,25 @@ int pdsc_adminq_post(struct pdsc *pdsc,
 	}
 
 	wc.qcq = &pdsc->adminqcq;
-	index = __pdsc_adminq_post(pdsc, &pdsc->adminqcq, cmd, comp, &wc);
+	time_start = jiffies;
+	time_limit = time_start + HZ * pdsc->devcmd_timeout;
+	do {
+		index = __pdsc_adminq_post(pdsc, &pdsc->adminqcq, cmd, comp,
+					   &wc);
+		if (index != -EAGAIN)
+			break;
+
+		dev_dbg(pdsc->dev, "Retrying adminq cmd opcode %u\n",
+			cmd->opcode);
+		/* Give completion processing a chance to free up space */
+		msleep(1);
+	} while (time_before(jiffies, time_limit));
+
 	if (index < 0) {
 		err = index;
 		goto err_out;
 	}
 
-	time_start = jiffies;
-	time_limit = time_start + HZ * pdsc->devcmd_timeout;
 	do {
 		/* Timeslice the actual wait to catch IO errors etc early */
 		poll_jiffies = msecs_to_jiffies(poll_interval);
diff --git a/include/linux/pds/pds_core_if.h b/include/linux/pds/pds_core_if.h
index 17a87c1a55d7..babc6d573acd 100644
--- a/include/linux/pds/pds_core_if.h
+++ b/include/linux/pds/pds_core_if.h
@@ -22,7 +22,7 @@
 #define PDS_CORE_BAR0_INTR_CTRL_OFFSET		0x2000
 #define PDS_CORE_DEV_CMD_DONE			0x00000001
 
-#define PDS_CORE_DEVCMD_TIMEOUT			5
+#define PDS_CORE_DEVCMD_TIMEOUT			10
 
 #define PDS_CORE_CLIENT_ID			0
 #define PDS_CORE_ASIC_TYPE_CAPRI		0
-- 
2.17.1

Re: [PATCH net 1/2] pds_core: Prevent possible adminq overflow/stuck condition

[Michal Swiatkowski]

On Tue, Jan 28, 2025 at 04:43:36PM -0800, Shannon Nelson wrote:
> From: Brett Creeley <brett.creeley@amd.com>
> 
> The pds_core's adminq is protected by the adminq_lock, which prevents
> more than 1 command to be posted onto it at any one time. This makes it
> so the client drivers cannot simultaneously post adminq commands.
> However, the completions happen in a different context, which means
> multiple adminq commands can be posted sequentially and all waiting
> on completion.
> 
> On the FW side, the backing adminq request queue is only 16 entries
> long and the retry mechanism and/or overflow/stuck prevention is
> lacking. This can cause the adminq to get stuck, so commands are no
> longer processed and completions are no longer sent by the FW.
> 
> As an initial fix, prevent more than 16 outstanding adminq commands so
> there's no way to cause the adminq from getting stuck. This works
> because the backing adminq request queue will never have more than 16
> pending adminq commands, so it will never overflow. This is done by
> reducing the adminq depth to 16.
> 
> This is just the first step to fix this issue because there are already
> devices being used. Moving forward a new capability bit will be defined
> and set if the FW can gracefully handle the host driver/device having a
> deeper adminq.
> 
> Fixes: 792d36ccc163 ("pds_core: Clean up init/uninit flows to be more readable")
> Signed-off-by: Brett Creeley <brett.creeley@amd.com>
> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
> ---
>  drivers/net/ethernet/amd/pds_core/core.c | 5 +----
>  drivers/net/ethernet/amd/pds_core/core.h | 2 +-
>  2 files changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/amd/pds_core/core.c b/drivers/net/ethernet/amd/pds_core/core.c
> index 536635e57727..4830292d5f87 100644
> --- a/drivers/net/ethernet/amd/pds_core/core.c
> +++ b/drivers/net/ethernet/amd/pds_core/core.c
> @@ -325,10 +325,7 @@ static int pdsc_core_init(struct pdsc *pdsc)
>  	size_t sz;
>  	int err;
>  
> -	/* Scale the descriptor ring length based on number of CPUs and VFs */
> -	numdescs = max_t(int, PDSC_ADMINQ_MIN_LENGTH, num_online_cpus());
> -	numdescs += 2 * pci_sriov_get_totalvfs(pdsc->pdev);
> -	numdescs = roundup_pow_of_two(numdescs);
> +	numdescs = PDSC_ADMINQ_MAX_LENGTH;
>  	err = pdsc_qcq_alloc(pdsc, PDS_CORE_QTYPE_ADMINQ, 0, "adminq",
>  			     PDS_CORE_QCQ_F_CORE | PDS_CORE_QCQ_F_INTR,
>  			     numdescs,
> diff --git a/drivers/net/ethernet/amd/pds_core/core.h b/drivers/net/ethernet/amd/pds_core/core.h
> index 14522d6d5f86..543097983bf6 100644
> --- a/drivers/net/ethernet/amd/pds_core/core.h
> +++ b/drivers/net/ethernet/amd/pds_core/core.h
> @@ -16,7 +16,7 @@
>  
>  #define PDSC_WATCHDOG_SECS	5
>  #define PDSC_QUEUE_NAME_MAX_SZ  16
> -#define PDSC_ADMINQ_MIN_LENGTH	16	/* must be a power of two */
> +#define PDSC_ADMINQ_MAX_LENGTH	16	/* must be a power of two */
>  #define PDSC_NOTIFYQ_LENGTH	64	/* must be a power of two */
>  #define PDSC_TEARDOWN_RECOVERY	false
>  #define PDSC_TEARDOWN_REMOVING	true
> -- 

Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>

> 2.17.1

Re: [PATCH net 2/2] pds_core: Add a retry mechanism when the adminq is full

[Michal Swiatkowski]

On Tue, Jan 28, 2025 at 04:43:37PM -0800, Shannon Nelson wrote:
> From: Brett Creeley <brett.creeley@amd.com>
> 
> If the adminq is full, the driver reports failure when trying to post
> new adminq commands. This is a bit aggressive and unexpected because
> technically the adminq post didn't fail in this case, it was just full.
> To harden this path add support for a bounded retry mechanism.
> 
> It's possible some commands take longer than expected, maybe hundreds
> of milliseconds or seconds due to other processing on the device side,
> so to further reduce the chance of failure due to adminq full increase
> the PDS_CORE_DEVCMD_TIMEOUT from 5 to 10 seconds.
> 
> The caller of pdsc_adminq_post() may still see -EAGAIN reported if the
> space in the adminq never freed up. In this case they can choose to
> call the function again or fail. For now, no callers will retry.
> 
> Fixes: 01ba61b55b20 ("pds_core: Add adminq processing and commands")
> Signed-off-by: Brett Creeley <brett.creeley@amd.com>
> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
> ---
>  drivers/net/ethernet/amd/pds_core/adminq.c | 22 ++++++++++++++++++----
>  include/linux/pds/pds_core_if.h            |  2 +-
>  2 files changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/amd/pds_core/adminq.c b/drivers/net/ethernet/amd/pds_core/adminq.c
> index c83a0a80d533..387de1712827 100644
> --- a/drivers/net/ethernet/amd/pds_core/adminq.c
> +++ b/drivers/net/ethernet/amd/pds_core/adminq.c
> @@ -181,7 +181,10 @@ static int __pdsc_adminq_post(struct pdsc *pdsc,
>  	else
>  		avail -= q->head_idx + 1;
>  	if (!avail) {
> -		ret = -ENOSPC;
> +		if (!pdsc_is_fw_running(pdsc))
> +			ret = -ENXIO;
> +		else
> +			ret = -EAGAIN;

Short if will fit nice here, anyway:
Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>

>  		goto err_out_unlock;
>  	}
>  
> @@ -251,14 +254,25 @@ int pdsc_adminq_post(struct pdsc *pdsc,
>  	}
>  
>  	wc.qcq = &pdsc->adminqcq;
> -	index = __pdsc_adminq_post(pdsc, &pdsc->adminqcq, cmd, comp, &wc);
> +	time_start = jiffies;
> +	time_limit = time_start + HZ * pdsc->devcmd_timeout;
> +	do {
> +		index = __pdsc_adminq_post(pdsc, &pdsc->adminqcq, cmd, comp,
> +					   &wc);
> +		if (index != -EAGAIN)
> +			break;
> +
> +		dev_dbg(pdsc->dev, "Retrying adminq cmd opcode %u\n",
> +			cmd->opcode);
> +		/* Give completion processing a chance to free up space */
> +		msleep(1);
> +	} while (time_before(jiffies, time_limit));
> +
>  	if (index < 0) {
>  		err = index;
>  		goto err_out;
>  	}
>  
> -	time_start = jiffies;
> -	time_limit = time_start + HZ * pdsc->devcmd_timeout;
>  	do {
>  		/* Timeslice the actual wait to catch IO errors etc early */
>  		poll_jiffies = msecs_to_jiffies(poll_interval);
> diff --git a/include/linux/pds/pds_core_if.h b/include/linux/pds/pds_core_if.h
> index 17a87c1a55d7..babc6d573acd 100644
> --- a/include/linux/pds/pds_core_if.h
> +++ b/include/linux/pds/pds_core_if.h
> @@ -22,7 +22,7 @@
>  #define PDS_CORE_BAR0_INTR_CTRL_OFFSET		0x2000
>  #define PDS_CORE_DEV_CMD_DONE			0x00000001
>  
> -#define PDS_CORE_DEVCMD_TIMEOUT			5
> +#define PDS_CORE_DEVCMD_TIMEOUT			10
>  
>  #define PDS_CORE_CLIENT_ID			0
>  #define PDS_CORE_ASIC_TYPE_CAPRI		0
> -- 
> 2.17.1

Re: [PATCH net 2/2] pds_core: Add a retry mechanism when the adminq is full

[Jakub Kicinski]

On Tue, 28 Jan 2025 16:43:37 -0800 Shannon Nelson wrote:
> If the adminq is full, the driver reports failure when trying to post
> new adminq commands. This is a bit aggressive and unexpected because
> technically the adminq post didn't fail in this case, it was just full.
> To harden this path add support for a bounded retry mechanism.
> 
> It's possible some commands take longer than expected, maybe hundreds
> of milliseconds or seconds due to other processing on the device side,
> so to further reduce the chance of failure due to adminq full increase
> the PDS_CORE_DEVCMD_TIMEOUT from 5 to 10 seconds.
> 
> The caller of pdsc_adminq_post() may still see -EAGAIN reported if the
> space in the adminq never freed up. In this case they can choose to
> call the function again or fail. For now, no callers will retry.

How about a semaphore? You can initialize it to the number of slots
in the queue, and use down_timeout() if you want the 10 sec timeout?

Re: [PATCH net 2/2] pds_core: Add a retry mechanism when the adminq is full

[Brett Creeley]

On 1/29/2025 7:03 PM, Jakub Kicinski wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> On Tue, 28 Jan 2025 16:43:37 -0800 Shannon Nelson wrote:
>> If the adminq is full, the driver reports failure when trying to post
>> new adminq commands. This is a bit aggressive and unexpected because
>> technically the adminq post didn't fail in this case, it was just full.
>> To harden this path add support for a bounded retry mechanism.
>>
>> It's possible some commands take longer than expected, maybe hundreds
>> of milliseconds or seconds due to other processing on the device side,
>> so to further reduce the chance of failure due to adminq full increase
>> the PDS_CORE_DEVCMD_TIMEOUT from 5 to 10 seconds.
>>
>> The caller of pdsc_adminq_post() may still see -EAGAIN reported if the
>> space in the adminq never freed up. In this case they can choose to
>> call the function again or fail. For now, no callers will retry.
> 
> How about a semaphore? You can initialize it to the number of slots
> in the queue, and use down_timeout() if you want the 10 sec timeout?

After spending time digging into it a bit more, I think that this is 
probably the best long term solution.

It seems like we could refactor and replace the pds_core's adminq_refcnt 
that was originally introduced to resolve race conditions related to the 
adminq use/teardown between various client drivers (i.e. vfio/vdpa) with 
a semaphore(). This would solve both the race condition issues mentioned 
above and also the adminq overflow issue in this series.

However, I'm hoping you can accept this v1 solution as the fix for net 
because it does solve a problem and is a simple solution.
In the meantime we can commit to working on a refactor to use a 
semaphore/down_timeout() instead of the adminq_refcnt and this 
sleep/-EAGAIN mechanism for the long term solution that gets pushed to 
net-next. Ideally we do this right now, but it's a bit more of a 
refactor than we feel comfortable with for net.

Thanks,

Brett