RE: [PATCH] bonding: avoid repeated display of same link status change

[Manish Kumar Singh <mk.singh@oracle.com>]

> -----Original Message-----
> From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
> Sent: 18 सितम्बर 2018 19:30
> To: Manish Kumar Singh; Eric Dumazet; netdev@vger.kernel.org
> Cc: Jay Vosburgh; Veaceslav Falico; Andy Gospodarek; David S. Miller; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH] bonding: avoid repeated display of same link status
> change
> 
> 
> 
> On 09/17/2018 10:05 PM, Manish Kumar Singh wrote:
> >
> >
> >> -----Original Message-----
> >> From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
> >> Sent: 17 सितम्बर 2018 20:08
> >> To: Manish Kumar Singh; netdev@vger.kernel.org
> >> Cc: Jay Vosburgh; Veaceslav Falico; Andy Gospodarek; David S. Miller;
> linux-
> >> kernel@vger.kernel.org
> >> Subject: Re: [PATCH] bonding: avoid repeated display of same link status
> >> change
> >>
> >>
> >>
> >> On 09/17/2018 12:20 AM, mk.singh@oracle.com wrote:
> >>> From: Manish Kumar Singh <mk.singh@oracle.com>
> >>>
> >>> When link status change needs to be committed and rtnl lock couldn't be
> >>> taken, avoid redisplay of same link status change message.
> >>>
> >>> Signed-off-by: Manish Kumar Singh <mk.singh@oracle.com>
> >>> ---
> >>>  drivers/net/bonding/bond_main.c | 6 ++++--
> >>>  include/net/bonding.h           | 1 +
> >>>  2 files changed, 5 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/net/bonding/bond_main.c
> >> b/drivers/net/bonding/bond_main.c
> >>> index 217b790d22ed..fb4e3aff1677 100644
> >>> --- a/drivers/net/bonding/bond_main.c
> >>> +++ b/drivers/net/bonding/bond_main.c
> >>> @@ -2087,7 +2087,7 @@ static int bond_miimon_inspect(struct bonding
> >> *bond)
> >>>  			bond_propose_link_state(slave, BOND_LINK_FAIL);
> >>>  			commit++;
> >>>  			slave->delay = bond->params.downdelay;
> >>> -			if (slave->delay) {
> >>> +			if (slave->delay && !bond->rtnl_needed) {
> >>>  				netdev_info(bond->dev, "link status down
> for
> >> %sinterface %s, disabling it in %d ms\n",
> >>>  					    (BOND_MODE(bond) ==
> >>>  					     BOND_MODE_ACTIVEBACKUP) ?
> >>> @@ -2127,7 +2127,7 @@ static int bond_miimon_inspect(struct bonding
> >> *bond)
> >>>  			commit++;
> >>>  			slave->delay = bond->params.updelay;
> >>>
> >>> -			if (slave->delay) {
> >>> +			if (slave->delay && !bond->rtnl_needed) {
> >>>  				netdev_info(bond->dev, "link status up for
> >> interface %s, enabling it in %d ms\n",
> >>>  					    slave->dev->name,
> >>>  					    ignore_updelay ? 0 :
> >>> @@ -2301,9 +2301,11 @@ static void bond_mii_monitor(struct
> >> work_struct *work)
> >>>  		if (!rtnl_trylock()) {
> >>>  			delay = 1;
> >>>  			should_notify_peers = false;
> >>> +			bond->rtnl_needed = true;
> >>
> >> How can you set a shared variable with no synchronization ?
> > Thanks Eric for reviewing the patch. rtnl_needed is not a shared variable, it
> is part of bonding structure, that is one per bonding driver instance. There
> can't be two parallel instances of bond_miimon_inspect for a single  bonding
> driver instance at any given point of time. and only bond_miimon_inspect
> updates it. That’s why I think there is no need of any synchronization here.
> >
> >
> 
> If rtnl_trylock() can not grab RTNL,
> there is no way the current thread can set the  variable without a race, if the
> word including rtnl_needed is shared by other fields in the structure.
> 
> Your patch adds a subtle possibility of future bugs, even if it runs fine today.
> 
> Do not pave the way for future bugs, make your code robust, please.

Thankyou Eric, we are making the changes and will repost the patch after testing it.
-Manish
> 
> 
> 
> 
> 
> 
>