[openib-general] Re: [Andrew Morton] inappropriate use of in_atomic()

[Michael S. Tsirkin]

Quoting r. Roland Dreier <roland at topspin.com>:
> Subject: [Andrew Morton] inappropriate use of in_atomic()
> 
> drivers/infiniband/core/mad.c is in Andrew's list...
> 
> >From looking at the code, the best fix I can come up with is just to
> always use GFP_ATOMIC ... worst case we drop a MAD under memory
> pressure.  The other option is to change ib_post_send_mad() to take a
> GFP_ mask as a parameter, but that doesn't seem worth doing...
> 
> --- infiniband/core/mad.c	(revision 1975)
> +++ infiniband/core/mad.c	(working copy)
> @@ -666,11 +666,7 @@ static int handle_outgoing_dr_smp(struct
>  	if (!ret || !device->process_mad)
>  		goto out;
>  
> -	if (in_atomic() || irqs_disabled())
> -		alloc_flags = GFP_ATOMIC;
> -	else
> -		alloc_flags = GFP_KERNEL;
> -	local = kmalloc(sizeof *local, alloc_flags);
> +	local = kmalloc(sizeof *local, GFP_ATOMIC);
>  	if (!local) {
>  		ret = -ENOMEM;
>  		printk(KERN_ERR PFX "No memory for ib_mad_local_private\n");
> @@ -860,9 +856,7 @@ int ib_post_send_mad(struct ib_mad_agent
>  		}
>  
>  		/* Allocate MAD send WR tracking structure */
> -		mad_send_wr = kmalloc(sizeof *mad_send_wr,
> -				      (in_atomic() || irqs_disabled()) ?
> -				      GFP_ATOMIC : GFP_KERNEL);
> +		mad_send_wr = kmalloc(sizeof *mad_send_wr, GFP_ATOMIC);
>  		if (!mad_send_wr) {
>  			printk(KERN_ERR PFX "No memory for "
>  			       "ib_mad_send_wr_private\n");
> 
> 
> 
> 
> 
> Date: Thu, 10 Mar 2005 20:40:06 -0800
> From: Andrew Morton <akpm at osdl.org>
> Subject: inappropriate use of in_atomic()
> 
> 
> in_atomic() is not a reliable indication of whether it is currently safe
> to call schedule().
> 
> This is because the lockdepth beancounting which in_atomic() uses is only
> accumulated if CONFIG_PREEMPT=y.  in_atomic() will return false inside
> spinlocks if CONFIG_PREEMPT=n.
> 
> Consequently the use of in_atomic() in the below files is probably
> deadlocky if CONFIG_PREEMPT=n:
> 
> 	arch/ppc64/kernel/viopath.c
> 	drivers/net/irda/sir_kthread.c
> 	drivers/net/wireless/airo.c
> 	drivers/video/amba-clcd.c
> 	drivers/acpi/osl.c
> 	drivers/ieee1394/ieee1394_transactions.c
> 	drivers/infiniband/core/mad.c
> 
> Note that the same beancounting is used for the "scheduling while atomic"
> warning, so if the code calls schedule with locks held, we won't get a
> warning.  Both are tied to CONFIG_PREEMPT=y.
> 
> The kernel provides no reliable runtime way of detecting whether or not it
> is safe to call schedule().
> 
> Can we please find ways to change the above code to not use in_atomic()? 
> Then we can whack #ifndef MODULE around its definition to reduce
> reoccurrences.  Will probably rename it to something more scary as well.
> 
> Thanks.
> 

Sdp also has a couple of uses.
Maybe we can use the atomic branch in all cases here, as well?
Libor?

-- 
MST - Michael S. Tsirkin