netdev - Re: [net-next 03/16] net/mlx5e: Encapsulate updating netdev queues into a function

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] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 26 Feb 2020 07:38:05 +0000
From:   Saeed Mahameed <saeedm@...lanox.com>
To:     "kuba@...nel.org" <kuba@...nel.org>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Tariq Toukan <tariqt@...lanox.com>,
        Maxim Mikityanskiy <maximmi@...lanox.com>
Subject: Re: [net-next 03/16] net/mlx5e: Encapsulate updating netdev queues
 into a function

On Tue, 2020-02-25 at 17:41 -0800, Jakub Kicinski wrote:
> On Tue, 25 Feb 2020 17:12:33 -0800 Saeed Mahameed wrote:
> > From: Maxim Mikityanskiy <maximmi@...lanox.com>
> > 
> > As a preparation for one of the following commits, create a
> > function to
> > encapsulate the code that notifies the kernel about the new amount
> > of
> > RX and TX queues. The code will be called multiple times in the
> > next
> > commit.
> > 
> > Signed-off-by: Maxim Mikityanskiy <maximmi@...lanox.com>
> > Reviewed-by: Tariq Toukan <tariqt@...lanox.com>
> > Signed-off-by: Saeed Mahameed <saeedm@...lanox.com>
> > ---
> >  .../net/ethernet/mellanox/mlx5/core/en_main.c | 19 ++++++++++++---
> > ----
> >  1 file changed, 12 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> > b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> > index a4d3e1b6ab20..85a86ff72aac 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> > @@ -2869,6 +2869,17 @@ static void mlx5e_netdev_set_tcs(struct
> > net_device *netdev)
> >  		netdev_set_tc_queue(netdev, tc, nch, 0);
> >  }
> >  
> > +static void mlx5e_update_netdev_queues(struct mlx5e_priv *priv)
> > +{
> > +	int num_txqs = priv->channels.num * priv-
> > >channels.params.num_tc;
> > +	int num_rxqs = priv->channels.num * priv->profile->rq_groups;
> > +	struct net_device *netdev = priv->netdev;
> > +
> > +	mlx5e_netdev_set_tcs(netdev);
> > +	netif_set_real_num_tx_queues(netdev, num_txqs);
> > +	netif_set_real_num_rx_queues(netdev, num_rxqs);
> > +}
> > +
> >  static void mlx5e_build_txq_maps(struct mlx5e_priv *priv)
> >  {
> >  	int i, ch;
> > @@ -2890,13 +2901,7 @@ static void mlx5e_build_txq_maps(struct
> > mlx5e_priv *priv)
> >  
> >  void mlx5e_activate_priv_channels(struct mlx5e_priv *priv)
> >  {
> > -	int num_txqs = priv->channels.num * priv-
> > >channels.params.num_tc;
> > -	int num_rxqs = priv->channels.num * priv->profile->rq_groups;
> > -	struct net_device *netdev = priv->netdev;
> > -
> > -	mlx5e_netdev_set_tcs(netdev);
> > -	netif_set_real_num_tx_queues(netdev, num_txqs);
> > -	netif_set_real_num_rx_queues(netdev, num_rxqs);
> > +	mlx5e_update_netdev_queues(priv);
> 
> Not sure where we stand on just moving bad code, but set_real_num_
> _queues can fail, Dave just pointed this out to someone recently in
> review.
> 

good point, but likewise not sure if this patch is the place to fix the
issue, the good thing is that now it is fixable since maxim added the
ability to allow failing mlx5_switch_channels() and roll back.

in patch 6 of this series;
net/mlx5e: Fix configuration of XPS cpumasks and netdev queues in
corner cases

you can see that Maxim is using the new function introduced in this
patch as a hook that is allowed to fail and bail out when switching
between channels:

err = mlx5e_safe_switch_channels(priv, &new_channels,
mlx5e_num_channels_changed);

so all we need is to not ignore the return value of
netif_set_real_num_tx_queues() and abort mlx5e_num_channels_changed()
in case of failure which will abort mlx5e_safe_switch_channels() and
old channels will keep working.

I guess this should be an incremental oneliner patch after this series.

Thanks,
Saeed.

> >  
> >  	mlx5e_build_txq_maps(priv);
> >  	mlx5e_activate_channels(&priv->channels);