Re: [BUG?][2.6.25-mm1] sleeping during IRQ disabled

[Petr Tesarik <ptesarik@suse.cz>]

On Wed, 2008-05-07 at 09:57 +0900, Hidetoshi Seto wrote:
> Hi all,
> 
> Luck, Tony wrote:
> >> So it's definitely in mainline, and its definitely
> >> not Seto-san's patch.
> > 
> > Here's the root of the problem (arch/ia64/kernel/entry.S):
> > 
> Please see:
> 
>     841 GLOBAL_ENTRY(ia64_leave_kernel)
>     842         PT_REGS_UNWIND_INFO(0)
>     843         /*
>     844          * work.need_resched etc. mustn't get changed by this CPU before it returns to
>     845          * user- or fsys-mode, hence we disable interrupts early on.
>     846          *
>     847          * p6 controls whether current_thread_info()->flags needs to be check for
>     848          * extra work.  We always check for extra work when returning to user-level.
>     849          * With CONFIG_PREEMPT, we also check for extra work when the preempt_count
>     850          * is 0.  After extra work processing has been completed, execution
>     851          * resumes at .work_processed_syscall with p6 set to 1 if the extra-work-check
>     852          * needs to be redone.
>     853          */
>     854 #ifdef CONFIG_PREEMPT
>     855         rsm psr.i                               // disable interrupts
>     856         cmp.eq p0,pLvSys=r0,r0                  // pLvSys=0: leave from
>     856 kernel
>     857 (pKStk) adds r20=TI_PRE_COUNT+IA64_TASK_SIZE,r13
>     858         ;;
>     859         .pred.rel.mutex pUStk,pKStk
>     860 (pKStk) ld4 r21=[r20]                   // r21 <- preempt_count
>     861 (pUStk) mov r21=0                       // r21 <- 0
>     862         ;;
>     863         cmp.eq p6,p0=r21,r0             // p6 <- pUStk || (preempt_count
>     863  = 0)
>     864 #else
>     865 (pUStk) rsm psr.i
>     866         cmp.eq p0,pLvSys=r0,r0          // pLvSys=0: leave from kernel
>     867 (pUStk) cmp.eq.unc p6,p0=r0,r0          // p6 <- pUStk
>     868 #endif
>     869 .work_processed_kernel:
>     870         adds r17=TI_FLAGS+IA64_TASK_SIZE,r13
>     871         ;;
>     872 (p6)    ld4 r31=[r17]                           // load current_thread_i
>     872 nfo()->flags
>     873         adds r21=PT(PR)+16,r12
>     874         ;;
>     875
>     876         lfetch [r21],PT(CR_IPSR)-PT(PR)
>     877         adds r2=PT(B6)+16,r12
>     878         adds r3=PT(R16)+16,r12
>     879         ;;
>     880         lfetch [r21]
>     881         ld8 r28=[r2],8          // load b6
>     882         adds r29=PT(R24)+16,r12
>     883
>     884         ld8.fill r16=[r3],PT(AR_CSD)-PT(R16)
>     885         adds r30=PT(AR_CCV)+16,r12
>     886 (p6)    and r19=TIF_WORK_MASK,r31               // any work other than T
>     886 IF_SYSCALL_TRACE?
>     887         ;;
>     888         ld8.fill r24=[r29]
>     889         ld8 r15=[r30]           // load ar.ccv
>     890 (p6)    cmp4.ne.unc p6,p0=r19, r0               // any special work pend
>     890 ing?
>     891         ;;
>     892         ld8 r29=[r2],16         // load b7
>     893         ld8 r30=[r3],16         // load ar.csd
>     894 (p6)    br.cond.spnt .work_pending
> 
> and:
> 
>    1160 .work_pending_syscall:
>    1161         add r2=-8,r2
>    1162         add r3=-8,r3
>    1163         ;;
>    1164         st8 [r2]=r8
>    1165         st8 [r3]=r10
>    1166 .work_pending:
>    1167         tbit.z p6,p0=r31,TIF_NEED_RESCHED               // current_thread_info()->need_resched=0?
>    1168 (p6)    br.cond.sptk.few .notify
>    1169 #ifdef CONFIG_PREEMPT
>    1170 (pKStk) dep r21=-1,r0,PREEMPT_ACTIVE_BIT,1
>    1171         ;;
>    1172 (pKStk) st4 [r20]=r21
>    1173         ssm psr.i               // enable interrupts
>    1174 #endif
> >    1175         br.call.spnt.many rp=schedule
> >    1176 .ret9:  cmp.eq p6,p0=r0,r0                              // p6 <- 1
> >    1177         rsm psr.i               // disable interrupts
> >    1178         ;;
> >    1179 #ifdef CONFIG_PREEMPT
> >    1180 (pKStk) adds r20=TI_PRE_COUNT+IA64_TASK_SIZE,r13
> >    1181         ;;
> >    1182 (pKStk) st4 [r20]=r0            // preempt_count() <- 0
> >    1183 #endif
> >    1184 (pLvSys)br.cond.sptk.few  .work_pending_syscall_end
> >    1185         br.cond.sptk.many .work_processed_kernel        // re-check
> >    1186 
> >    1187 .notify:
> >    1188 (pUStk) br.call.spnt.many rp=notify_resume_user
> > 
> > on line 1188 we call notify_resume_user() with interrupts disabled (at
> > least if we fall through from the code above ... I didn't check the
> > state of interrupts if we branch to ".notify").
> 
> AFAIK, we always call notify_resume_user() with interrupts disabled.
> Is this right?
> 
> > So we start down this call chain to the might_sleep() check:
> > 
> >  [<a000000100011bf0>] show_stack+0x50/0xa0
> >  [<a000000100011c70>] dump_stack+0x30/0x60
> >  [<a000000100061a90>] __might_sleep+0x1f0/0x220
> >  [<a000000100709020>] down_read+0x20/0x60
> >  [<a0000001000fe060>] access_process_vm+0x60/0x2c0
> >  [<a00000010002a280>] ia64_sync_kernel_rbs+0x40/0x100
> >  [<a00000010002a400>] do_sync_rbs+0xc0/0x100
> >  [<a00000010000ac30>] unw_init_running+0x70/0xa0
> >  [<a00000010002a5c0>] ia64_sync_krbs+0x80/0xa0
> >  [<a000000100012690>] do_notify_resume_user+0x110/0x140
> >  [<a00000010000aac0>] notify_resume_user+0x40/0x60
> >  [<a00000010000a9f0>] skip_rbs_switch+0xe0/0x110
> >  [<a000000000010740>] __kernel_syscall_via_break+0x0/0x20
> 
> So, I think the problem is not "why interrupts are disabled," but
> "why sleep in this path which always with interrupts disabled."
> 
> It obviously means ia64_sync_kernel_rbs should care about that.
> The function was introduced by the following commit:
> 
> > commit 3b2ce0b17824c42bc2e46f7dd903b4acf5e9fff9
> > Author: Petr Tesarik <ptesarik@suse.cz>
> > Date:   Wed Dec 12 15:23:34 2007 +0100
> >
> >     [IA64] Synchronize kernel RSE to user-space and back
> 
> Hmm, could you make ia64_sync_kernel_rbs to safe with interrupts
> disabled, Petr?

No, the point of that function is to copy part of the kernel RBS to user
RBS. Accesses to user space are always allowed to sleep and there's
nothing I can do about it (without rewriting the whole memory management
in Linux from scratch). All I can do is to take the simpler approach
without TIF_RESTORE_RSE I proposed in the very beginning of the RSE sync
discussion, but which was then turned down, because Roland warned about
possible severe performance degradations.

The introduction of TIF_RESTORE_RSE was originally Shaohua's idea, so
maybe he knows how to do it properly.

BTW why must be interrupts disabled in this path? Would it be possible
to re-enable them for the duration of the synchronization, or does it
harm somehow?

Petr Tesarik