* Re: [PATCH] bonding: replace system timer with work queue
[not found] <Pine.LNX.4.61.0702281000050.9439@tm8103-a.perex-int.cz>
@ 2007-03-01 7:35 ` Andrew Morton
2007-03-01 16:00 ` Stephen Hemminger
2007-03-01 17:00 ` Jay Vosburgh
0 siblings, 2 replies; 3+ messages in thread
From: Andrew Morton @ 2007-03-01 7:35 UTC (permalink / raw)
To: Jaroslav Kysela; +Cc: LKML, Stephen Hemminger, Oleg Nesterov, netdev
On Wed, 28 Feb 2007 10:12:01 +0100 (CET) Jaroslav Kysela <perex@suse.cz> wrote:
> Hi,
>
> please, review and apply to mm tree for further testing. The patch
> is also available at
> ftp://ftp.alsa-project.org/pub/kernel-patches/bonding-workqueue.patch .
Please cc netdev@vger.kernel.org on net-related patches, thanks.
> Thank you,
> Jaroslav
>
> ==================
> bonding: replace system timer with work queue
>
> This patch replaces system timer with work queue in monitor functions.
> The reason for this change is that bonding handlers calls various
> sleeping functions from the timer handler which is not allowed.
Which sleeping functions? I'd have expected the kernel to spew runtime
warnings when this happens, but I don't recall any such reports.
> Because we cannot share the main workqueue threads (rtnl_lock is used
> also in linkwatch_event) - new bond workqueue thread is created.
>
> Signed-off-by: Jaroslav Kysela <perex@suse.cz>
>
> diff -rupN linux-2.6.20.orig/drivers/net/bonding/bond_3ad.c linux-2.6.20/drivers/net/bonding/bond_3ad.c
> --- linux-2.6.20.orig/drivers/net/bonding/bond_3ad.c 2007-02-04 19:44:54.000000000 +0100
> +++ linux-2.6.20/drivers/net/bonding/bond_3ad.c 2007-02-28 09:19:43.831369202 +0100
> @@ -2097,8 +2097,10 @@ void bond_3ad_unbind_slave(struct slave
> * times out, and it selects an aggregator for the ports that are yet not
> * related to any aggregator, and selects the active aggregator for a bond.
> */
> -void bond_3ad_state_machine_handler(struct bonding *bond)
> +void bond_3ad_state_machine_handler(struct work_struct *work)
> {
> + struct ad_bond_info *ad_info = container_of(work, struct ad_bond_info, ad_work.work);
> + struct bonding *bond = (struct bonding *)((char *)ad_info - offsetof(struct bonding, ad_info));
We can use containers_of here too?
> -void bond_alb_monitor(struct bonding *bond)
> +void bond_alb_monitor(struct work_struct *work)
> {
> - struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
> + struct alb_bond_info *bond_info = container_of(work, struct alb_bond_info, alb_work.work);
> + struct bonding *bond = (struct bonding *)((char *)bond_info - offsetof(struct bonding, alb_info));
And here.
> + cancel_rearming_delayed_workqueue(bond_wq, &(BOND_AD_INFO(bond).ad_work));
> break;
> case BOND_MODE_TLB:
> case BOND_MODE_ALB:
> - del_timer_sync(&(BOND_ALB_INFO(bond).alb_timer));
> + cancel_rearming_delayed_workqueue(bond_wq, &(BOND_ALB_INFO(bond).alb_work));
> break;
> default:
> break;
> @@ -4289,6 +4272,14 @@ static int bond_init(struct net_device *
> rwlock_init(&bond->lock);
> rwlock_init(&bond->curr_slave_lock);
>
> + /* initialize work */
> + INIT_DELAYED_WORK(&bond->mii_work, (void *)&bond_mii_monitor);
> + if (params->mode == BOND_MODE_ACTIVEBACKUP) {
> + INIT_DELAYED_WORK(&bond->arp_work, (void *)&bond_activebackup_arp_mon);
> + } else {
> + INIT_DELAYED_WORK(&bond->arp_work, (void *)&bond_loadbalance_arp_mon);
> + }
Can we lose the unneeded braces, the unneeded typecasts and fit the code
into 80 cols?
<does all that>
yup.
> bond->params = *params; /* copy params struct */
>
> /* Initialize pointers */
> @@ -4782,6 +4773,12 @@ static int __init bonding_init(void)
> goto err;
> }
>
> + bond_wq = create_singlethread_workqueue("bond");
> + if (bond_wq == NULL) {
> + res = -ENOMEM;
> + goto err;
> + }
> +
> res = bond_create_sysfs();
> if (res)
> goto err;
> @@ -4807,6 +4804,7 @@ static void __exit bonding_exit(void)
>
> rtnl_lock();
> bond_free_all();
> + destroy_workqueue(bond_wq);
> bond_destroy_sysfs();
> rtnl_unlock();
Are you sure that all pending delayed works have been cancelled when we
destroy this workqueue?
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] bonding: replace system timer with work queue
2007-03-01 7:35 ` [PATCH] bonding: replace system timer with work queue Andrew Morton
@ 2007-03-01 16:00 ` Stephen Hemminger
2007-03-01 17:00 ` Jay Vosburgh
1 sibling, 0 replies; 3+ messages in thread
From: Stephen Hemminger @ 2007-03-01 16:00 UTC (permalink / raw)
To: Andrew Morton; +Cc: Jaroslav Kysela, LKML, Oleg Nesterov, netdev
Andrew Morton wrote:
> On Wed, 28 Feb 2007 10:12:01 +0100 (CET) Jaroslav Kysela <perex@suse.cz> wrote:
>
>
>> Hi,
>>
>> please, review and apply to mm tree for further testing. The patch
>> is also available at
>> ftp://ftp.alsa-project.org/pub/kernel-patches/bonding-workqueue.patch .
>>
>
> Please cc netdev@vger.kernel.org on net-related patches, thanks.
>
>
>> Thank you,
>> Jaroslav
>>
>> ==================
>> bonding: replace system timer with work queue
>>
>> This patch replaces system timer with work queue in monitor functions.
>> The reason for this change is that bonding handlers calls various
>> sleeping functions from the timer handler which is not allowed.
>>
>
> Which sleeping functions? I'd have expected the kernel to spew runtime
> warnings when this happens, but I don't recall any such reports.
>
>
>
>> Because we cannot share the main workqueue threads (rtnl_lock is used
>> also in linkwatch_event) - new bond workqueue thread is created.
>>
>> Signed-off-by: Jaroslav Kysela <perex@suse.cz>
>>
>> diff -rupN linux-2.6.20.orig/drivers/net/bonding/bond_3ad.c linux-2.6.20/drivers/net/bonding/bond_3ad.c
>> --- linux-2.6.20.orig/drivers/net/bonding/bond_3ad.c 2007-02-04 19:44:54.000000000 +0100
>> +++ linux-2.6.20/drivers/net/bonding/bond_3ad.c 2007-02-28 09:19:43.831369202 +0100
>> @@ -2097,8 +2097,10 @@ void bond_3ad_unbind_slave(struct slave
>> * times out, and it selects an aggregator for the ports that are yet not
>> * related to any aggregator, and selects the active aggregator for a bond.
>> */
>> -void bond_3ad_state_machine_handler(struct bonding *bond)
>> +void bond_3ad_state_machine_handler(struct work_struct *work)
>> {
>> + struct ad_bond_info *ad_info = container_of(work, struct ad_bond_info, ad_work.work);
>> + struct bonding *bond = (struct bonding *)((char *)ad_info - offsetof(struct bonding, ad_info));
>>
>
> We can use containers_of here too?
>
>
>> -void bond_alb_monitor(struct bonding *bond)
>> +void bond_alb_monitor(struct work_struct *work)
>> {
>> - struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
>> + struct alb_bond_info *bond_info = container_of(work, struct alb_bond_info, alb_work.work);
>> + struct bonding *bond = (struct bonding *)((char *)bond_info - offsetof(struct bonding, alb_info));
>>
>
> And here.
>
>
>> + cancel_rearming_delayed_workqueue(bond_wq, &(BOND_AD_INFO(bond).ad_work));
>>
As I mentioned earlier this call to cancel_rearming_delayed_workqueue
can deadlock
with netlink_watch. This happens if:
dev_close
rtnl_lock carrier lost on device
bond_close netlink related workqueue event waiting
for rtnl
cancel_workqueue
spinning waiting for workq to drain
The agreed upon semantics is to never do any operation that waits for workq
to drain with RTNL held.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] bonding: replace system timer with work queue
2007-03-01 7:35 ` [PATCH] bonding: replace system timer with work queue Andrew Morton
2007-03-01 16:00 ` Stephen Hemminger
@ 2007-03-01 17:00 ` Jay Vosburgh
1 sibling, 0 replies; 3+ messages in thread
From: Jay Vosburgh @ 2007-03-01 17:00 UTC (permalink / raw)
To: Andrew Morton
Cc: Jaroslav Kysela, LKML, Stephen Hemminger, Oleg Nesterov, netdev,
Andy Gospodarek
Andrew Morton <akpm@linux-foundation.org> wrote:
>On Wed, 28 Feb 2007 10:12:01 +0100 (CET) Jaroslav Kysela <perex@suse.cz> wrote:
>> ==================
>> bonding: replace system timer with work queue
>>
>> This patch replaces system timer with work queue in monitor functions.
>> The reason for this change is that bonding handlers calls various
>> sleeping functions from the timer handler which is not allowed.
>
>Which sleeping functions? I'd have expected the kernel to spew runtime
>warnings when this happens, but I don't recall any such reports.
This affects one specific mode (balance-alb) in one specific
case (moving MAC addresses around, which happens during failover or
initialization), and a full fix is more complicated than just a switch
to work queues, although that is part of the full fix. There are three
things going on: calls to sleeping functions with locks held, the same
calls from the timer context, and rtnl hold issues.
The actual functions affected are various things called by
notifier NETDEV_CHANGEADDR callbacks started by dev_set_mac_address() as
well as some of the driver level set_mac_address functions that may
sleep.
Andy Gospodarek <andy@greyhouse.net> and I have been working
jointly on a two phased fix for these problems: he's working up the
short term fix, which includes the changeover to workqueues, and I've
been working on the long term fix, which involves refactoring the
bonding link monitoring and failover system. Jaroslav's patch looks to
be a subset of the patch Andy is working on.
-J
---
-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2007-03-01 17:00 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <Pine.LNX.4.61.0702281000050.9439@tm8103-a.perex-int.cz>
2007-03-01 7:35 ` [PATCH] bonding: replace system timer with work queue Andrew Morton
2007-03-01 16:00 ` Stephen Hemminger
2007-03-01 17:00 ` Jay Vosburgh
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).