Re: [B.A.T.M.A.N.] [PATCHv2 no pgp] batman-adv: receive packets directly using skbs

[Sven Eckelmann <sven.eckelmann@gmx.de>]

[-- Attachment #1: Type: text/plain, Size: 4459 bytes --]

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 835 bytes --]