Sleeping function called from illegal context...

Sleeping function called from illegal context...

[Greg KH]

So I got bold and enabled CONFIG_PREEMPT in 2.5.39, and got the
following message at boot time:

Sleeping function called from illegal context at slab.c:1374
c12a5ea8 c0117a26 c0296260 c0298202 0000055e c1283060 c013ab4f c0298202 
       0000055e c03b668c 00000002 00000003 c01ff5ec c03b668c 00000042 c12838e0 
       c12838e0 c12838e0 00000246 cfdee214 c0207830 04000000 c03b65f0 c0109be2 
Call Trace:
 [<c0117a26>]__might_sleep+0x56/0x5d
 [<c013ab4f>]kmalloc+0x4f/0x330
 [<c01ff5ec>]piix_tune_chipset+0x33c/0x350
 [<c0207830>]ide_intr+0x0/0x320
 [<c0109be2>]request_irq+0x52/0xa0
 [<c0200a33>]init_irq+0x263/0x400
 [<c0207830>]ide_intr+0x0/0x320
 [<c0200edc>]hwif_init+0x10c/0x260
 [<c02006ad>]probe_hwif_init+0x1d/0x70
 [<c02121d1>]ide_setup_pci_device+0x41/0x70
 [<c01ff7a5>]piix_init_one+0x35/0x40
 [<c010511b>]init+0x8b/0x250
 [<c0105090>]init+0x0/0x250
 [<c01055f9>]kernel_thread_helper+0x5/0xc

kksymoops is very nice :)

The system still seems to be running ok, but I think I'll turn off
CONFIG_PREEMPT just to be sure.

This is a SMP kernel running on a UP box.  Other configuration options
available if needed.

thanks,

greg k-h

Re: Sleeping function called from illegal context...

[Andrew Morton]

Greg KH wrote:
> 
> So I got bold and enabled CONFIG_PREEMPT in 2.5.39, and got the
> following message at boot time:
> 
> Sleeping function called from illegal context at slab.c:1374
> c12a5ea8 c0117a26 c0296260 c0298202 0000055e c1283060 c013ab4f c0298202
>        0000055e c03b668c 00000002 00000003 c01ff5ec c03b668c 00000042 c12838e0
>        c12838e0 c12838e0 00000246 cfdee214 c0207830 04000000 c03b65f0 c0109be2
> Call Trace:
>  [<c0117a26>]__might_sleep+0x56/0x5d
>  [<c013ab4f>]kmalloc+0x4f/0x330
>  [<c01ff5ec>]piix_tune_chipset+0x33c/0x350
>  [<c0207830>]ide_intr+0x0/0x320
>  [<c0109be2>]request_irq+0x52/0xa0
>  [<c0200a33>]init_irq+0x263/0x400
>  [<c0207830>]ide_intr+0x0/0x320
>  [<c0200edc>]hwif_init+0x10c/0x260
>  [<c02006ad>]probe_hwif_init+0x1d/0x70
>  [<c02121d1>]ide_setup_pci_device+0x41/0x70
>  [<c01ff7a5>]piix_init_one+0x35/0x40
>  [<c010511b>]init+0x8b/0x250
>  [<c0105090>]init+0x0/0x250
>  [<c01055f9>]kernel_thread_helper+0x5/0xc
> 

Everyone will get this.  It's IDE's init_irq() function doing
unsafe things inside ide_lock.

It'll be quite harmless at boot-time, but it'd be nice to get
it fixed up.

Re: (more) Sleeping function called from illegal context...

[Luc Van Oostenryck]

With CONFIG_PREEMPT=y on an SMP AMD (2CPU):

Sleeping function called from illegal context at /kernel/l-2.5.39/include/asm/semaphore.h:119
c1b75e20 c0117094 c0280b00 c028d480 00000077 c1b41cf0 c016bcf9 c028d480
        00000077 c1b41c50 ffffffea c1b74000 00001000 c01937c6 c02e9578 c1b41cf0
        c1b41c50 f7f0b840 c1b75eda c018ad0a c1b41c50 c02e9578 00000020 00000000
Call Trace:
  [<c0117094>]__might_sleep+0x54/0x58
  [<c016bcf9>]driverfs_create_file+0x39/0xa0
  [<c01937c6>]device_create_file+0x26/0x40
  [<c018ad0a>]pci_pool_create+0xea/0x170
  [<c02034af>]hcd_buffer_create+0x3f/0x80
  [<c02039c3>]usb_hcd_pci_probe+0x253/0x370
  [<c01563d8>]alloc_inode+0x58/0x190
  [<c018b751>]pci_device_probe+0x41/0x60
  [<c0191b88>]probe+0x18/0x30
  [<c0191c2b>]found_match+0x2b/0x60
  [<c0191d57>]do_driver_attach+0x37/0x50
  [<c019296c>]bus_for_each_dev+0x9c/0x130
  [<c0191d83>]driver_attach+0x13/0x20
  [<c0191d20>]do_driver_attach+0x0/0x50
  [<c0192ee4>]driver_register+0x94/0xb0
  [<c018b856>]pci_register_driver+0x36/0x50
  [<c01050b7>]init+0x47/0x1c0
  [<c0105070>]init+0x0/0x1c0
  [<c010553d>]kernel_thread_helper+0x5/0x18

Re: (more) Sleeping function called from illegal context...

[Andrew Morton]

Luc Van Oostenryck wrote:
> 
> With CONFIG_PREEMPT=y on an SMP AMD (2CPU):
> 
> Sleeping function called from illegal context at /kernel/l-2.5.39/include/asm/semaphore.h:119
> c1b75e20 c0117094 c0280b00 c028d480 00000077 c1b41cf0 c016bcf9 c028d480
>         00000077 c1b41c50 ffffffea c1b74000 00001000 c01937c6 c02e9578 c1b41cf0
>         c1b41c50 f7f0b840 c1b75eda c018ad0a c1b41c50 c02e9578 00000020 00000000
> Call Trace:
>   [<c0117094>]__might_sleep+0x54/0x58
>   [<c016bcf9>]driverfs_create_file+0x39/0xa0
>   [<c01937c6>]device_create_file+0x26/0x40
>   [<c018ad0a>]pci_pool_create+0xea/0x170
>   [<c02034af>]hcd_buffer_create+0x3f/0x80
>   [<c02039c3>]usb_hcd_pci_probe+0x253/0x370
>   [<c01563d8>]alloc_inode+0x58/0x190
>   [<c018b751>]pci_device_probe+0x41/0x60
>   [<c0191b88>]probe+0x18/0x30
>   [<c0191c2b>]found_match+0x2b/0x60
>   [<c0191d57>]do_driver_attach+0x37/0x50
>   [<c019296c>]bus_for_each_dev+0x9c/0x130
>   [<c0191d83>]driver_attach+0x13/0x20
>   [<c0191d20>]do_driver_attach+0x0/0x50
>   [<c0192ee4>]driver_register+0x94/0xb0
>   [<c018b856>]pci_register_driver+0x36/0x50
>   [<c01050b7>]init+0x47/0x1c0
>   [<c0105070>]init+0x0/0x1c0
>   [<c010553d>]kernel_thread_helper+0x5/0x18
> 

pci_pool_create() is calling device_create_file() under pools_lock.

Re: (more) Sleeping function called from illegal context...

[Luc Van Oostenryck]

With CONFIG_PREEMPT=y on an SMP AMD (2CPU):

Sleeping function called from illegal context at /kernel/l-2.5.39/include/asm/semaphore.h:119
c1b4ff7c c0117094 c0280b00 c02bc680 00000077 f7f78540 c01ffc8c c02bc680
        00000077 c1b4e000 c1b4e000 00000001 c1b4ffdc c1b4ffc0 00000206 f7f78568
        c1b4e000 00000001 c1b4ffdc c01fff35 c01fff00 00000000 00000000 00000000
Call Trace:
  [<c0117094>]__might_sleep+0x54/0x58
  [<c01ffc8c>]usb_hub_events+0x6c/0x2e0
  [<c01fff35>]usb_hub_thread+0x35/0xe0
  [<c01fff00>]usb_hub_thread+0x0/0xe0
  [<c0115500>]default_wake_function+0x0/0x40
  [<c010553d>]kernel_thread_helper+0x5/0x18

Re: (more) Sleeping function called from illegal context...

[Andrew Morton]

Luc Van Oostenryck wrote:
> 
> With CONFIG_PREEMPT=y on an SMP AMD (2CPU):
> 
> Sleeping function called from illegal context at /kernel/l-2.5.39/include/asm/semaphore.h:119
> c1b4ff7c c0117094 c0280b00 c02bc680 00000077 f7f78540 c01ffc8c c02bc680
>         00000077 c1b4e000 c1b4e000 00000001 c1b4ffdc c1b4ffc0 00000206 f7f78568
>         c1b4e000 00000001 c1b4ffdc c01fff35 c01fff00 00000000 00000000 00000000
> Call Trace:
>   [<c0117094>]__might_sleep+0x54/0x58
>   [<c01ffc8c>]usb_hub_events+0x6c/0x2e0
>   [<c01fff35>]usb_hub_thread+0x35/0xe0
>   [<c01fff00>]usb_hub_thread+0x0/0xe0
>   [<c0115500>]default_wake_function+0x0/0x40
>   [<c010553d>]kernel_thread_helper+0x5/0x18

usb_hub_events() does down() inside hub_event_lock.

Re: (more) Sleeping function called from illegal context...

[Greg KH]

On Fri, Sep 27, 2002 at 06:24:07PM -0700, Andrew Morton wrote:
> Luc Van Oostenryck wrote:
> > 
> > With CONFIG_PREEMPT=y on an SMP AMD (2CPU):
> > 
> > Sleeping function called from illegal context at /kernel/l-2.5.39/include/asm/semaphore.h:119
> > c1b4ff7c c0117094 c0280b00 c02bc680 00000077 f7f78540 c01ffc8c c02bc680
> >         00000077 c1b4e000 c1b4e000 00000001 c1b4ffdc c1b4ffc0 00000206 f7f78568
> >         c1b4e000 00000001 c1b4ffdc c01fff35 c01fff00 00000000 00000000 00000000
> > Call Trace:
> >   [<c0117094>]__might_sleep+0x54/0x58
> >   [<c01ffc8c>]usb_hub_events+0x6c/0x2e0
> >   [<c01fff35>]usb_hub_thread+0x35/0xe0
> >   [<c01fff00>]usb_hub_thread+0x0/0xe0
> >   [<c0115500>]default_wake_function+0x0/0x40
> >   [<c010553d>]kernel_thread_helper+0x5/0x18
> 
> usb_hub_events() does down() inside hub_event_lock.

Yup, just got that one myself, along with a few other USB goodies :(

This is a very good debugging tool, thanks for doing it.

greg k-h

Re: (more) Sleeping function called from illegal context...

[Luc Van Oostenryck]

With CONFIG_PREEMPT=y on an SMP AMD (2CPU):

Sleeping function called from illegal context at slab.c:1374
f79dbe2c c0117094 c0280b00 c02847cb 0000055e f7817b40 c01328fd c02847cb
        0000055e f79da000 f7ede980 c03ecc58 f7817b40 c025af91 c18cf248 c0266b3e
        00000024 000001d0 f79da000 00000286 00000001 bffffc9c c02f63e0 c0266e20
Call Trace:
  [<c0117094>]__might_sleep+0x54/0x58
  [<c01328fd>]kmalloc+0x5d/0x1e0
  [<c025af91>]fib_add_ifaddr+0x61/0x110
  [<c0266b3e>]__sctp_get_local_addr_list+0x9e/0x140
  [<c0266e20>]sctp_netdev_event+0x30/0x60
  [<c01241ae>]notifier_call_chain+0x1e/0x40
  [<c02566f5>]inet_insert_ifa+0x1b5/0x1c0
  [<c02567b4>]inet_set_ifa+0xb4/0xc0
  [<c0257091>]devinet_ioctl+0x511/0x740
  [<c0259897>]inet_ioctl+0x157/0x1b0
  [<c021e276>]sock_ioctl+0x56/0x90
  [<c0150039>]sys_ioctl+0x289/0x2d8
  [<c0107d11>]error_code+0x2d/0x38
  [<c01072cf>]syscall_call+0x7/0xb

Re: (more) Sleeping function called from illegal context...

[Andrew Morton]

Luc Van Oostenryck wrote:
> 
> With CONFIG_PREEMPT=y on an SMP AMD (2CPU):
> 
> Sleeping function called from illegal context at slab.c:1374
> f79dbe2c c0117094 c0280b00 c02847cb 0000055e f7817b40 c01328fd c02847cb
>         0000055e f79da000 f7ede980 c03ecc58 f7817b40 c025af91 c18cf248 c0266b3e
>         00000024 000001d0 f79da000 00000286 00000001 bffffc9c c02f63e0 c0266e20
> Call Trace:
>   [<c0117094>]__might_sleep+0x54/0x58
>   [<c01328fd>]kmalloc+0x5d/0x1e0
>   [<c025af91>]fib_add_ifaddr+0x61/0x110
>   [<c0266b3e>]__sctp_get_local_addr_list+0x9e/0x140
>   [<c0266e20>]sctp_netdev_event+0x30/0x60
>   [<c01241ae>]notifier_call_chain+0x1e/0x40
>   [<c02566f5>]inet_insert_ifa+0x1b5/0x1c0
>   [<c02567b4>]inet_set_ifa+0xb4/0xc0
>   [<c0257091>]devinet_ioctl+0x511/0x740
>   [<c0259897>]inet_ioctl+0x157/0x1b0
>   [<c021e276>]sock_ioctl+0x56/0x90
>   [<c0150039>]sys_ioctl+0x289/0x2d8
>   [<c0107d11>]error_code+0x2d/0x38
>   [<c01072cf>]syscall_call+0x7/0xb
> 

sctp_v4_get_local_addr_list():

                /* XXX BUG: sleeping allocation with lock held -DaveM */
                addr = t_new(struct sockaddr_storage_list, GFP_KERNEL);

Is true.  We're holding dev_base_lock, inetdev_lock and in_dev->lock
here.

Re: [Lksctp-developers] Re: (more) Sleeping function called from illegal context...

[Jon Grimm]

Thanks.  We'll fix it up.

Best Regards,
Jon Grimm

Andrew Morton wrote:
> 

> 
> sctp_v4_get_local_addr_list():
> 
>                 /* XXX BUG: sleeping allocation with lock held -DaveM */
>                 addr = t_new(struct sockaddr_storage_list, GFP_KERNEL);
> 
> Is true.  We're holding dev_base_lock, inetdev_lock and in_dev->lock
> here.
>

Re: [Lksctp-developers] Re: (more) Sleeping function called from illegal context...

[Jon Grimm]

See Patch below to stop any bleeding.   I'll feed the patch in through
DaveM.  

Thanks, Jon  

Andrew Morton wrote:
> 
> Luc Van Oostenryck wrote:
> >

> >
> 
> sctp_v4_get_local_addr_list():
> 
>                 /* XXX BUG: sleeping allocation with lock held -DaveM */
>                 addr = t_new(struct sockaddr_storage_list, GFP_KERNEL);
> 
> Is true.  We're holding dev_base_lock, inetdev_lock and in_dev->lock
> here.

# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or
higher.
# This patch includes the following deltas:
#	           ChangeSet	1.655   -> 1.656  
#	 net/sctp/protocol.c	1.7     -> 1.8    
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 02/09/30	jgrimm@touki.austin.ibm.com	1.656
# sctp:  Fix GFP_KERNEL allocation with lock held. 
# --------------------------------------------
#
diff -Nru a/net/sctp/protocol.c b/net/sctp/protocol.c
--- a/net/sctp/protocol.c	Mon Sep 30 08:20:55 2002
+++ b/net/sctp/protocol.c	Mon Sep 30 08:20:55 2002
@@ -119,8 +119,7 @@
 
 	for (ifa = in_dev->ifa_list; ifa; ifa = ifa->ifa_next) {
 		/* Add the address to the local list.  */
-		/* XXX BUG: sleeping allocation with lock held -DaveM */
-		addr = t_new(struct sockaddr_storage_list, GFP_KERNEL);
+		addr = t_new(struct sockaddr_storage_list, GFP_ATOMIC);
 		if (addr) {
 			INIT_LIST_HEAD(&addr->list);
 			addr->a.v4.sin_family = AF_INET;
@@ -157,8 +156,7 @@
 	read_lock_bh(&in6_dev->lock);
 	for (ifp = in6_dev->addr_list; ifp; ifp = ifp->if_next) {
 		/* Add the address to the local list.  */
-		/* XXX BUG: sleeping allocation with lock held -DaveM */
-		addr = t_new(struct sockaddr_storage_list, GFP_KERNEL);
+		addr = t_new(struct sockaddr_storage_list, GFP_ATOMIC);
 		if (addr) {
 			addr->a.v6.sin6_family = AF_INET6;
 			addr->a.v6.sin6_port = 0;

Re: Sleeping function called from illegal context...

[Andre Hedrick]

No SLEEPING under a KMALLOC on a spinlock while calling request_irq is not
the brightest thing ever done!

So your finger pointing stinks!

See in 2.4-ac there is a stub to indicate the brokeness.

int init_irq (ide_hwif_t *hwif)
{
        unsigned long flags;
        unsigned int index;
        ide_hwgroup_t *hwgroup, *new_hwgroup;
        ide_hwif_t *match = NULL;

#if 0
        /* Allocate the buffer and no sleep allowed */
        new_hwgroup = kmalloc(sizeof(ide_hwgroup_t),GFP_ATOMIC);
#else
        /* Allocate the buffer and potentially sleep first */
        new_hwgroup = kmalloc(sizeof(ide_hwgroup_t),GFP_KERNEL);
#endif

#ifndef __IRQ_HELL_SPIN
        save_and_cli(flags);
#else
        spin_lock_irqsave(&io_request_lock, flags);
#endif

<snip>

                if (request_irq(hwif->irq,&ide_intr,sa,hwif->name,hwgroup)) {
                        if (!match)
                                kfree(hwgroup);
#ifndef __IRQ_HELL_SPIN
                        restore_flags(flags);
#else
                        spin_unlock_irqrestore(&io_request_lock, flags);
#endif
                        return 1;
                }

<snip>

        /* all CPUs; safe now that hwif->hwgroup is set up */
#ifndef __IRQ_HELL_SPIN
        restore_flags(flags);
#else
        spin_unlock_irqrestore(&io_request_lock, flags);
#endif


See in trying to move to a spinlock it goes totally south.
So now that you know the where, and why ... please go fix.
See I am off working with AC on the issues for 2.4.

Also with PREMPT, bah never mind.

Regards,

Andre Hedrick
LAD Storage Consulting Group

On Fri, 27 Sep 2002, Andrew Morton wrote:

> Greg KH wrote:
> > 
> > So I got bold and enabled CONFIG_PREEMPT in 2.5.39, and got the
> > following message at boot time:
> > 
> > Sleeping function called from illegal context at slab.c:1374
> > c12a5ea8 c0117a26 c0296260 c0298202 0000055e c1283060 c013ab4f c0298202
> >        0000055e c03b668c 00000002 00000003 c01ff5ec c03b668c 00000042 c12838e0
> >        c12838e0 c12838e0 00000246 cfdee214 c0207830 04000000 c03b65f0 c0109be2
> > Call Trace:
> >  [<c0117a26>]__might_sleep+0x56/0x5d
> >  [<c013ab4f>]kmalloc+0x4f/0x330
> >  [<c01ff5ec>]piix_tune_chipset+0x33c/0x350
> >  [<c0207830>]ide_intr+0x0/0x320
> >  [<c0109be2>]request_irq+0x52/0xa0
> >  [<c0200a33>]init_irq+0x263/0x400
> >  [<c0207830>]ide_intr+0x0/0x320
> >  [<c0200edc>]hwif_init+0x10c/0x260
> >  [<c02006ad>]probe_hwif_init+0x1d/0x70
> >  [<c02121d1>]ide_setup_pci_device+0x41/0x70
> >  [<c01ff7a5>]piix_init_one+0x35/0x40
> >  [<c010511b>]init+0x8b/0x250
> >  [<c0105090>]init+0x0/0x250
> >  [<c01055f9>]kernel_thread_helper+0x5/0xc
> > 
> 
> Everyone will get this.  It's IDE's init_irq() function doing
> unsafe things inside ide_lock.
> 
> It'll be quite harmless at boot-time, but it'd be nice to get
> it fixed up.
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: Sleeping function called from illegal context...

[Robert Love]

On Fri, 2002-09-27 at 22:04, Andre Hedrick wrote:

> See in trying to move to a spinlock it goes totally south.
> So now that you know the where, and why ... please go fix.
> See I am off working with AC on the issues for 2.4.
> 
> Also with PREMPT, bah never mind.

Sigh... I do not want to start this but this problem has nothing to do
with preemption and everything to do with you sleeping while holding a
lock.  It exists whether preempt is on or off.

	Robert Love

Re: Sleeping function called from illegal context...

[Andre Hedrick]

On 27 Sep 2002, Robert Love wrote:

> On Fri, 2002-09-27 at 22:04, Andre Hedrick wrote:
> 
> > See in trying to move to a spinlock it goes totally south.
> > So now that you know the where, and why ... please go fix.
> > See I am off working with AC on the issues for 2.4.
> > 
> > Also with PREMPT, bah never mind.
> 
> Sigh... I do not want to start this but this problem has nothing to do
> with preemption and everything to do with you sleeping while holding a
> lock.  It exists whether preempt is on or off.

Robert,

Glad we agree on the lock issue, thanks for confirming the point!
There is an issue of interrupt acknowledgement and when one can pre-empt.
I would like to resolve the issue, but I need a global caller/notifier api
from you so I can block IO in a safe spot on the 'data transfer' state
bar.  Yeah, blah blah on underfined terms.

Some how I need to figure out how to address the pre-empt and keep the
driver data stable.  Initially I would suggest throttling back on the
request size to maybe 4k or 8k regardless.  I may not sound right but it
will serve the purpose.

Cheers,

Andre Hedrick
LAD Storage Consulting Group

Re: Sleeping function called from illegal context...

[Robert Love]

On Fri, 2002-09-27 at 23:21, Andre Hedrick wrote:

> Glad we agree on the lock issue, thanks for confirming the point!

Great!

> There is an issue of interrupt acknowledgement and when one can pre-empt.
> I would like to resolve the issue, but I need a global caller/notifier api
> from you so I can block IO in a safe spot on the 'data transfer' state
> bar.  Yeah, blah blah on underfined terms.

Well, I do not know what the problem is (or what the hell you are
talking about, to be honest).  You really should not have any problems
with preemption over regular SMP issues.  If your code has a problem
with other code running concurrently, then it should already hold a lock
and thus be non-preemptive?

Also note we do not preempt interrupt handlers (obviously).

If you have a critical section in which you do not want to be preempted,
do a:

	preempt_disable();
	/* critical section ... */
	preempt_enable();

This would have to be code that is in user-context and does not already
hold a lock.  There are very few explicit places that need this.  You
would be the first block driver, I believe.

Whatever this issue is, note it is entirely separate from the above
locking issue.  I also want to iterate that the locking problem
(rescheduling while holding a lock) is a problem on UP even.  Yes, think
about it.  Assuming the lock really needs to be held, it is protecting a
critical region.  If we reschedule, we can enter that region (or another
one of the same data protected hopefully by the same lock).  On SMP, we
would deadlock.  But on UP we will just silently corrupt the data. 
I.e., we can race on UP here.

	Robert Love

Re: Sleeping function called from illegal context...

[Alan Cox]

On Sat, 2002-09-28 at 04:21, Andre Hedrick wrote:
> There is an issue of interrupt acknowledgement and when one can pre-empt.
> I would like to resolve the issue, but I need a global caller/notifier api
> from you so I can block IO in a safe spot on the 'data transfer' state
> bar.  Yeah, blah blah on underfined terms.
> 
> Some how I need to figure out how to address the pre-empt and keep the
> driver data stable.  Initially I would suggest throttling back on the
> request size to maybe 4k or 8k regardless.  I may not sound right but it
> will serve the purpose.

For things like old old broken PIO where interrupting the data stream
screws up the data thats actually already covered. Pre-empt does
actually do some things sensibly, and one of them is that when you hold
a lock or disable irq you also disable pre-empt. That means hdparm -u0
PIO interface code is still going to do the right thing

Reminds me though Robert (and Jeff)

drivers/net/8390.c still needs ei_start_xmit fixing

pre-emption should be disabled between

       /* Mask interrupts from the ethercard.
           SMP: We have to grab the lock here otherwise the IRQ handler

and
       disable_irq_nosync(dev->irq);

        spin_lock(&ei_local->page_lock);


So that we don't leave the IRQ disabled due to pre-emption

(that code is wonderfully deranged but its the only way to make 8390
based chips not screw up things like serial I/O on a SMP box)

Re: Sleeping function called from illegal context...

[Robert Love]

On Sat, 2002-09-28 at 06:06, Alan Cox wrote:

> Reminds me though Robert (and Jeff)
> 
> drivers/net/8390.c still needs ei_start_xmit fixing
> 
> pre-emption should be disabled between
> 
>        /* Mask interrupts from the ethercard.
>            SMP: We have to grab the lock here otherwise the IRQ handler
> 
> and
>        disable_irq_nosync(dev->irq);
> 
>         spin_lock(&ei_local->page_lock);

Sounds reasonable enough.  What about the attached patch?  If we flip
the order of the disable_irq and spin_lock, we do not need to actually
explicitly disable preemption... the lock will do that for us.

Is this safe?

This also has the general benefit of not spinning on the lock with the
irq disabled (same sort of downside has preempting with the irq
disabled).

	Robert Love

diff -urN linux-2.5.39/drivers/net/8390.c linux/drivers/net/8390.c
--- linux-2.5.39/drivers/net/8390.c	Fri Sep 27 17:49:05 2002
+++ linux/drivers/net/8390.c	Sat Sep 28 13:02:47 2002
@@ -243,9 +243,9 @@
 	}
 
 	/* Ugly but a reset can be slow, yet must be protected */
-		
-	disable_irq_nosync(dev->irq);
+
 	spin_lock(&ei_local->page_lock);
+	disable_irq_nosync(dev->irq);
 		
 	/* Try to restart the card.  Perhaps the user has fixed something. */
 	ei_reset_8390(dev);
@@ -286,10 +286,9 @@
 	/*
 	 *	Slow phase with lock held.
 	 */
-	 
-	disable_irq_nosync(dev->irq);
-	
+
 	spin_lock(&ei_local->page_lock);
+	disable_irq_nosync(dev->irq);
 	
 	ei_local->irqlock = 1;
 

Re: Sleeping function called from illegal context...

[Robert Love]

On Fri, 2002-09-27 at 19:30, Greg KH wrote:

> The system still seems to be running ok, but I think I'll turn off
> CONFIG_PREEMPT just to be sure.

Note this has nothing to do with kernel preemption.  IDE explicitly
sleeps while purposely holding a lock.

It is just we do not have the ability to measure atomicity w/o
preemption enabled - e.g. the debugging only works when it is enabled.

	Robert Love

Re: Sleeping function called from illegal context...

[Greg KH]

On Fri, Sep 27, 2002 at 08:51:30PM -0400, Robert Love wrote:
> On Fri, 2002-09-27 at 19:30, Greg KH wrote:
> 
> > The system still seems to be running ok, but I think I'll turn off
> > CONFIG_PREEMPT just to be sure.
> 
> Note this has nothing to do with kernel preemption.  IDE explicitly
> sleeps while purposely holding a lock.
> 
> It is just we do not have the ability to measure atomicity w/o
> preemption enabled - e.g. the debugging only works when it is enabled.

Yes, you are correct.  Sorry for stating that.  It's shaking out lots of
potential proplems :)

greg k-h

Re: Sleeping function called from illegal context...

[John Levon]

On Fri, Sep 27, 2002 at 08:51:30PM -0400, Robert Love wrote:

> Note this has nothing to do with kernel preemption.  IDE explicitly
> sleeps while purposely holding a lock.
> 
> It is just we do not have the ability to measure atomicity w/o
> preemption enabled - e.g. the debugging only works when it is enabled.

Would it be particularly difficult to separate this debug tool from the
feature ? Surely we could make it so that CONFIG_PREEMPT depends on
CONFIG_MIGHT_SLEEP or whatever, and just adds the actual ability to
reschedule.

I have a bit of a problem with __might_sleep because I call sleepable
stuff holding a spinlock (yes, it is justified, and yes, it is safe
afaics, at least with PREEMPT=n)

regards
john

-- 
"When your name is Winner, that's it. You don't need a nickname."
	- Loser Lane

Re: Sleeping function called from illegal context...

[Andrew Morton]

John Levon wrote:
> 
> On Fri, Sep 27, 2002 at 08:51:30PM -0400, Robert Love wrote:
> 
> > Note this has nothing to do with kernel preemption.  IDE explicitly
> > sleeps while purposely holding a lock.
> >
> > It is just we do not have the ability to measure atomicity w/o
> > preemption enabled - e.g. the debugging only works when it is enabled.
> 
> Would it be particularly difficult to separate this debug tool from the
> feature ? Surely we could make it so that CONFIG_PREEMPT depends on
> CONFIG_MIGHT_SLEEP or whatever, and just adds the actual ability to
> reschedule.

We need a standalone CONFIG_MIGHT_SLEEP.  I sinfully hooked it
to CONFIG_DEBUG_KERNEL (it's not obvious why CONFIG_DEBUG_KERNEL
exists actually).

So yes, you could make CONFIG_MIGHT_SLEEP mutually exclusive
with CONFIG_OPROFILE. But that would make people look at you
suspiciously.

> I have a bit of a problem with __might_sleep because I call sleepable
> stuff holding a spinlock (yes, it is justified, and yes, it is safe
> afaics, at least with PREEMPT=n)

I'm looking at you suspiciously.  How come?

Re: Sleeping function called from illegal context...

[John Levon]

On Sat, Sep 28, 2002 at 10:05:17AM -0700, Andrew Morton wrote:

> We need a standalone CONFIG_MIGHT_SLEEP.  I sinfully hooked it
> to CONFIG_DEBUG_KERNEL (it's not obvious why CONFIG_DEBUG_KERNEL
> exists actually).

Ah, OK.

> > I have a bit of a problem with __might_sleep because I call sleepable
> > stuff holding a spinlock (yes, it is justified, and yes, it is safe
> > afaics, at least with PREEMPT=n)
> 
> I'm looking at you suspiciously.  How come?

NMI interrupt handler cannot block so it trylocks against a spinlock
instead. Buffer processing code needs to block against concurrent NMI
interrupts so takes the spinlock for them. All actual blocks on the
spinlock are beneath a down() on another semaphore, so a sleep whilst
holding the spinlock won't actually cause deadlock.

I don't know a way out of this that can safely ensure we've finished
processing an NMI on the remote CPU the buffer processing code is about
to look at.

[I'll post a new patch against 2.5.39 in a bit so you can see in
context]

regards
john

-- 
"When your name is Winner, that's it. You don't need a nickname."
	- Loser Lane

Re: Sleeping function called from illegal context...

[Robert Love]

On Sat, 2002-09-28 at 13:24, John Levon wrote:

> NMI interrupt handler cannot block so it trylocks against a spinlock
> instead. Buffer processing code needs to block against concurrent NMI
> interrupts so takes the spinlock for them. All actual blocks on the
> spinlock are beneath a down() on another semaphore, so a sleep whilst
> holding the spinlock won't actually cause deadlock.

If all accesses to the spinlock are taken under a semaphore, then the
spinlock is not needed (i.e. the down'ed semaphore provides the same
protection), or am I missing something?

If this is not the case - e.g. there are other accesses to these locks -
then you cannot sleep, no?

I really can think of no case in which it is safe to sleep while holding
a spinlock or otherwise atomic.  If it is, then the atomicity is not
needed, sort of by definition.

	Robert Love

Re: Sleeping function called from illegal context...

[John Levon]

On Sat, Sep 28, 2002 at 02:27:44PM -0400, Robert Love wrote:

> > NMI interrupt handler cannot block so it trylocks against a spinlock
> > instead. Buffer processing code needs to block against concurrent NMI
> > interrupts so takes the spinlock for them. All actual blocks on the
> > spinlock are beneath a down() on another semaphore, so a sleep whilst
> > holding the spinlock won't actually cause deadlock.
> 
> If all accesses to the spinlock are taken under a semaphore, then the
> spinlock is not needed (i.e. the down'ed semaphore provides the same
> protection), or am I missing something?
> 
> If this is not the case - e.g. there are other accesses to these locks -
> then you cannot sleep, no?

The other accessors are spin_trylock()ers, as I mentioned. They will not
block but they are not under the semaphore.

The spinlock cannot be a semaphore because NMI interrupts do not take to
kindly to up()

regards
john

-- 
"When your name is Winner, that's it. You don't need a nickname."
	- Loser Lane

Re: Sleeping function called from illegal context...

[William Lee Irwin III]

On Sat, 2002-09-28 at 13:24, John Levon wrote:
>> NMI interrupt handler cannot block so it trylocks against a spinlock
>> instead. Buffer processing code needs to block against concurrent NMI
>> interrupts so takes the spinlock for them. All actual blocks on the
>> spinlock are beneath a down() on another semaphore, so a sleep whilst
>> holding the spinlock won't actually cause deadlock.

On Sat, Sep 28, 2002 at 02:27:44PM -0400, Robert Love wrote:
> If all accesses to the spinlock are taken under a semaphore, then the
> spinlock is not needed (i.e. the down'ed semaphore provides the same
> protection), or am I missing something?
> If this is not the case - e.g. there are other accesses to these locks -
> then you cannot sleep, no?
> I really can think of no case in which it is safe to sleep while holding
> a spinlock or otherwise atomic.  If it is, then the atomicity is not
> needed, sort of by definition.

Actually, though he may be using a spinlock_t, when used this way, it
is not a spinlock, but rather a semaphore-like construct like PG_locked.
Spinlocks include blocking via busywait semantics, which this usage
does not have. It just happens to use the same data type. There are
other interesting abuses of spinlock-like constructs in "advanced"
locks, for instance, in non-sleeping handoff-scheduled queueing locks
(e.g. MCS spinlocks and rwlocks) it's a common idiom for one waiter to
set a "blocked" bit or lock word and then spin on it until another
waiter and/or cpu manipulating the lock clears it.


Bill