lkml.org 
[lkml]   [2018]   [Sep]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRE: [PATCH] bonding: avoid repeated display of same link status change


> -----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
>
>
>
>
>
>
>

\
 
 \ /
  Last update: 2018-09-24 09:06    [W:0.058 / U:0.284 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site