| Message ID | 20230524093638.8676-1-pieter.jansen-van-vuuren@amd.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | ca7d05007d0a95615a51cb5a624775db8c450f43 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] sfc: handle VI shortage on ef100 by readjusting the channels | expand |
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.
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.
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.
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
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!
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.
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;