From: Jon M. <jon...@er...> - 2004-05-13 19:46:45
|
Just to avoid misunderstandings: I am not working with the changes I suggested. I am struggling with trying to understand the usage of netdevice notifications, one of the issues Stephen H.'s called a showstopper in his review. Btw, does anybody know if NETDEV_DOWN/UP means that the carrier disappeared/reappeared (I hope so), or does it simply mean that somebody did "ifconfig ethX down" ? (I think so). Is there any description of how this is supposed to be used ? /Jon Jon Maloy wrote: Hi Mark, The general principle I had in mind when designing this was to not keep a node-lock and the port-lock at the same time, but in some cases this looks unavoidable, such as in the wakeup-function you corrected yesterday. In other cases I have just not been aware of the potential problem, even when it is easy to fix. The current problem is in fact on of the latter type; as it is completely unecessary to call port_send_protocol_msg() with the port lock on. What we need here is a small redesign to resolve the whole problem: Everywhere port_send_protocol_msg() is called (in port_timeout(), port_recv_protocol_msg() and a few other places) we should 1) build the message (we rename port_send_protocol_msg() to e.g. port_build_protocol_msg() and let it return an sk_buff* ) 2) Update the port's sequence number, when applicable. 3) Release the port lock. 4) Send the buffer through net_route_msg() There is one call where this is difficult, in port_abort_self((), since this itself is called with the lock on, but on the other hand this only sends a message to the own port, not to any link, so no node lock will be taken. This should resolve the deadlock problem between port and node locks once and for all, I hope. The rest is about proper documenation. Regards /Jon Mark Haverkamp wrote: On Wed, 2004-05-12 at 13:25, Mark Haverkamp wrote: On Wed, 2004-05-12 at 12:09, Jon Maloy wrote: Yeah, that's a classical one. I also think your solution is ok; the pending ports will be awakened at next message reception, so no harm done. Thanks /jon I found another one. I got another hang yesterday after the current deadlock fix. I re-added my spinlock debug code and found out that we're getting a deadlock between the node lock and the tipc_port lock. It looks like the port timeout handler is running on one CPU and a recv_msg is running on the other. I suppose as a workaround, we could make all the spin lock access in wakeup be conditional, but that will probably just make the problem just show up somewhere else. There should probably be an analysis of code paths and determine how the locks interact with each other. I have noticed that there is at least one place where three locks are required. This can cause problems like we've seen when different code paths need multiple locks unless there is some sort of back off method to insure no deadlocks. For instance if a function needs both the node lock and tipc_port lock, something like this could be done: - - - - - - again: spinlock(node_lock); spintrylock(tipc_port_lock) if failed to acquire then release node_lock and goto again. - - - - - - - - Unfortunately, if one of the locks was acquired before the function was entered, releasing that lock may allow some state to change that the caller was assuming wouldn't change and that could cause problems too. So, I'm not sure that this method could work in the link_wakeup_ports function. The node lock is held on entry and if the tipc_port lock can't be acquired, you probably can't just release the node lock and wait for it again since that could invalidate the tipc_recv_msg functions assumptions about what the node lock was protecting. Anyway, here is the raw trace from the debug: &node->lock lock timeout Call Trace: [<f8a8543a>] link_send+0xda/0x2a0 [tipc] [<f8a92c8e>] net_route_msg+0x41e/0x43d [tipc] [<f8a94962>] port_send_proto_msg+0x1a2/0x2a0 [tipc] [<f8aa1b10>] k_signal+0x60/0x170 [tipc] [<f8a956be>] port_timeout+0xbe/0x200 [tipc] [<f8aa1ccc>] timeout_receiver+0xac/0x110 [tipc] [<f8aa1a9f>] receive_signal+0x14f/0x160 [tipc] [<c011ab51>] scheduler_tick+0x111/0x690 [<c0124ec3>] tasklet_action+0x73/0xc0 [<c0124bc8>] __do_softirq+0xb8/0xc0 [<c0124c05>] do_softirq+0x35/0x40 [<c011526d>] smp_apic_timer_interrupt+0xdd/0x150 [<c0105d62>] apic_timer_interrupt+0x1a/0x20 [<c01119c0>] delay_pit+0x20/0x30 [<c0221694>] __delay+0x14/0x20 [<f8a8685f>] link_send_sections_long+0x50f/0xb30 [tipc] [<c01152d1>] smp_apic_timer_interrupt+0x141/0x150 [<f8a8366f>] link_schedule_port+0x13f/0x1e0 [tipc] [<f8a86095>] link_send_sections_fast+0x5b5/0x870 [tipc] [<f8a97b92>] tipc_send+0x92/0x9d0 [tipc] [<c03f14cf>] schedule+0x37f/0x7a0 [<f8aa3386>] recv_msg+0x2b6/0x560 [tipc] [<f8aa2d30>] send_packet+0x90/0x180 [tipc] [<c011b0d0>] default_wake_function+0x0/0x20 [<c036c83e>] sock_sendmsg+0x8e/0xb0 [<f8a8784e>] tipc_recv_msg+0x47e/0x8c0 [tipc] [<c01435ba>] buffered_rmqueue+0xfa/0x220 [<c036c61a>] sockfd_lookup+0x1a/0x80 [<c036dd61>] sys_sendto+0xe1/0x100 [<c0128f62>] del_timer_sync+0x42/0x140 [<c036d109>] sock_poll+0x29/0x30 [<c017884b>] do_pollfd+0x5b/0xa0 [<c036ddb6>] sys_send+0x36/0x40 [<c036e60e>] sys_socketcall+0x12e/0x240 [<c0105373>] syscall_call+0x7/0xb port->publ lock timeout Call Trace: [<f8a8383a>] link_wakeup_ports+0x12a/0x1d0 [tipc] [<f8a87bce>] tipc_recv_msg+0x7fe/0x8c0 [tipc] [<c014949d>] __kmalloc+0x19d/0x250 [<f8aa5d59>] recv_msg+0x39/0x50 [tipc] [<c0375af2>] netif_receive_skb+0x172/0x1b0 [<c0375bb4>] process_backlog+0x84/0x120 [<c0375cd0>] net_rx_action+0x80/0x120 [<c0124bc8>] __do_softirq+0xb8/0xc0 [<c0124c05>] do_softirq+0x35/0x40 [<c0107ce5>] do_IRQ+0x175/0x230 [<c0105ea5>] nmi_stack_correct+0x1e/0x2e [<c0105ce0>] common_interrupt+0x18/0x20 |