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 | 8 ++++++--
include/net/bonding.h | 1 +
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 2b01180be834..b3d95c7040ac 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2096,7 +2096,8 @@ 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 &&
+ !atomic64_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 +2137,8 @@ static int bond_miimon_inspect(struct bonding *bond)
commit++;
slave->delay = bond->params.updelay;
- if (slave->delay) {
+ if (slave->delay &&
+ !atomic64_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 +2312,11 @@ static void bond_mii_monitor(struct work_struct *work)
if (!rtnl_trylock()) {
delay = 1;
should_notify_peers = false;
+ atomic64_set(&bond->rtnl_needed, 1);
goto re_arm;
}
+ atomic64_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..20c3c875266f 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;
+ atomic64_t rtnl_needed;
};
#define bond_slave_get_rcu(dev) \
--
2.14.1
From: [email protected]
Date: Wed, 31 Oct 2018 16:27:28 +0530
> - if (slave->delay) {
> + if (slave->delay &&
> + !atomic64_read(&bond->rtnl_needed)) {
...
> + !atomic64_read(&bond->rtnl_needed)) {
...
> + atomic64_set(&bond->rtnl_needed, 1);
...
> + atomic64_set(&bond->rtnl_needed, 0);
...
> @@ -229,6 +229,7 @@ struct bonding {
> struct dentry *debug_dir;
> #endif /* CONFIG_DEBUG_FS */
> struct rtnl_link_stats64 bond_stats;
> + atomic64_t rtnl_needed;
There is nothing "atomic" about a value that is only set and read.
And using a full 64-bit value for something taking on only '0' and
'1' is unnecessary as well.
On Fri, Nov 02, 2018 at 11:31:38PM -0700, David Miller wrote:
> From: [email protected]
> Date: Wed, 31 Oct 2018 16:27:28 +0530
>
> > - if (slave->delay) {
> > + if (slave->delay &&
> > + !atomic64_read(&bond->rtnl_needed)) {
> ...
> > + !atomic64_read(&bond->rtnl_needed)) {
> ...
> > + atomic64_set(&bond->rtnl_needed, 1);
> ...
> > + atomic64_set(&bond->rtnl_needed, 0);
> ...
> > @@ -229,6 +229,7 @@ struct bonding {
> > struct dentry *debug_dir;
> > #endif /* CONFIG_DEBUG_FS */
> > struct rtnl_link_stats64 bond_stats;
> > + atomic64_t rtnl_needed;
>
> There is nothing "atomic" about a value that is only set and read.
>
> And using a full 64-bit value for something taking on only '0' and
> '1' is unnecessary as well.
Part of the misunderstanding is caused by the fact that this is actually
a v4 but not marked as such:
v1: https://patchwork.ozlabs.org/patch/955789/
v2: https://patchwork.ozlabs.org/patch/970421/
v3: https://patchwork.ozlabs.org/patch/988241/
When commenting v3, I didn't know about the v2 discussion where Eric
Dumazet NACKed the patch because of potential conflict issues:
https://patchwork.ozlabs.org/patch/970421/#1992397
https://patchwork.ozlabs.org/patch/988241/#2017317
On the other hand, there is no need for atomic64_t. Simple atomic_t
(with explaining comment) would suffice. On architectures allowing
atomic read/write for 32-bit integers, there would be no performance
penalty. On architectures not allowing it, atomic_read() and
atomic_set() are implemented to be safe.
Michal Kubecek
> -----Original Message-----
> From: Michal Kubecek [mailto:[email protected]]
> Sent: 05 नवम्बर 2018 01:11
> To: David Miller
> Cc: Manish Kumar Singh; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]; linux-
> [email protected]
> Subject: Re: [PATCH] bonding:avoid repeated display of same link status
> change
>
> On Fri, Nov 02, 2018 at 11:31:38PM -0700, David Miller wrote:
> > From: [email protected]
> > Date: Wed, 31 Oct 2018 16:27:28 +0530
> >
> > > - if (slave->delay) {
> > > + if (slave->delay &&
> > > + !atomic64_read(&bond->rtnl_needed)) {
> > ...
> > > + !atomic64_read(&bond->rtnl_needed)) {
> > ...
> > > + atomic64_set(&bond->rtnl_needed, 1);
> > ...
> > > + atomic64_set(&bond->rtnl_needed, 0);
> > ...
> > > @@ -229,6 +229,7 @@ struct bonding {
> > > struct dentry *debug_dir;
> > > #endif /* CONFIG_DEBUG_FS */
> > > struct rtnl_link_stats64 bond_stats;
> > > + atomic64_t rtnl_needed;
> >
> > There is nothing "atomic" about a value that is only set and read.
> >
> > And using a full 64-bit value for something taking on only '0' and
> > '1' is unnecessary as well.
>
> Part of the misunderstanding is caused by the fact that this is actually
> a v4 but not marked as such:
>
> v1: https://patchwork.ozlabs.org/patch/955789/
> v2: https://patchwork.ozlabs.org/patch/970421/
> v3: https://patchwork.ozlabs.org/patch/988241/
>
> When commenting v3, I didn't know about the v2 discussion where Eric
> Dumazet NACKed the patch because of potential conflict issues:
>
> https://patchwork.ozlabs.org/patch/970421/#1992397
> https://patchwork.ozlabs.org/patch/988241/#2017317
>
> On the other hand, there is no need for atomic64_t. Simple atomic_t
> (with explaining comment) would suffice. On architectures allowing
> atomic read/write for 32-bit integers, there would be no performance
> penalty. On architectures not allowing it, atomic_read() and
> atomic_set() are implemented to be safe.
Sorry for late response, I was off to work for couple of weeks.
v3: https://patchwork.ozlabs.org/patch/988241/ was sent with atomic_t and after seeing your comment, I sent it with atomic64_t.
Please let me know if v3 was fine ?
Thanks,
Manish
>
> Michal Kubecek