Prev: Slot allocation for execution out of line (XOL)
Next: [PATCH] intel_agp: Don't oops with zero stolen memory
From: Christoph Hellwig on 14 Jun 2010 14:00 On Mon, Jun 14, 2010 at 01:59:13PM +0530, Srikar Dronamraju wrote: > @@ -850,7 +850,19 @@ do_notify_resume(struct pt_regs *regs, void *unused, __u32 thread_info_flags) > > if (thread_info_flags & _TIF_UPROBE) { > clear_thread_flag(TIF_UPROBE); > +#ifdef CONFIG_X86_32 > + /* > + * On x86_32, do_notify_resume() gets called with > + * interrupts disabled. Hence enable interrupts if they > + * are still disabled. > + */ > + native_irq_enable(); > +#endif > uprobe_notify_resume(regs); > + > +#ifdef CONFIG_X86_32 > + native_irq_disable(); > +#endif I'm no x86 port guru, but this looks rather worriesome to me. Why does do_notify_resume have different calling conventions on 32 vs 64-bit? And if there is a good reason that 32-bit has them disabled, why is enabling them in the middle of do_notify_resume okay? > +void arch_uprobe_disable_sstep(struct pt_regs *regs) > +{ > + /* Disable single-stepping by clearing what we set */ > + clear_thread_flag(TIF_SINGLESTEP); > + clear_thread_flag(TIF_FORCED_TF); > + regs->flags &= ~X86_EFLAGS_TF; > +} This seems to have one layer of indentation too much. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Srikar Dronamraju on 15 Jun 2010 02:30 * Christoph Hellwig <hch(a)infradead.org> [2010-06-14 13:54:23]: > On Mon, Jun 14, 2010 at 01:59:13PM +0530, Srikar Dronamraju wrote: > > @@ -850,7 +850,19 @@ do_notify_resume(struct pt_regs *regs, void *unused, __u32 thread_info_flags) > > > > if (thread_info_flags & _TIF_UPROBE) { > > clear_thread_flag(TIF_UPROBE); > > +#ifdef CONFIG_X86_32 > > + /* > > + * On x86_32, do_notify_resume() gets called with > > + * interrupts disabled. Hence enable interrupts if they > > + * are still disabled. > > + */ > > + native_irq_enable(); > > +#endif > > uprobe_notify_resume(regs); > > + > > +#ifdef CONFIG_X86_32 > > + native_irq_disable(); > > +#endif > > I'm no x86 port guru, but this looks rather worriesome to me. Why does > do_notify_resume have different calling conventions on 32 vs 64-bit? > And if there is a good reason that 32-bit has them disabled, why is > enabling them in the middle of do_notify_resume okay? Thanks for bringing this up. I have no idea about why do_notify_resume() gets called with interrupts disabled in 32 bit. I would be happy to know the reason and rework based on inputs. I did query a few people about this but I havent got an answer on why we they are disabled on 32 bit and if its Okay to enable at this place. Ingo, Is there any person whom I could check with to get to know why the interrupts are disabled on x86_32? > > > +void arch_uprobe_disable_sstep(struct pt_regs *regs) > > +{ > > + /* Disable single-stepping by clearing what we set */ > > + clear_thread_flag(TIF_SINGLESTEP); > > + clear_thread_flag(TIF_FORCED_TF); > > + regs->flags &= ~X86_EFLAGS_TF; > > +} > > This seems to have one layer of indentation too much. Okay, I shall fix this in the next iteration. -- Thanks and Regards Srikar -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Oleg Nesterov on 15 Jun 2010 08:00 On 06/15, Srikar Dronamraju wrote: > > * Christoph Hellwig <hch(a)infradead.org> [2010-06-14 13:54:23]: > > > On Mon, Jun 14, 2010 at 01:59:13PM +0530, Srikar Dronamraju wrote: > > > @@ -850,7 +850,19 @@ do_notify_resume(struct pt_regs *regs, void *unused, __u32 thread_info_flags) > > > > > > if (thread_info_flags & _TIF_UPROBE) { > > > clear_thread_flag(TIF_UPROBE); > > > +#ifdef CONFIG_X86_32 > > > + /* > > > + * On x86_32, do_notify_resume() gets called with > > > + * interrupts disabled. Hence enable interrupts if they > > > + * are still disabled. > > > + */ > > > + native_irq_enable(); > > > +#endif > > > uprobe_notify_resume(regs); > > > + > > > +#ifdef CONFIG_X86_32 > > > + native_irq_disable(); > > > +#endif > > > > I'm no x86 port guru, but this looks rather worriesome to me. Why does > > do_notify_resume have different calling conventions on 32 vs 64-bit? > > And if there is a good reason that 32-bit has them disabled, why is > > enabling them in the middle of do_notify_resume okay? > > Thanks for bringing this up. I have no idea about why do_notify_resume() > gets called with interrupts disabled in 32 bit. Perhaps just because there is no reason to explicitly enable irqs? > I would be happy to know > the reason and rework based on inputs. I did query a few people about > this but I havent got an answer on why we they are disabled on 32 bit and > if its Okay to enable at this place. I think it is OK to enable interrupts. do_notify_resume() calls do_signal() which enables them anyway. But there is another question I already asked. Why the code uses native_irq_enable()? IIRC, you explained that local_irq_enable() doesn't work for unkown reason. This is strange, and imho should be explained. And I do not see a need to disable irqs again. Oleg. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Srikar Dronamraju on 15 Jun 2010 08:20 > > > > > > I'm no x86 port guru, but this looks rather worriesome to me. Why does > > > do_notify_resume have different calling conventions on 32 vs 64-bit? > > > And if there is a good reason that 32-bit has them disabled, why is > > > enabling them in the middle of do_notify_resume okay? > > > > Thanks for bringing this up. I have no idea about why do_notify_resume() > > gets called with interrupts disabled in 32 bit. > > Perhaps just because there is no reason to explicitly enable irqs? > > > I would be happy to know > > the reason and rework based on inputs. I did query a few people about > > this but I havent got an answer on why we they are disabled on 32 bit and > > if its Okay to enable at this place. > > I think it is OK to enable interrupts. do_notify_resume() calls do_signal() > which enables them anyway. > > But there is another question I already asked. Why the code uses > native_irq_enable()? IIRC, you explained that local_irq_enable() doesn't > work for unkown reason. This is strange, and imho should be explained. > local_irq_enable() translates to raw_local_irq_enable(). However raw_local_irq_enable on x86 seems to depend on CONFIG_PARAVIRT. On a machine, where CONFIG_PARAVIRT was defined, local_irq_enable translates to something other than native_irq_enable. It translates to PVOP_VCALLEE0(pv_irq_ops.irq_enable); Is it okay to use local_irq_enable() and then make CONFIG_UPROBES depend on !CONFIG_PARAVIRT? > And I do not see a need to disable irqs again. > > Oleg. > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Oleg Nesterov on 15 Jun 2010 09:30 On 06/15, Srikar Dronamraju wrote: > > > I think it is OK to enable interrupts. do_notify_resume() calls do_signal() > > which enables them anyway. > > > > But there is another question I already asked. Why the code uses > > native_irq_enable()? IIRC, you explained that local_irq_enable() doesn't > > work for unkown reason. This is strange, and imho should be explained. > > local_irq_enable() translates to raw_local_irq_enable(). > However raw_local_irq_enable on x86 seems to depend on CONFIG_PARAVIRT. > On a machine, where CONFIG_PARAVIRT was defined, local_irq_enable > translates to something other than native_irq_enable. > It translates to PVOP_VCALLEE0(pv_irq_ops.irq_enable); I see, and my question is why PVOP_VCALLEE0(pv_irq_ops.irq_enable) doesn't work ? If it doesn't here, why it works for other callers of local_irq_enable? I think we should ask paravirt developers. > Is it okay to use local_irq_enable() and then make CONFIG_UPROBES depend > on !CONFIG_PARAVIRT? I dunno, and I know nothing about paravirt. But please note that currently native_irq_enable has the only caller, raw_local_irq_enable(). It is really strange that do_notify_resume() has to use it, and it uses it to bypass the paravirt layer which perhaps can introduce other problems. Oleg. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
|
Next
|
Last
Pages: 1 2 Prev: Slot allocation for execution out of line (XOL) Next: [PATCH] intel_agp: Don't oops with zero stolen memory |