Re: [PATCH net V3 3/3] bonding: change ipsec_lock from spin lock to mutex

[Hangbin Liu <liuhangbin@gmail.com>]

On Thu, Aug 08, 2024 at 10:05:26AM +0000, Jianbo Liu wrote:
> On Thu, 2024-08-08 at 17:34 +0800, Hangbin Liu wrote:
> > On Mon, Aug 05, 2024 at 08:03:57AM +0300, Tariq Toukan wrote:
> > >  drivers/net/bonding/bond_main.c | 75 +++++++++++++++++------------
> > > ----
> > >  include/net/bonding.h           |  2 +-
> > >  2 files changed, 40 insertions(+), 37 deletions(-)
> > > 
> > > diff --git a/drivers/net/bonding/bond_main.c
> > > b/drivers/net/bonding/bond_main.c
> > > index e550b1c08fdb..56764f1c39b8 100644
> > > --- a/drivers/net/bonding/bond_main.c
> > > +++ b/drivers/net/bonding/bond_main.c
> > > @@ -481,35 +476,43 @@ static void bond_ipsec_add_sa_all(struct
> > > bonding *bond)
> > >         struct bond_ipsec *ipsec;
> > >         struct slave *slave;
> > >  
> > > -       rcu_read_lock();
> > > -       slave = rcu_dereference(bond->curr_active_slave);
> > > -       if (!slave)
> > > -               goto out;
> > > +       slave = rtnl_dereference(bond->curr_active_slave);
> > > +       real_dev = slave ? slave->dev : NULL;
> > > +       if (!real_dev)
> > > +               return;
> > >  
> > > -       real_dev = slave->dev;
> > > +       mutex_lock(&bond->ipsec_lock);
> > >         if (!real_dev->xfrmdev_ops ||
> > >             !real_dev->xfrmdev_ops->xdo_dev_state_add ||
> > >             netif_is_bond_master(real_dev)) {
> > > -               spin_lock_bh(&bond->ipsec_lock);
> > >                 if (!list_empty(&bond->ipsec_list))
> > >                         slave_warn(bond_dev, real_dev,
> > >                                    "%s: no slave
> > > xdo_dev_state_add\n",
> > >                                    __func__);
> > > -               spin_unlock_bh(&bond->ipsec_lock);
> > >                 goto out;
> > >         }
> > >  
> > > -       spin_lock_bh(&bond->ipsec_lock);
> > >         list_for_each_entry(ipsec, &bond->ipsec_list, list) {
> > > +               struct net_device *dev = ipsec->xs->xso.real_dev;
> > > +
> > > +               /* If new state is added before ipsec_lock acquired
> > > */
> > > +               if (dev) {
> > > +                       if (dev == real_dev)
> > > +                               continue;
> > Hi Jianbo,
> > 
> > Why we skip the deleting here if dev == real_dev? What if the state
> 
> Here the bond active slave is updated. If dev == real_dev, the state
> (should be newly added) is offloaded to new active, so no need to
> delete and add back again.  
> 
> > is added again on the same slave? From the previous logic it looks we
> 
> Why is it added to the same slave? It's not the active one.

OK, I got what you mean now. Thanks for the explaination.

Reviewed-by: Hangbin Liu <liuhangbin@gmail.com>