[PATCH net-next v2 0/4] bonding work-queues, try_rtnl() & notifications

[PATCH net-next v2 0/4] bonding work-queues, try_rtnl() & notifications

[Mahesh Bandewar]

This patch series tries to address the issue discovered in various work-
queues and the way these handlers deal with the RTNL. Especially for 
notification handling. If RTNL can not be acquired, these handlers ignore
sending notifications and just re-arm the timer. This could be very 
problematic if the re-arm timer has larger value (e.g. in minutes).

Mahesh Bandewar (4):
  bonding: Handle notifications during work-queue processing gracefully
  bonding: Do not ignore notifications for miimon-work-queue
  bonding: Do not ignore notifications for AD-work-queue
  bonding: Do not ignore notifications for ARP-work-queue

 drivers/net/bonding/bond_3ad.c  | 21 +++++++++++---
 drivers/net/bonding/bond_main.c | 64 ++++++++++++++++++++++++-----------------
 include/net/bonding.h           | 19 ++++++++++++
 3 files changed, 74 insertions(+), 30 deletions(-)

-- 
2.2.0.rc0.207.ga3a616c

Re: [PATCH net-next v2 0/4] bonding work-queues, try_rtnl() & notifications

[David Miller]

From: Mahesh Bandewar <maheshb@google.com>
Date: Fri, 20 Mar 2015 22:39:15 -0700

> This patch series tries to address the issue discovered in various work-
> queues and the way these handlers deal with the RTNL. Especially for 
> notification handling. If RTNL can not be acquired, these handlers ignore
> sending notifications and just re-arm the timer. This could be very 
> problematic if the re-arm timer has larger value (e.g. in minutes).

I already made it very clear how I want this kind of problem solved.

Make the person holding the RTNL semaphore do the notification work
for you.

All of this deferral stuff is error prone and sloppy.

Re: [PATCH net-next v2 0/4] bonding work-queues, try_rtnl() & notifications

[Mahesh Bandewar]

On Sat, Mar 21, 2015 at 3:17 PM, David Miller <davem@davemloft.net> wrote:
>
> From: Mahesh Bandewar <maheshb@google.com>
> Date: Fri, 20 Mar 2015 22:39:15 -0700
>
> > This patch series tries to address the issue discovered in various work-
> > queues and the way these handlers deal with the RTNL. Especially for
> > notification handling. If RTNL can not be acquired, these handlers ignore
> > sending notifications and just re-arm the timer. This could be very
> > problematic if the re-arm timer has larger value (e.g. in minutes).
>
> I already made it very clear how I want this kind of problem solved.
>
> Make the person holding the RTNL semaphore do the notification work
> for you.
>
> All of this deferral stuff is error prone and sloppy.

I think you missed my earlier reply to your message. In summary, the
notification deferral is 1ms which should not cause any trashing; more
over with that suggested approach the rtnl-owner will have to do your
job and will be subjected to the latencies which may be undesirable.

OTOH this patch series is not adding any new deferral work, but just
making it bug-free.

Thanks,
--mahesh..

Re: [PATCH net-next v2 0/4] bonding work-queues, try_rtnl() & notifications

[David Miller]

From: Mahesh Bandewar <maheshb@google.com>
Date: Sat, 21 Mar 2015 19:01:29 -0700

> I think you missed my earlier reply to your message. In summary, the
> notification deferral is 1ms which should not cause any trashing;
> more over with that suggested approach the rtnl-owner will have to
> do your job and will be subjected to the latencies which may be
> undesirable.

Someone has to do the work, and if the rtnl-owner does it we do not
incur all of the overhead the resubmission entails.

> OTOH this patch series is not adding any new deferral work, but just
> making it bug-free.

It's continuing a bad approach to the problem.

Re: [PATCH net-next v2 0/4] bonding work-queues, try_rtnl() & notifications

[Maciej Żenczykowski]

> Someone has to do the work, and if the rtnl-owner does it we do not
> incur all of the overhead the resubmission entails.

Are you suggesting:
- adding a global queue of (function pointer, long arg)
- changing rtnl_unlock to iterate through the queue calling
function(arg) before it actually releases the rtnl lock
?

(life might be easier if you can actually pass 2 args, but that's a
minor detail)

I guess at that point you could even have a generic method

void asynch_call_under_rtnl(function, arg) {
  rtnl_callback_add_to_queue(function,arg)
  if (rtnl_try_lock())
    rtnl_unlock();
}

- Maciej

Re: [PATCH net-next v2 0/4] bonding work-queues, try_rtnl() & notifications

[David Miller]

From: Maciej Żenczykowski <maze@google.com>
Date: Sat, 21 Mar 2015 19:23:51 -0700

>> Someone has to do the work, and if the rtnl-owner does it we do not
>> incur all of the overhead the resubmission entails.
> 
> Are you suggesting:
> - adding a global queue of (function pointer, long arg)
> - changing rtnl_unlock to iterate through the queue calling
> function(arg) before it actually releases the rtnl lock
> ?

Something like that.

> I guess at that point you could even have a generic method
> 
> void asynch_call_under_rtnl(function, arg) {
>   rtnl_callback_add_to_queue(function,arg)
>   if (rtnl_try_lock())
>     rtnl_unlock();
> }

Right.

Re: [PATCH net-next v2 0/4] bonding work-queues, try_rtnl() & notifications

[Maciej Żenczykowski]

> One major hurdle as I see in this approach is that if a module uses this and
> gets unloaded before the next rtnl-unlock call, it would face catastrophic
> consequences. However it can be avoided but there will be cost to manipulate
> that while the gains are not that great.

One very dirty solution would be to always build the couple functions
that need to be called like this into the kernel proper...

But on second thought I actually don't see the problem - functions get
called *before* mutex is released.
All you need to do is rtnl_lock/unlock post deregistration (of any
code that could add callbacks) in the module exit code. Since you've
already deregistered no-one will add a new callback to the queue, and
since you grab/release the rtnl lock you flush the queue, and you're
clear.