From: Manish Kumar Singh <mk.singh@oracle.com>
To: Eric Dumazet <eric.dumazet@gmail.com>, netdev@vger.kernel.org
Cc: Jay Vosburgh <j.vosburgh@gmail.com>,
Veaceslav Falico <vfalico@gmail.com>,
Andy Gospodarek <andy@greyhouse.net>,
"David S. Miller" <davem@davemloft.net>,
linux-kernel@vger.kernel.org
Subject: RE: [PATCH] bonding: avoid repeated display of same link status change
Date: Mon, 22 Oct 2018 00:29:13 -0700 (PDT) [thread overview]
Message-ID: <0548f1e3-1cc6-4194-99ff-20fb00205574@default> (raw)
In-Reply-To: <dd819430-a67e-4507-b03f-d9d40528e74d@default>
> -----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
> >
> >
> >
> >
> >
> >
> >
next prev parent reply other threads:[~2018-10-22 7:29 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-09-17 7:20 [PATCH] bonding: avoid repeated display of same link status change mk.singh
2018-09-17 14:38 ` Eric Dumazet
2018-09-18 5:05 ` Manish Kumar Singh
2018-09-18 14:00 ` Eric Dumazet
2018-09-24 7:05 ` Manish Kumar Singh
2018-10-22 7:29 ` Manish Kumar Singh [this message]
2018-10-23 15:29 [PATCH] bonding:avoid " mk.singh
2018-10-23 15:54 ` Mahesh Bandewar (महेश बंडेवार)
2018-10-23 16:10 ` Eric Dumazet
2018-10-23 16:26 ` Michal Kubecek
2018-10-23 16:38 ` Michal Kubecek
2018-10-25 9:21 ` Manish Kumar Singh
2018-10-25 9:29 ` Michal Kubecek
2018-10-26 6:49 ` Manish Kumar Singh
2018-10-23 18:08 ` David Miller
2018-10-31 10:57 mk.singh
2018-11-03 6:31 ` David Miller
2018-11-04 19:41 ` Michal Kubecek
2018-11-20 10:41 ` Manish Kumar Singh
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=0548f1e3-1cc6-4194-99ff-20fb00205574@default \
--to=mk.singh@oracle.com \
--cc=andy@greyhouse.net \
--cc=davem@davemloft.net \
--cc=eric.dumazet@gmail.com \
--cc=j.vosburgh@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=vfalico@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).