netdev - Re: [PATCH net-next 4/4] bonding: Do not ignore notifications for ARP-work-queue

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Thu, 12 Mar 2015 19:03:48 -0700
From:	Mahesh Bandewar <maheshb@...gle.com>
To:	Nikolay Aleksandrov <nikolay@...hat.com>
Cc:	Jay Vosburgh <j.vosburgh@...il.com>,
	Andy Gospodarek <andy@...yhouse.net>,
	Veaceslav Falico <vfalico@...il.com>,
	David Miller <davem@...emloft.net>,
	Maciej Zenczykowski <maze@...gle.com>,
	netdev <netdev@...r.kernel.org>,
	Eric Dumazet <edumazet@...gle.com>
Subject: Re: [PATCH net-next 4/4] bonding: Do not ignore notifications for ARP-work-queue

On Thu, Mar 12, 2015 at 9:12 AM, Nikolay Aleksandrov <nikolay@...hat.com> wrote:
> On 03/12/2015 06:54 AM, Mahesh Bandewar wrote:
>> This patch adds code to reschedule the ARP-work (aggressively)
>> to handle the notifications before resuming the regular cycle.
>>
>> Signed-off-by: Mahesh Bandewar <maheshb@...gle.com>
>> ---
>>  drivers/net/bonding/bond_main.c | 38 ++++++++++++++++++++++----------------
>>  1 file changed, 22 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index 54ecb7a22bae..882974d543d2 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -2814,17 +2814,20 @@ static void bond_activebackup_arp_mon(struct work_struct *work)
>>                                           arp_work.work);
>>       bool should_notify_peers = false;
>>       bool should_notify_rtnl = false;
>> -     int delta_in_ticks;
>> +     unsigned long delta_in_ticks;
>>
>>       delta_in_ticks = msecs_to_jiffies(bond->params.arp_interval);
>>
>>       if (!bond_has_slaves(bond))
>>               goto re_arm;
>>
>> -     rcu_read_lock();
>> -
>>       should_notify_peers = bond_should_notify_peers(bond);
>> +     if (bond_get_notif_pending(bond, BOND_ARP_NOTIF)) {
>> +             rcu_read_lock();
>> +             goto eval_arp_probe;
>> +     }
>>
>> +     rcu_read_lock();
> ^^^^^^^
> Since rcu_read_lock() is acquired in both cases, why don't you leave it
> where it is now ? Then you'll be able to save a line and drop the { }
> on the "if" above.
>
OK, I was under false impression of not-needing-rcu for
should_notify_peers() and hence removed the rcu_read_lock() from the
original location. I'll reinstate.

>
>>       if (bond_ab_arp_inspect(bond)) {
>>               rcu_read_unlock();
>>
>> @@ -2841,25 +2844,28 @@ static void bond_activebackup_arp_mon(struct work_struct *work)
>>               rcu_read_lock();
>>       }
>>
>> +eval_arp_probe:
>>       should_notify_rtnl = bond_ab_arp_probe(bond);
> ^^^^^
> Keep in mind that bond_ab_arp_probe() calls bond_arp_send_all() each time
> if we have an active slave. We could be sending ARP requests each tick
> until rtnl gets acquired.
>
well, that will not be a good behavior however seldom it would be.
I'll update code to avoid that in the next patch-set.

>>       rcu_read_unlock();
>>
>>  re_arm:
>> -     if (bond->params.arp_interval)
>> -             queue_delayed_work(bond->wq, &bond->arp_work, delta_in_ticks);
>> -
>>       if (should_notify_peers || should_notify_rtnl) {
>> -             if (!rtnl_trylock())
>> -                     return;
>> -
>> -             if (should_notify_peers)
>> -                     call_netdevice_notifiers(NETDEV_NOTIFY_PEERS,
>> -                                              bond->dev);
>> -             if (should_notify_rtnl)
>> -                     bond_slave_state_notify(bond);
>> -
>> -             rtnl_unlock();
>> +             if (!rtnl_trylock()) {
>> +                     delta_in_ticks = 1;
>> +                     bond_set_notif_pending(bond, BOND_ARP_NOTIF, 1);
>> +             } else {
>> +                     if (should_notify_rtnl)
>> +                             bond_slave_state_notify(bond);
>> +                     if (should_notify_peers)
>> +                             call_netdevice_notifiers(NETDEV_NOTIFY_PEERS,
>> +                                                      bond->dev);
>> +                     rtnl_unlock();
>> +                     bond_set_notif_pending(bond, BOND_ARP_NOTIF, 0);
>> +             }
>>       }
>> +
>> +     if (bond->params.arp_interval)
>> +             queue_delayed_work(bond->wq, &bond->arp_work, delta_in_ticks);
>>  }
>>
>>  /*-------------------------- netdev event handling --------------------------*/
>>
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html