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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <058f2e61-159f-536c-7a1b-eb58f742a66d@amd.com>
Date: Tue, 30 May 2023 11:21:35 +0100
From: Pieter Jansen van Vuuren <pieter.jansen-van-vuuren@....com>
To: Edward Cree <ecree.xilinx@...il.com>, netdev@...r.kernel.org,
 linux-net-drivers@....com
Cc: davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com,
 edumazet@...gle.com, habetsm.xilinx@...il.com
Subject: Re: [PATCH net-next] sfc: handle VI shortage on ef100 by readjusting
 the channels



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@....com>
>> Reviewed-by: Martin Habets <habetsm.xilinx@...il.com>
> 
> Reviewed-by: Edward Cree <ecree.xilinx@...il.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.