On Mon, Dec 28, 2009 at 04:10:08PM +0100, Simon Wunderlich wrote:
This patch removes the (ugly and racy) packet receiving thread and the kernel socket usage. Instead, packets are received directly by registering the ethernet type and handling skbs instead of self-allocated buffers.
Some consequences and comments:
- we don't copy the payload data when forwarding/sending/receiving data anymore. This should boost performance.
- packets from/to different interfaces can be (theoretically) processed simultaneously. Only the big originator hash lock might be in the way.
- this might introduce new race conditions.
- aggregation and vis code still use packet buffers and are not (yet) converted.
This is the second version of this patch to be released, i would consider it experimental and would hereby like to as for reviews before committing it. Some things you might want to test:
- performace differences (are there any?)
- do all components still work? (vis, batctl ping, ...)
- do high load situations or multiple interfaces cause problems
- any memory leaks i might have overlooked?
I did some tests on my 9 node qemu environment and could confirm that usual sending/receiving, forwarding, vis, batctl ping etc works. However i can not talk about the performance from this setup.
Things changed from the first version are:
- always keep the skb->data at the batman header within the packet handling functions
- use skb_copy() before modifying the data, if needed
- more fragmentation friendly header handling
- use skb_headlen() instead of asking skb->len
- fix some small bugs (use kfree(skb) -> kfree_skb(skb))
Signed-off-by: Simon Wunderlich siwu@hrz.tu-chemnitz.de
This patch looks better to me, but your tests yesterday showed that there is a possible deadlock due to the usage of spinlocks with disabled and enabled irqs. I know that you started to convert all spin_lock to spin_lock_irqsave, but this may include locks which aren't affected by the wo-irqs/w-irqs situation.
I try to summarize where each lock is used so we can decide if it is ok to revert some of them to spin_lock again. This should also resolve a request I got from Andrew some time ago.
- vis_hash_lock - vis_set_mode() - is_vis_server() - receive_client_update_packet() - send_vis_packets() - vis_init() - vis_quit()
-> receive function is called with disable irq -> so we must use spin_lock_irqsave
- hna_global_hash_lock - hna_local_add - hna_global_add_orig - hna_global_fill_buffer_text - hna_global_del_orig - transtable_search
-> add functions are used when we update routes after we received a bat packet. So this must disable irqs too
- hna_local_hash_lock - hna_local_add - hna_local_fill_buffer - hna_local_fill_buffer_text - hna_local_purge - hna_global_add_orig - generate_vis_packet
-> add functions are used when we update routes after we received a bat packet. So this must disable irqs too
- forw_bcast_list_lock - _add_bcast_packet_to_list - send_outstanding_bcast_packet - purge_outstanding_packets
-> Was already changed to irqsave in "batman-adv: Use forw_bcast_list_lock always in disabled interrupt context"
- forw_bat_list_lock - new_aggregated_packet - add_bat_packet_to_list - send_outstanding_bat_packet - purge_outstanding_packets
-> for example add_bat_packet_to_list is called by schedule_forward_packet which is called by receive_bat_packet - called by receive_aggr_bat_packet - called by recv_bat_packet (called with disabled irqs). So we must use irqsave
- orig_hash_lock - bat_device_write - hardif_add_interface - originator_init - originator_free - purge_orig - proc_originators_read - slide_own_bcast_window - recv_bat_packet - recv_my_icmp_packet - recv_icmp_ttl_exceeded - recv_icmp_packet - recv_unicast_packet - recv_bcast_packet - interface_tx - generate_vis_packet - broadcast_vis_packet - unicast_vis_packet
-> for example all recv_ functions are called with disabled irqs. So we must use irqsave
So it is complete correct to use irqsave everywhere (as you have commited it now).
Best regards, Sven