From: Manish Kumar Singh <[email protected]>
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 <[email protected]>
---
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 2b01180be834..af9ef889a429 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2096,7 +2096,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) ?
@@ -2136,7 +2136,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 :
@@ -2310,9 +2310,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 a4f116f06c50..a4353506bb4f 100644
--- a/include/net/bonding.h
+++ b/include/net/bonding.h
@@ -229,6 +229,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
On Tue, Oct 23, 2018 at 8:29 AM, <[email protected]> wrote:
> From: Manish Kumar Singh <[email protected]>
>
> 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 <[email protected]>
> ---
> 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 2b01180be834..af9ef889a429 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -2096,7 +2096,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)) {
Atomic operations are expensive (on certain architectures) and miimon
runs quite frequently. Is the added cost of these atomic operations
even worth just to avoid *duplicate info* messages? This seems like a
overkill!
> netdev_info(bond->dev, "link status down for %sinterface %s, disabling it in %d ms\n",
> (BOND_MODE(bond) ==
> BOND_MODE_ACTIVEBACKUP) ?
> @@ -2136,7 +2136,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 :
> @@ -2310,9 +2310,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 a4f116f06c50..a4353506bb4f 100644
> --- a/include/net/bonding.h
> +++ b/include/net/bonding.h
> @@ -229,6 +229,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
>
On 10/23/2018 08:54 AM, Mahesh Bandewar (महेश बंडेवार) wrote:
> Atomic operations are expensive (on certain architectures) and miimon
> runs quite frequently. Is the added cost of these atomic operations
> even worth just to avoid *duplicate info* messages? This seems like a
> overkill!
atomic_read() is a simple read, no atomic operation involved.
Same remark for atomic_set()
On Tue, Oct 23, 2018 at 09:10:44AM -0700, Eric Dumazet wrote:
>
>
> On 10/23/2018 08:54 AM, Mahesh Bandewar (महेश बंडेवार) wrote:
>
> > Atomic operations are expensive (on certain architectures) and miimon
> > runs quite frequently. Is the added cost of these atomic operations
> > even worth just to avoid *duplicate info* messages? This seems like a
> > overkill!
>
> atomic_read() is a simple read, no atomic operation involved.
>
> Same remark for atomic_set()
Which makes me wonder if the patch really needs atomic_t.
Michal Kubecek
On Tue, Oct 23, 2018 at 06:26:14PM +0200, Michal Kubecek wrote:
> On Tue, Oct 23, 2018 at 09:10:44AM -0700, Eric Dumazet wrote:
> >
> >
> > On 10/23/2018 08:54 AM, Mahesh Bandewar (महेश बंडेवार) wrote:
> >
> > > Atomic operations are expensive (on certain architectures) and miimon
> > > runs quite frequently. Is the added cost of these atomic operations
> > > even worth just to avoid *duplicate info* messages? This seems like a
> > > overkill!
> >
> > atomic_read() is a simple read, no atomic operation involved.
> >
> > Same remark for atomic_set()
>
> Which makes me wonder if the patch really needs atomic_t.
IMHO it does not. AFAICS multiple instances of bond_mii_monitor() cannot
run simultaneously for the same bond so that there doesn't seem to be
anything to collide with. (And if they could, we would need to test and
set the flag atomically in bond_miimon_inspect().)
Michal Kubecek
From: [email protected]
Date: Tue, 23 Oct 2018 20:59:24 +0530
> @@ -229,6 +229,7 @@ struct bonding {
> struct dentry *debug_dir;
> #endif /* CONFIG_DEBUG_FS */
> struct rtnl_link_stats64 bond_stats;
> + atomic_t rtnl_needed;
As mentioned by others, if the only operations you perform on a value
are set and read, using atomic_t is utterly and totally pointless.
I really have no idea what is achieved by using atomic_t in this set
of circumstances.
It is not guaranteeing that the value stays stable after you read it,
and it is not guaranteeing that another thread won't overwrite the
value you just set it to.
All of those things, if important, need proper synchronization. An
atomic_t by itself will not do that for you.
> -----Original Message-----
> From: Michal Kubecek [mailto:[email protected]]
> Sent: 23 अक्तूबर 2018 22:08
> To: Eric Dumazet
> Cc: Mahesh Bandewar (महेश बंडेवार); Manish Kumar Singh; linux-netdev; Jay
> Vosburgh; Veaceslav Falico; Andy Gospodarek; David S. Miller; linux-
> [email protected]
> Subject: Re: [PATCH] bonding:avoid repeated display of same link status
> change
>
> On Tue, Oct 23, 2018 at 06:26:14PM +0200, Michal Kubecek wrote:
> > On Tue, Oct 23, 2018 at 09:10:44AM -0700, Eric Dumazet wrote:
> > >
> > >
> > > On 10/23/2018 08:54 AM, Mahesh Bandewar (महेश बंडेवार) wrote:
> > >
> > > > Atomic operations are expensive (on certain architectures) and miimon
> > > > runs quite frequently. Is the added cost of these atomic operations
> > > > even worth just to avoid *duplicate info* messages? This seems like a
> > > > overkill!
> > >
> > > atomic_read() is a simple read, no atomic operation involved.
> > >
> > > Same remark for atomic_set()
> >
> > Which makes me wonder if the patch really needs atomic_t.
>
> IMHO it does not. AFAICS multiple instances of bond_mii_monitor() cannot
> run simultaneously for the same bond so that there doesn't seem to be
> anything to collide with. (And if they could, we would need to test and
> set the flag atomically in bond_miimon_inspect().)
>
Yes, Michal, we are inline with your understanding.
when the -original- patch was posted to upstream there was no -synchronization- nor -racing- addressing code was in read/write of this added filed, as we -never- saw need for either.
-only- writer of the added field is bond_mii_monitor.
-only- reader of the added field is bond_miimon_inspect.
-this writer & reader -never- can run concurrently.
-writer invokes the reader.
hence, imo uint_8 rtnl_needed is all what is needed; with bond_mii_monitor doing rtnl_needed = 1; and bond_miimon_inspect doing if rtnl_needed.
here is the gravity of the situation with multiple customers whose names including machine names redacted:
4353 May 31 02:38:57 hostname kernel: ixgbe 0000:03:00.0: removed PHC on p2p1
4354 May 31 02:38:57 hostname kernel: public: link status down for active interface p2p1, disabling it in 100 ms
4355 May 31 02:38:57 hostname kernel: public: link status down for active interface p2p1, disabling it in 100 ms
4356 May 31 02:38:57 hostname kernel: public: link status definitely down for interface p2p1, disabling it
4357 May 31 02:38:57 hostname kernel: public: making interface p2p2 the new active one
4358 May 31 02:38:59 hostname kernel: ixgbe 0000:03:00.0: registered PHC device on p2p1
4359 May 31 02:39:00 hostname kernel: ixgbe 0000:03:00.0 p2p1: NIC Link is Up 10 Gbps, Flow Control: RX/TX
4360 May 31 02:39:00 hostname kernel: public: link status up for interface p2p1, enabling it in 200 ms
4361 May 31 02:39:00 hostname kernel: public: link status definitely up for interface p2p1, 10000 Mbps full duplex
4362 May 31 02:45:37 hostname journal: Missed 217723 kernel messages
4363 May 31 02:45:37 hostname kernel: public: link status down for active interface p2p2, disabling it in 100 ms
---------------------
11000+ APPROX SAME REPEATED MESSAGES in second
---------------------
15877 May 31 02:45:37 hostname kernel: public: link status down for active interface p2p2, disabling it in 100 ms
15878 May 31 02:45:37 hostname kernel: public: link status definitely down for interface p2p2, disabling it
15879 May 31 02:45:37 hostname kernel: public: making interface p2p1 the new active one
Thanks,
Manish
> Michal Kubecek
On Thu, Oct 25, 2018 at 02:21:05AM -0700, Manish Kumar Singh wrote:
> > From: Michal Kubecek [mailto:[email protected]]
> > IMHO it does not. AFAICS multiple instances of bond_mii_monitor() cannot
> > run simultaneously for the same bond so that there doesn't seem to be
> > anything to collide with. (And if they could, we would need to test and
> > set the flag atomically in bond_miimon_inspect().)
> >
> Yes, Michal, we are inline with your understanding.
> when the -original- patch was posted to upstream there was no
> -synchronization- nor -racing- addressing code was in read/write of this
> added filed, as we -never- saw need for either.
>
> -only- writer of the added field is bond_mii_monitor.
> -only- reader of the added field is bond_miimon_inspect.
> -this writer & reader -never- can run concurrently.
> -writer invokes the reader.
>
> hence, imo uint_8 rtnl_needed is all what is needed; with bond_mii_monitor doing rtnl_needed = 1; and bond_miimon_inspect doing if rtnl_needed.
>
> here is the gravity of the situation with multiple customers whose names including machine names redacted:
>
> 4353 May 31 02:38:57 hostname kernel: ixgbe 0000:03:00.0: removed PHC on p2p1
> 4354 May 31 02:38:57 hostname kernel: public: link status down for active interface p2p1, disabling it in 100 ms
> 4355 May 31 02:38:57 hostname kernel: public: link status down for active interface p2p1, disabling it in 100 ms
> 4356 May 31 02:38:57 hostname kernel: public: link status definitely down for interface p2p1, disabling it
> 4357 May 31 02:38:57 hostname kernel: public: making interface p2p2 the new active one
> 4358 May 31 02:38:59 hostname kernel: ixgbe 0000:03:00.0: registered PHC device on p2p1
> 4359 May 31 02:39:00 hostname kernel: ixgbe 0000:03:00.0 p2p1: NIC Link is Up 10 Gbps, Flow Control: RX/TX
> 4360 May 31 02:39:00 hostname kernel: public: link status up for interface p2p1, enabling it in 200 ms
> 4361 May 31 02:39:00 hostname kernel: public: link status definitely up for interface p2p1, 10000 Mbps full duplex
> 4362 May 31 02:45:37 hostname journal: Missed 217723 kernel messages
> 4363 May 31 02:45:37 hostname kernel: public: link status down for active interface p2p2, disabling it in 100 ms
> ---------------------
> 11000+ APPROX SAME REPEATED MESSAGES in second
> ---------------------
> 15877 May 31 02:45:37 hostname kernel: public: link status down for active interface p2p2, disabling it in 100 ms
> 15878 May 31 02:45:37 hostname kernel: public: link status definitely down for interface p2p2, disabling it
> 15879 May 31 02:45:37 hostname kernel: public: making interface p2p1 the new active one
When I was replying, I didn't know this was a v2 and I haven't seen the
v1 discussion. I have read it since and I think I understand Eric's
point now. The thing is that just adding e.g. u8 is OK as it is now.
However, someone could later add another u8 next to it which would also
be perfectly OK on its own but reads/writes to these two could collide
between each other.
And as pointed out by a colleague, even having atomic_t and u8 flag in
one 64-bit word could be a problem on architectures which cannot do an
atomic read/write from/to a 32-bit word (sparc seems to be one).
Michal Kubecek
> -----Original Message-----
> From: Michal Kubecek [mailto:[email protected]]
> Sent: 25 अक्तूबर 2018 14:59
> To: Manish Kumar Singh
> Cc: Eric Dumazet; Mahesh Bandewar (महेश बंडेवार); linux-netdev; Jay
> Vosburgh; Veaceslav Falico; Andy Gospodarek; David S. Miller; linux-
> [email protected]
> Subject: Re: [PATCH] bonding:avoid repeated display of same link status
> change
>
> On Thu, Oct 25, 2018 at 02:21:05AM -0700, Manish Kumar Singh wrote:
> > > From: Michal Kubecek [mailto:[email protected]]
> > > IMHO it does not. AFAICS multiple instances of bond_mii_monitor()
> cannot
> > > run simultaneously for the same bond so that there doesn't seem to be
> > > anything to collide with. (And if they could, we would need to test and
> > > set the flag atomically in bond_miimon_inspect().)
> > >
> > Yes, Michal, we are inline with your understanding.
> > when the -original- patch was posted to upstream there was no
> > -synchronization- nor -racing- addressing code was in read/write of this
> > added filed, as we -never- saw need for either.
> >
> > -only- writer of the added field is bond_mii_monitor.
> > -only- reader of the added field is bond_miimon_inspect.
> > -this writer & reader -never- can run concurrently.
> > -writer invokes the reader.
> >
> > hence, imo uint_8 rtnl_needed is all what is needed; with
> bond_mii_monitor doing rtnl_needed = 1; and bond_miimon_inspect doing
> if rtnl_needed.
> >
> > here is the gravity of the situation with multiple customers whose names
> including machine names redacted:
> >
> > 4353 May 31 02:38:57 hostname kernel: ixgbe 0000:03:00.0: removed PHC
> on p2p1
> > 4354 May 31 02:38:57 hostname kernel: public: link status down for active
> interface p2p1, disabling it in 100 ms
> > 4355 May 31 02:38:57 hostname kernel: public: link status down for active
> interface p2p1, disabling it in 100 ms
> > 4356 May 31 02:38:57 hostname kernel: public: link status definitely down
> for interface p2p1, disabling it
> > 4357 May 31 02:38:57 hostname kernel: public: making interface p2p2 the
> new active one
> > 4358 May 31 02:38:59 hostname kernel: ixgbe 0000:03:00.0: registered PHC
> device on p2p1
> > 4359 May 31 02:39:00 hostname kernel: ixgbe 0000:03:00.0 p2p1: NIC Link is
> Up 10 Gbps, Flow Control: RX/TX
> > 4360 May 31 02:39:00 hostname kernel: public: link status up for interface
> p2p1, enabling it in 200 ms
> > 4361 May 31 02:39:00 hostname kernel: public: link status definitely up for
> interface p2p1, 10000 Mbps full duplex
> > 4362 May 31 02:45:37 hostname journal: Missed 217723 kernel messages
> > 4363 May 31 02:45:37 hostname kernel: public: link status down for active
> interface p2p2, disabling it in 100 ms
> > ---------------------
> > 11000+ APPROX SAME REPEATED MESSAGES in second
> > ---------------------
> > 15877 May 31 02:45:37 hostname kernel: public: link status down for active
> interface p2p2, disabling it in 100 ms
> > 15878 May 31 02:45:37 hostname kernel: public: link status definitely down
> for interface p2p2, disabling it
> > 15879 May 31 02:45:37 hostname kernel: public: making interface p2p1 the
> new active one
>
> When I was replying, I didn't know this was a v2 and I haven't seen the
> v1 discussion. I have read it since and I think I understand Eric's
> point now. The thing is that just adding e.g. u8 is OK as it is now.
> However, someone could later add another u8 next to it which would also
> be perfectly OK on its own but reads/writes to these two could collide
> between each other.
>
> And as pointed out by a colleague, even having atomic_t and u8 flag in
> one 64-bit word could be a problem on architectures which cannot do an
> atomic read/write from/to a 32-bit word (sparc seems to be one).
Thanks Michal for explaining it, now we understand the problem what Eric was referring to in v1 of the patch.
I could think of fixing it in 3 ways, Please suggest which one would be safe and optimal fix:
1. Use type unit64_t for rtnl_needed .
2. Use type atomic64_t for rtnl_needed and atomic64_set/read.
3. Use type uint64_t for rtnl_needed with spinlock protection.
I think option 3 would be overkill keeping in mind the frequency of bond_mii_monitor.
Thanks,
Manish
>
> Michal Kubecek