| Message ID | 20180917072059.32657-1-mk.singh@oracle.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | bonding: avoid repeated display of same link status change | expand |
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 ? A bool is particularly dangerous here, at least on some arches. > goto re_arm; > } > > + bond->rtnl_needed = false; > bond_for_each_slave(bond, slave, iter) { > bond_commit_link_state(slave, BOND_SLAVE_NOTIFY_LATER); > } > diff --git a/include/net/bonding.h b/include/net/bonding.h > index 808f1d167349..50d61cf77855 100644 > --- a/include/net/bonding.h > +++ b/include/net/bonding.h > @@ -234,6 +234,7 @@ struct bonding { > struct dentry *debug_dir; > #endif /* CONFIG_DEBUG_FS */ > struct rtnl_link_stats64 bond_stats; > + bool rtnl_needed; > }; > > #define bond_slave_get_rcu(dev) \ >
> -----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. > > A bool is particularly dangerous here, at least on some arches. Thank you for cautioning us on bool usage. even a u8 can meet our requirement. we will change it. but; if time permits can you share more on "particularly dangerous here, at least on some arches". F > > > goto re_arm; > > } > > > > + bond->rtnl_needed = false; > > bond_for_each_slave(bond, slave, iter) { > > bond_commit_link_state(slave, > BOND_SLAVE_NOTIFY_LATER); > > } > > diff --git a/include/net/bonding.h b/include/net/bonding.h > > index 808f1d167349..50d61cf77855 100644 > > --- a/include/net/bonding.h > > +++ b/include/net/bonding.h > > @@ -234,6 +234,7 @@ struct bonding { > > struct dentry *debug_dir; > > #endif /* CONFIG_DEBUG_FS */ > > struct rtnl_link_stats64 bond_stats; > > + bool rtnl_needed; > > }; > > > > #define bond_slave_get_rcu(dev) \ > >
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.
> -----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 > > > > > > >
> -----Original Message----- > From: Manish Kumar Singh > Sent: 24 सितम्बर 2018 12:36 > To: 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 > > > > > -----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 Please review the updated patch, I have changed the rtnl_needed to an atomic variable, to avoid any race condition: 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..fac5350bf19c 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 && !atomic_read(&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 && !atomic_read(&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; + atomic_set(&bond->rtnl_needed, 1); goto re_arm; } + atomic_set(&bond->rtnl_needed, 0); bond_for_each_slave(bond, slave, iter) { bond_commit_link_state(slave, BOND_SLAVE_NOTIFY_LATER); } diff --git a/include/net/bonding.h b/include/net/bonding.h index 808f1d167349..ffc1219f7a07 100644 --- a/include/net/bonding.h +++ b/include/net/bonding.h @@ -234,6 +234,7 @@ struct bonding { struct dentry *debug_dir; #endif /* CONFIG_DEBUG_FS */ struct rtnl_link_stats64 bond_stats; + atomic_t rtnl_needed; }; #define bond_slave_get_rcu(dev) \ -- 2.14.1 Thanks, Manish > > > > > > > > > > > > > >
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; goto re_arm; } + bond->rtnl_needed = false; bond_for_each_slave(bond, slave, iter) { bond_commit_link_state(slave, BOND_SLAVE_NOTIFY_LATER); } diff --git a/include/net/bonding.h b/include/net/bonding.h index 808f1d167349..50d61cf77855 100644 --- a/include/net/bonding.h +++ b/include/net/bonding.h @@ -234,6 +234,7 @@ struct bonding { struct dentry *debug_dir; #endif /* CONFIG_DEBUG_FS */ struct rtnl_link_stats64 bond_stats; + bool rtnl_needed; }; #define bond_slave_get_rcu(dev) \