[PATCH net-next] sfc: handle VI shortage on ef100 by readjusting the channels

[PATCH net-next] sfc: handle VI shortage on ef100 by readjusting the channels

[Pieter Jansen van Vuuren]

When fewer VIs are allocated than what is allowed we can readjust
the channels by calling efx_mcdi_alloc_vis() again.

Signed-off-by: Pieter Jansen van Vuuren <pieter.jansen-van-vuuren@amd.com>
Reviewed-by: Martin Habets <habetsm.xilinx@gmail.com>
---
 drivers/net/ethernet/sfc/ef100_netdev.c | 51 ++++++++++++++++++++++---
 1 file changed, 45 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/sfc/ef100_netdev.c b/drivers/net/ethernet/sfc/ef100_netdev.c
index d916877b5a9a..c201e001f3b8 100644
--- a/drivers/net/ethernet/sfc/ef100_netdev.c
+++ b/drivers/net/ethernet/sfc/ef100_netdev.c
@@ -40,19 +40,26 @@ static int ef100_alloc_vis(struct efx_nic *efx, unsigned int *allocated_vis)
 	unsigned int tx_vis = efx->n_tx_channels + efx->n_extra_tx_channels;
 	unsigned int rx_vis = efx->n_rx_channels;
 	unsigned int min_vis, max_vis;
+	int rc;
 
 	EFX_WARN_ON_PARANOID(efx->tx_queues_per_channel != 1);
 
 	tx_vis += efx->n_xdp_channels * efx->xdp_tx_per_channel;
 
 	max_vis = max(rx_vis, tx_vis);
-	/* Currently don't handle resource starvation and only accept
-	 * our maximum needs and no less.
+	/* We require at least a single complete TX channel worth of queues. */
+	min_vis = efx->tx_queues_per_channel;
+
+	rc = efx_mcdi_alloc_vis(efx, min_vis, max_vis,
+				NULL, allocated_vis);
+
+	/* We retry allocating VIs by reallocating channels when we have not
+	 * been able to allocate the maximum VIs.
 	 */
-	min_vis = max_vis;
+	if (!rc && *allocated_vis < max_vis)
+		rc = -EAGAIN;
 
-	return efx_mcdi_alloc_vis(efx, min_vis, max_vis,
-				  NULL, allocated_vis);
+	return rc;
 }
 
 static int ef100_remap_bar(struct efx_nic *efx, int max_vis)
@@ -133,9 +140,41 @@ static int ef100_net_open(struct net_device *net_dev)
 		goto fail;
 
 	rc = ef100_alloc_vis(efx, &allocated_vis);
-	if (rc)
+	if (rc && rc != -EAGAIN)
 		goto fail;
 
+	/* Try one more time but with the maximum number of channels
+	 * equal to the allocated VIs, which would more likely succeed.
+	 */
+	if (rc == -EAGAIN) {
+		rc = efx_mcdi_free_vis(efx);
+		if (rc)
+			goto fail;
+
+		efx_remove_interrupts(efx);
+		efx->max_channels = allocated_vis;
+
+		rc = efx_probe_interrupts(efx);
+		if (rc)
+			goto fail;
+
+		rc = efx_set_channels(efx);
+		if (rc)
+			goto fail;
+
+		rc = ef100_alloc_vis(efx, &allocated_vis);
+		if (rc && rc != -EAGAIN)
+			goto fail;
+
+		/* It should be very unlikely that we failed here again, but in
+		 * such a case we return ENOSPC.
+		 */
+		if (rc == -EAGAIN) {
+			rc = -ENOSPC;
+			goto fail;
+		}
+	}
+
 	rc = efx_probe_channels(efx);
 	if (rc)
 		return rc;
-- 
2.25.1

Re: [PATCH net-next] sfc: handle VI shortage on ef100 by readjusting the channels

[Simon Horman]

On Wed, May 24, 2023 at 10:36:38AM +0100, Pieter Jansen van Vuuren wrote:
> When fewer VIs are allocated than what is allowed we can readjust
> the channels by calling efx_mcdi_alloc_vis() again.
> 
> Signed-off-by: Pieter Jansen van Vuuren <pieter.jansen-van-vuuren@amd.com>
> Reviewed-by: Martin Habets <habetsm.xilinx@gmail.com>

Hi Pieter,

this patch looks good to me.

Reviewed-by: Simon Horman <simon.horman@corigine.com>

But during the review I noticed that Smatch flags some
problems in ef100_netdev.c that you may wish to address.
Please see below.

> ---
>  drivers/net/ethernet/sfc/ef100_netdev.c | 51 ++++++++++++++++++++++---
>  1 file changed, 45 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/sfc/ef100_netdev.c b/drivers/net/ethernet/sfc/ef100_netdev.c
> index d916877b5a9a..c201e001f3b8 100644
> --- a/drivers/net/ethernet/sfc/ef100_netdev.c
> +++ b/drivers/net/ethernet/sfc/ef100_netdev.c

...

> @@ -133,9 +140,41 @@ static int ef100_net_open(struct net_device *net_dev)
>  		goto fail;
>  
>  	rc = ef100_alloc_vis(efx, &allocated_vis);
> -	if (rc)
> +	if (rc && rc != -EAGAIN)
>  		goto fail;
>  
> +	/* Try one more time but with the maximum number of channels
> +	 * equal to the allocated VIs, which would more likely succeed.
> +	 */
> +	if (rc == -EAGAIN) {
> +		rc = efx_mcdi_free_vis(efx);
> +		if (rc)
> +			goto fail;
> +
> +		efx_remove_interrupts(efx);
> +		efx->max_channels = allocated_vis;
> +
> +		rc = efx_probe_interrupts(efx);
> +		if (rc)
> +			goto fail;
> +
> +		rc = efx_set_channels(efx);
> +		if (rc)
> +			goto fail;
> +
> +		rc = ef100_alloc_vis(efx, &allocated_vis);
> +		if (rc && rc != -EAGAIN)
> +			goto fail;
> +
> +		/* It should be very unlikely that we failed here again, but in
> +		 * such a case we return ENOSPC.
> +		 */
> +		if (rc == -EAGAIN) {
> +			rc = -ENOSPC;
> +			goto fail;
> +		}
> +	}
> +
>  	rc = efx_probe_channels(efx);
>  	if (rc)
>  		return rc;

Not strictly related to this patch, but Smatch says that on error this should
probably free some resources. So perhaps:

		goto fail;

Also not strictly related this patch, but Smatch also noticed that
in ef100_probe_netdev net_dev does not seem to be freed on the error path.

Re: [PATCH net-next] sfc: handle VI shortage on ef100 by readjusting the channels

[Pieter Jansen van Vuuren]

On 24/05/2023 11:09, Simon Horman wrote:
> On Wed, May 24, 2023 at 10:36:38AM +0100, Pieter Jansen van Vuuren wrote:
>> When fewer VIs are allocated than what is allowed we can readjust
>> the channels by calling efx_mcdi_alloc_vis() again.
>>
>> Signed-off-by: Pieter Jansen van Vuuren <pieter.jansen-van-vuuren@amd.com>
>> Reviewed-by: Martin Habets <habetsm.xilinx@gmail.com>
> 
> Hi Pieter,
> 
> this patch looks good to me.
> 
> Reviewed-by: Simon Horman <simon.horman@corigine.com>
> 
> But during the review I noticed that Smatch flags some
> problems in ef100_netdev.c that you may wish to address.
> Please see below.
> 
>> ---
>>  drivers/net/ethernet/sfc/ef100_netdev.c | 51 ++++++++++++++++++++++---
>>  1 file changed, 45 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/sfc/ef100_netdev.c b/drivers/net/ethernet/sfc/ef100_netdev.c
>> index d916877b5a9a..c201e001f3b8 100644
>> --- a/drivers/net/ethernet/sfc/ef100_netdev.c
>> +++ b/drivers/net/ethernet/sfc/ef100_netdev.c
> 
...
>> +		/* It should be very unlikely that we failed here again, but in
>> +		 * such a case we return ENOSPC.
>> +		 */
>> +		if (rc == -EAGAIN) {
>> +			rc = -ENOSPC;
>> +			goto fail;
>> +		}
>> +	}
>> +
>>  	rc = efx_probe_channels(efx);
>>  	if (rc)
>>  		return rc;
> 
> Not strictly related to this patch, but Smatch says that on error this should
> probably free some resources. So perhaps:
> 
> 		goto fail;
> 
> Also not strictly related this patch, but Smatch also noticed that
> in ef100_probe_netdev net_dev does not seem to be freed on the error path.

Thank you for the review Simon. Yes, I think this requires some attention, I think
this is one of a few that we need to look at. So it will likely become a separate
patch set addressing Smatch issues.

Re: [PATCH net-next] sfc: handle VI shortage on ef100 by readjusting the channels

[Simon Horman]

On Wed, May 24, 2023 at 02:52:48PM +0100, Pieter Jansen van Vuuren wrote:
> 
> 
> On 24/05/2023 11:09, Simon Horman wrote:
> > On Wed, May 24, 2023 at 10:36:38AM +0100, Pieter Jansen van Vuuren wrote:
> >> When fewer VIs are allocated than what is allowed we can readjust
> >> the channels by calling efx_mcdi_alloc_vis() again.
> >>
> >> Signed-off-by: Pieter Jansen van Vuuren <pieter.jansen-van-vuuren@amd.com>
> >> Reviewed-by: Martin Habets <habetsm.xilinx@gmail.com>
> > 
> > Hi Pieter,
> > 
> > this patch looks good to me.
> > 
> > Reviewed-by: Simon Horman <simon.horman@corigine.com>
> > 
> > But during the review I noticed that Smatch flags some
> > problems in ef100_netdev.c that you may wish to address.
> > Please see below.
> > 
> >> ---
> >>  drivers/net/ethernet/sfc/ef100_netdev.c | 51 ++++++++++++++++++++++---
> >>  1 file changed, 45 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/net/ethernet/sfc/ef100_netdev.c b/drivers/net/ethernet/sfc/ef100_netdev.c
> >> index d916877b5a9a..c201e001f3b8 100644
> >> --- a/drivers/net/ethernet/sfc/ef100_netdev.c
> >> +++ b/drivers/net/ethernet/sfc/ef100_netdev.c
> > 
> ...
> >> +		/* It should be very unlikely that we failed here again, but in
> >> +		 * such a case we return ENOSPC.
> >> +		 */
> >> +		if (rc == -EAGAIN) {
> >> +			rc = -ENOSPC;
> >> +			goto fail;
> >> +		}
> >> +	}
> >> +
> >>  	rc = efx_probe_channels(efx);
> >>  	if (rc)
> >>  		return rc;
> > 
> > Not strictly related to this patch, but Smatch says that on error this should
> > probably free some resources. So perhaps:
> > 
> > 		goto fail;
> > 
> > Also not strictly related this patch, but Smatch also noticed that
> > in ef100_probe_netdev net_dev does not seem to be freed on the error path.
> 
> Thank you for the review Simon. Yes, I think this requires some attention, I think
> this is one of a few that we need to look at. So it will likely become a separate
> patch set addressing Smatch issues.

Thanks Pieter,

I agree that these are separate issues and can
be handled outside the scope of this patch.

Re: [PATCH net-next] sfc: handle VI shortage on ef100 by readjusting the channels

[Edward Cree]

On 24/05/2023 10:36, Pieter Jansen van Vuuren wrote:
> When fewer VIs are allocated than what is allowed we can readjust
> the channels by calling efx_mcdi_alloc_vis() again.
> 
> Signed-off-by: Pieter Jansen van Vuuren <pieter.jansen-van-vuuren@amd.com>
> Reviewed-by: Martin Habets <habetsm.xilinx@gmail.com>

Reviewed-by: Edward Cree <ecree.xilinx@gmail.com>
though see below for one nit (fix in a follow-up?)

> ---
>  drivers/net/ethernet/sfc/ef100_netdev.c | 51 ++++++++++++++++++++++---
>  1 file changed, 45 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/sfc/ef100_netdev.c b/drivers/net/ethernet/sfc/ef100_netdev.c
> index d916877b5a9a..c201e001f3b8 100644
> --- a/drivers/net/ethernet/sfc/ef100_netdev.c
> +++ b/drivers/net/ethernet/sfc/ef100_netdev.c
> @@ -40,19 +40,26 @@ static int ef100_alloc_vis(struct efx_nic *efx, unsigned int *allocated_vis)
>  	unsigned int tx_vis = efx->n_tx_channels + efx->n_extra_tx_channels;
>  	unsigned int rx_vis = efx->n_rx_channels;
>  	unsigned int min_vis, max_vis;
> +	int rc;
>  
>  	EFX_WARN_ON_PARANOID(efx->tx_queues_per_channel != 1);
>  
>  	tx_vis += efx->n_xdp_channels * efx->xdp_tx_per_channel;
>  
>  	max_vis = max(rx_vis, tx_vis);
> -	/* Currently don't handle resource starvation and only accept
> -	 * our maximum needs and no less.
> +	/* We require at least a single complete TX channel worth of queues. */
> +	min_vis = efx->tx_queues_per_channel;
> +
> +	rc = efx_mcdi_alloc_vis(efx, min_vis, max_vis,
> +				NULL, allocated_vis);

I'd like a check here like
    if (rc == -EAGAIN)
            rc = -ESOMETHINGELSE;
 just to avoid confusion if the MC or MCDI machinery returns EAGAIN
 for whatever reason.
Or perhaps better still, don't overload EAGAIN like this and instead
 have this function return 1 in the "we succeeded but didn't get
 max_vis" case (would need a comment above the function, documenting
 this), since that's not a value that can happen in-band.

-ed

Re: [PATCH net-next] sfc: handle VI shortage on ef100 by readjusting the channels

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by David S. Miller <davem@davemloft.net>:

On Wed, 24 May 2023 10:36:38 +0100 you wrote:
> When fewer VIs are allocated than what is allowed we can readjust
> the channels by calling efx_mcdi_alloc_vis() again.
> 
> Signed-off-by: Pieter Jansen van Vuuren <pieter.jansen-van-vuuren@amd.com>
> Reviewed-by: Martin Habets <habetsm.xilinx@gmail.com>
> ---
>  drivers/net/ethernet/sfc/ef100_netdev.c | 51 ++++++++++++++++++++++---
>  1 file changed, 45 insertions(+), 6 deletions(-)

Here is the summary with links:
  - [net-next] sfc: handle VI shortage on ef100 by readjusting the channels
    https://git.kernel.org/netdev/net-next/c/ca7d05007d0a

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

Re: [PATCH net-next] sfc: handle VI shortage on ef100 by readjusting the channels

[Pieter Jansen van Vuuren]

On 25/05/2023 15:51, Edward Cree wrote:
> On 24/05/2023 10:36, Pieter Jansen van Vuuren wrote:
>> When fewer VIs are allocated than what is allowed we can readjust
>> the channels by calling efx_mcdi_alloc_vis() again.
>>
>> Signed-off-by: Pieter Jansen van Vuuren <pieter.jansen-van-vuuren@amd.com>
>> Reviewed-by: Martin Habets <habetsm.xilinx@gmail.com>
> 
> Reviewed-by: Edward Cree <ecree.xilinx@gmail.com>
> though see below for one nit (fix in a follow-up?)
> 
>> ---
>>  drivers/net/ethernet/sfc/ef100_netdev.c | 51 ++++++++++++++++++++++---
>>  1 file changed, 45 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/sfc/ef100_netdev.c b/drivers/net/ethernet/sfc/ef100_netdev.c
>> index d916877b5a9a..c201e001f3b8 100644
>> --- a/drivers/net/ethernet/sfc/ef100_netdev.c
>> +++ b/drivers/net/ethernet/sfc/ef100_netdev.c
>> @@ -40,19 +40,26 @@ static int ef100_alloc_vis(struct efx_nic *efx, unsigned int *allocated_vis)
>>  	unsigned int tx_vis = efx->n_tx_channels + efx->n_extra_tx_channels;
>>  	unsigned int rx_vis = efx->n_rx_channels;
>>  	unsigned int min_vis, max_vis;
>> +	int rc;
>>  
>>  	EFX_WARN_ON_PARANOID(efx->tx_queues_per_channel != 1);
>>  
>>  	tx_vis += efx->n_xdp_channels * efx->xdp_tx_per_channel;
>>  
>>  	max_vis = max(rx_vis, tx_vis);
>> -	/* Currently don't handle resource starvation and only accept
>> -	 * our maximum needs and no less.
>> +	/* We require at least a single complete TX channel worth of queues. */
>> +	min_vis = efx->tx_queues_per_channel;
>> +
>> +	rc = efx_mcdi_alloc_vis(efx, min_vis, max_vis,
>> +				NULL, allocated_vis);
> 
> I'd like a check here like
>     if (rc == -EAGAIN)
>             rc = -ESOMETHINGELSE;
>  just to avoid confusion if the MC or MCDI machinery returns EAGAIN
>  for whatever reason.
> Or perhaps better still, don't overload EAGAIN like this and instead
>  have this function return 1 in the "we succeeded but didn't get
>  max_vis" case (would need a comment above the function, documenting
>  this), since that's not a value that can happen in-band.
> 
> -ed

Thank you Ed. Yes, I will work with you on the follow-up and then avoid
overloading EAGAIN.