Re: [PATCH -v2 6/8] kexec jump: fix for lockdep
On Fri, 2008-08-08 at 12:13 +0200, Peter Zijlstra wrote: > On Fri, 2008-08-08 at 14:52 +0800, Huang Ying wrote: > > Replace local_irq_disable() with raw_local_irq_disable() to prevent > > lockdep complain. > Uhhm, please provide more information - just using raw_* to silence > lockdep is generally the wrong thing to do. In traditional kexec, the new kernel will replace current one, so the irq is simply disabled. But now jumping back from kexeced kernel is supported, so the irq should be enabled again. The code sequence of irq during kexec jump is as follow: local_irq_disable(); /* in kernel_kexec() */ local_irq_disable(); /* in machine_kexec() */ local_irq_enable(); /* in kernel_kexec() */ The disable and enable is not match. Maybe another method is to use local_irq_save(), local_irq_restore() pair in machine_kexec(), so the disable and enable is matched. Best Regrards, Huang Ying ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH -v2 3/8] kexec jump: check code size in control page
Hi, Vivek, On Fri, 2008-08-08 at 10:09 -0400, Vivek Goyal wrote: [...] > > --- a/arch/x86/kernel/relocate_kernel_32.S > > +++ b/arch/x86/kernel/relocate_kernel_32.S > > @@ -20,10 +20,11 @@ > > #define PAGE_ATTR (_PAGE_PRESENT | _PAGE_RW | _PAGE_ACCESSED | _PAGE_DIRTY) > > #define PAE_PGD_ATTR (_PAGE_PRESENT) > > > > -/* control_page + PAGE_SIZE/2 ~ control_page + PAGE_SIZE * 3/4 are > > - * used to save some data for jumping back > > +/* control_page + KEXEC_CONTROL_CODE_MAX_SIZE > > + * ~ control_page + PAGE_SIZE * 3/4 are used to save some data for > > + * jumping back > > */ > > Hi Huang, > > Above comment is not very clear. Can you please elaborate it. I thought > that PAGE_SIZE/2 is used for control code and rest half is shared between > kjump data and stack. What is PAGE_SIZE *3/4? Yes. Rest half is shared between kjump data and stack. I will change it. > > +++ b/arch/x86/kernel/vmlinux_check_32.lds.S > > @@ -0,0 +1,7 @@ > > +/* > > + * Link time checks > > + */ > > + > > +#include > > + > > +ASSERT(kexec_control_code_size <= KEXEC_CONTROL_CODE_MAX_SIZE, > "kexec control code size is too big") > > Will it make sense to move it into vmlinux_32.lds.S itself? Creating a > separate > file for a single check seems superfluous. I hope other ones can use it. But for now, put it in vmlinux_32.lds.S is better. I will change it. Best Regards, HUang Ying ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH -v2 7/8] kexec jump: ftrace_enabled_save/restore
Hi, Steven, On Fri, 2008-08-08 at 10:30 -0400, Steven Rostedt wrote: [...] > The only problem with this approach is what happens if the user changes > the enabled in between these two calls. This would make ftrace > inconsistent. > > I have a patch from the -rt tree that handles what you want. It is > attached below. Not sure how well it will apply to mainline. > > I really need to go through the rt patch set and start submitting a bunch > of clean-up/fixes to mainline. We've been meaning to do it, just have been > distracted :-( Your version is better in general sense. Thank you very much! But in this specific situation of kexec/kjump. The execution environment is that other CPUs are disabled, local irq is disabled, and it is not permitted to switch to other process. But it is safe and sufficient to use non-locked version here. So to satisfy both demands, I think it is better to provide both version, locked and non-locked. What do you think about that? Best Regards, Huang Ying ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH -v2 6/8] kexec jump: fix for lockdep
On Mon, 2008-08-11 at 08:59 +0800, Huang Ying wrote: > On Fri, 2008-08-08 at 12:13 +0200, Peter Zijlstra wrote: > > On Fri, 2008-08-08 at 14:52 +0800, Huang Ying wrote: > > > Replace local_irq_disable() with raw_local_irq_disable() to prevent > > > lockdep complain. > > Uhhm, please provide more information - just using raw_* to silence > > lockdep is generally the wrong thing to do. > > In traditional kexec, the new kernel will replace current one, so the > irq is simply disabled. But now jumping back from kexeced kernel is > supported, so the irq should be enabled again. > > The code sequence of irq during kexec jump is as follow: > > local_irq_disable(); /* in kernel_kexec() */ > local_irq_disable(); /* in machine_kexec() */ > local_irq_enable(); /* in kernel_kexec() */ > > The disable and enable is not match. Maybe another method is to use > local_irq_save(), local_irq_restore() pair in machine_kexec(), so the > disable and enable is matched. And its the machine kernel's lockdep instance that goes complain? whichever annotation gets used - and I think I can agree that raw_* might be approriate there, this should be accompanied with a rather elaborate changelog and preferably a comment in the code too. Without such we'll be wondering in the years to come WTH happens here. ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH -v2 6/8] kexec jump: fix for lockdep
On Mon, 2008-08-11 at 08:09 +0200, Peter Zijlstra wrote: > On Mon, 2008-08-11 at 08:59 +0800, Huang Ying wrote: > > On Fri, 2008-08-08 at 12:13 +0200, Peter Zijlstra wrote: > > > On Fri, 2008-08-08 at 14:52 +0800, Huang Ying wrote: > > > > Replace local_irq_disable() with raw_local_irq_disable() to prevent > > > > lockdep complain. > > > Uhhm, please provide more information - just using raw_* to silence > > > lockdep is generally the wrong thing to do. > > > > In traditional kexec, the new kernel will replace current one, so the > > irq is simply disabled. But now jumping back from kexeced kernel is > > supported, so the irq should be enabled again. > > > > The code sequence of irq during kexec jump is as follow: > > > > local_irq_disable(); /* in kernel_kexec() */ > > local_irq_disable(); /* in machine_kexec() */ > > local_irq_enable(); /* in kernel_kexec() */ > > > > The disable and enable is not match. Maybe another method is to use > > local_irq_save(), local_irq_restore() pair in machine_kexec(), so the > > disable and enable is matched. > > And its the machine kernel's lockdep instance that goes complain? > > whichever annotation gets used - and I think I can agree that raw_* > might be approriate there, this should be accompanied with a rather > elaborate changelog and preferably a comment in the code too. Without > such we'll be wondering in the years to come WTH happens here. Sorry, I find there is no complain from lockdep. Un-paired irq disable/enable has no problem with lockdep, just increase something such as "redundant_hardirqs_off". Please ignore this thread. Best Regards, Huang Ying ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
