[PATCH net-next] net: mana: Improve mana_set_channels() for low mem conditions

[PATCH net-next] net: mana: Improve mana_set_channels() for low mem conditions

[Shradha Gupta]

The mana_set_channels() function requires detaching the mana
driver and reattaching it with changed channel values.
During this operation if the system is low on memory, the reattach
might fail, causing the network device being down.
To avoid this we pre-allocate buffers at the beginning of set operation,
to prevent complete network loss

Signed-off-by: Shradha Gupta <shradhagupta@linux.microsoft.com>
---
 .../ethernet/microsoft/mana/mana_ethtool.c    | 28 +++++++++++--------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
index d6a35fbda447..5077493fdfde 100644
--- a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
+++ b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
@@ -345,27 +345,31 @@ static int mana_set_channels(struct net_device *ndev,
 	struct mana_port_context *apc = netdev_priv(ndev);
 	unsigned int new_count = channels->combined_count;
 	unsigned int old_count = apc->num_queues;
-	int err, err2;
+	int err;
+
+	apc->num_queues = new_count;
+	err = mana_pre_alloc_rxbufs(apc, ndev->mtu);
+	apc->num_queues = old_count;
+	if (err) {
+		netdev_err(ndev, "Insufficient memory for new allocations");
+		return err;
+	}
 
 	err = mana_detach(ndev, false);
 	if (err) {
 		netdev_err(ndev, "mana_detach failed: %d\n", err);
-		return err;
+		goto out;
 	}
 
 	apc->num_queues = new_count;
 	err = mana_attach(ndev);
-	if (!err)
-		return 0;
-
-	netdev_err(ndev, "mana_attach failed: %d\n", err);
-
-	/* Try to roll it back to the old configuration. */
-	apc->num_queues = old_count;
-	err2 = mana_attach(ndev);
-	if (err2)
-		netdev_err(ndev, "mana re-attach failed: %d\n", err2);
+	if (err) {
+		apc->num_queues = old_count;
+		netdev_err(ndev, "mana_attach failed: %d\n", err);
+	}
 
+out:
+	mana_pre_dealloc_rxbufs(apc);
 	return err;
 }
 
-- 
2.34.1

Re: [PATCH net-next] net: mana: Improve mana_set_channels() for low mem conditions

[Gerhard Engleder]

On 29.08.24 16:16, Shradha Gupta wrote:
> The mana_set_channels() function requires detaching the mana
> driver and reattaching it with changed channel values.
> During this operation if the system is low on memory, the reattach
> might fail, causing the network device being down.
> To avoid this we pre-allocate buffers at the beginning of set operation,
> to prevent complete network loss
> 
> Signed-off-by: Shradha Gupta <shradhagupta@linux.microsoft.com>
> ---
>   .../ethernet/microsoft/mana/mana_ethtool.c    | 28 +++++++++++--------
>   1 file changed, 16 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> index d6a35fbda447..5077493fdfde 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> @@ -345,27 +345,31 @@ static int mana_set_channels(struct net_device *ndev,
>   	struct mana_port_context *apc = netdev_priv(ndev);
>   	unsigned int new_count = channels->combined_count;
>   	unsigned int old_count = apc->num_queues;
> -	int err, err2;
> +	int err;
> +
> +	apc->num_queues = new_count;
> +	err = mana_pre_alloc_rxbufs(apc, ndev->mtu);
> +	apc->num_queues = old_count;

Are you sure that temporary changing num_queues has no side effects on
other num_queues users like mana_chn_setxdp()?

Gerhard

RE: [PATCH net-next] net: mana: Improve mana_set_channels() for low mem conditions

[Haiyang Zhang]

> -----Original Message-----
> From: Gerhard Engleder <gerhard@engleder-embedded.com>
> Sent: Thursday, August 29, 2024 3:54 PM
> To: Shradha Gupta <shradhagupta@linux.microsoft.com>; linux-
> hyperv@vger.kernel.org; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-rdma@vger.kernel.org
> Cc: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> <haiyangz@microsoft.com>; Wei Liu <wei.liu@kernel.org>; Dexuan Cui
> <decui@microsoft.com>; David S. Miller <davem@davemloft.net>; Eric
> Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo
> Abeni <pabeni@redhat.com>; Long Li <longli@microsoft.com>; Simon Horman
> <horms@kernel.org>; Konstantin Taranov <kotaranov@microsoft.com>;
> Souradeep Chakrabarti <schakrabarti@linux.microsoft.com>; Erick Archer
> <erick.archer@outlook.com>; Pavan Chebbi <pavan.chebbi@broadcom.com>;
> Ahmed Zaki <ahmed.zaki@intel.com>; Colin Ian King
> <colin.i.king@gmail.com>; Shradha Gupta <shradhagupta@microsoft.com>
> Subject: Re: [PATCH net-next] net: mana: Improve mana_set_channels() for
> low mem conditions
> 
> [Some people who received this message don't often get email from
> gerhard@engleder-embedded.com. Learn why this is important at
> https://aka.ms/LearnAboutSenderIdentification ]
> 
> On 29.08.24 16:16, Shradha Gupta wrote:
> > The mana_set_channels() function requires detaching the mana
> > driver and reattaching it with changed channel values.
> > During this operation if the system is low on memory, the reattach
> > might fail, causing the network device being down.
> > To avoid this we pre-allocate buffers at the beginning of set
> operation,
> > to prevent complete network loss
> >
> > Signed-off-by: Shradha Gupta <shradhagupta@linux.microsoft.com>
> > ---
> >   .../ethernet/microsoft/mana/mana_ethtool.c    | 28 +++++++++++-------
> -
> >   1 file changed, 16 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> > index d6a35fbda447..5077493fdfde 100644
> > --- a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> > +++ b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> > @@ -345,27 +345,31 @@ static int mana_set_channels(struct net_device
> *ndev,
> >       struct mana_port_context *apc = netdev_priv(ndev);
> >       unsigned int new_count = channels->combined_count;
> >       unsigned int old_count = apc->num_queues;
> > -     int err, err2;
> > +     int err;
> > +
> > +     apc->num_queues = new_count;
> > +     err = mana_pre_alloc_rxbufs(apc, ndev->mtu);
> > +     apc->num_queues = old_count;
> 
> Are you sure that temporary changing num_queues has no side effects on
> other num_queues users like mana_chn_setxdp()?
> 

mana_chn_setxdp() is protected by rtnl_lock, which is OK. But I'm not sure
if all other users are protected. mana_get_stats64() seems not.

@Shradha Gupta You can add num_queues as an argument of mana_pre_alloc_rxbufs()
to avoid changing apc->num_queues.

Thanks,
- Haiyang

Re: [PATCH net-next] net: mana: Improve mana_set_channels() for low mem conditions

[Shradha Gupta]

On Thu, Aug 29, 2024 at 09:00:05PM +0000, Haiyang Zhang wrote:
> 
> 
> > -----Original Message-----
> > From: Gerhard Engleder <gerhard@engleder-embedded.com>
> > Sent: Thursday, August 29, 2024 3:54 PM
> > To: Shradha Gupta <shradhagupta@linux.microsoft.com>; linux-
> > hyperv@vger.kernel.org; netdev@vger.kernel.org; linux-
> > kernel@vger.kernel.org; linux-rdma@vger.kernel.org
> > Cc: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> > <haiyangz@microsoft.com>; Wei Liu <wei.liu@kernel.org>; Dexuan Cui
> > <decui@microsoft.com>; David S. Miller <davem@davemloft.net>; Eric
> > Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo
> > Abeni <pabeni@redhat.com>; Long Li <longli@microsoft.com>; Simon Horman
> > <horms@kernel.org>; Konstantin Taranov <kotaranov@microsoft.com>;
> > Souradeep Chakrabarti <schakrabarti@linux.microsoft.com>; Erick Archer
> > <erick.archer@outlook.com>; Pavan Chebbi <pavan.chebbi@broadcom.com>;
> > Ahmed Zaki <ahmed.zaki@intel.com>; Colin Ian King
> > <colin.i.king@gmail.com>; Shradha Gupta <shradhagupta@microsoft.com>
> > Subject: Re: [PATCH net-next] net: mana: Improve mana_set_channels() for
> > low mem conditions
> > 
> > [Some people who received this message don't often get email from
> > gerhard@engleder-embedded.com. Learn why this is important at
> > https://aka.ms/LearnAboutSenderIdentification ]
> > 
> > On 29.08.24 16:16, Shradha Gupta wrote:
> > > The mana_set_channels() function requires detaching the mana
> > > driver and reattaching it with changed channel values.
> > > During this operation if the system is low on memory, the reattach
> > > might fail, causing the network device being down.
> > > To avoid this we pre-allocate buffers at the beginning of set
> > operation,
> > > to prevent complete network loss
> > >
> > > Signed-off-by: Shradha Gupta <shradhagupta@linux.microsoft.com>
> > > ---
> > >   .../ethernet/microsoft/mana/mana_ethtool.c    | 28 +++++++++++-------
> > -
> > >   1 file changed, 16 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> > b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> > > index d6a35fbda447..5077493fdfde 100644
> > > --- a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> > > +++ b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> > > @@ -345,27 +345,31 @@ static int mana_set_channels(struct net_device
> > *ndev,
> > >       struct mana_port_context *apc = netdev_priv(ndev);
> > >       unsigned int new_count = channels->combined_count;
> > >       unsigned int old_count = apc->num_queues;
> > > -     int err, err2;
> > > +     int err;
> > > +
> > > +     apc->num_queues = new_count;
> > > +     err = mana_pre_alloc_rxbufs(apc, ndev->mtu);
> > > +     apc->num_queues = old_count;
> > 
> > Are you sure that temporary changing num_queues has no side effects on
> > other num_queues users like mana_chn_setxdp()?
> > 
> 
> mana_chn_setxdp() is protected by rtnl_lock, which is OK. But I'm not sure
> if all other users are protected. mana_get_stats64() seems not.
> 
> @Shradha Gupta You can add num_queues as an argument of mana_pre_alloc_rxbufs()
> to avoid changing apc->num_queues.
> 
> Thanks,
> - Haiyang

Thanks Haiyang and Gerhard. Instead of changing the apc structure value,
I will pass it to the mana_pre_alloc_rxbufs() function in the next
version. That should make sure other calls are unaffected.

Thanks,
Shradha.