| Message ID | 1456748580-10519-3-git-send-email-shreyas@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Mon, Feb 29, 2016 at 05:52:59PM +0530, Shreyas B. Prabhu wrote: > Before entering any idle state which can result in a state loss > we currently save the context in the stack before entering idle. > Encapsulate these steps in a macro IDLE_STATE_PREP. Move this > and other macros to commonly accessible location. There are two problems with this. First, your new macro does much more than create a stack frame and save some registers. It also messes with interrupts and potentially executes a blr instruction. That is not what people would expect from the name of the macro or the comments around it. It also means that it would be hard to reuse the macro in another place. Secondly, I don't think this change helps readability. Since the macro is only used in one place, it doesn't reduce the total number of lines of code, in fact it increases it slightly. Having the macro just means that someone reading the code has to look in two places to see what power7_powersave_common is actually doing, rather than having all the code in one place. If what your macro did was a single thing conceptually that took several instructions, then it might be helpful, but as it is, it doesn't help readability. Paul.
Hi Paul, On 03/17/2016 04:45 PM, Paul Mackerras wrote: > On Mon, Feb 29, 2016 at 05:52:59PM +0530, Shreyas B. Prabhu wrote: >> Before entering any idle state which can result in a state loss >> we currently save the context in the stack before entering idle. >> Encapsulate these steps in a macro IDLE_STATE_PREP. Move this >> and other macros to commonly accessible location. > > There are two problems with this. First, your new macro does much > more than create a stack frame and save some registers. It also > messes with interrupts and potentially executes a blr instruction. > That is not what people would expect from the name of the macro or the > comments around it. It also means that it would be hard to reuse the > macro in another place. > > Secondly, I don't think this change helps readability. Since the > macro is only used in one place, it doesn't reduce the total number of > lines of code, in fact it increases it slightly. This patch was in preparation for support for new POWER ISA v3 idle states. The idea was to have the common idle preparation steps in a macro which be reused while adding support for the new idle states. With this context do you think this macro with better comments make sense? Thanks, Shreyas
On Fri, Mar 18, 2016 at 08:23:24PM +0530, Shreyas B Prabhu wrote: > Hi Paul, > > On 03/17/2016 04:45 PM, Paul Mackerras wrote: > > On Mon, Feb 29, 2016 at 05:52:59PM +0530, Shreyas B. Prabhu wrote: > >> Before entering any idle state which can result in a state loss > >> we currently save the context in the stack before entering idle. > >> Encapsulate these steps in a macro IDLE_STATE_PREP. Move this > >> and other macros to commonly accessible location. > > > > There are two problems with this. First, your new macro does much > > more than create a stack frame and save some registers. It also > > messes with interrupts and potentially executes a blr instruction. > > That is not what people would expect from the name of the macro or the > > comments around it. It also means that it would be hard to reuse the > > macro in another place. > > > > Secondly, I don't think this change helps readability. Since the > > macro is only used in one place, it doesn't reduce the total number of > > lines of code, in fact it increases it slightly. > > This patch was in preparation for support for new POWER ISA v3 idle > states. The idea was to have the common idle preparation steps in a > macro which be reused while adding support for the new idle states. With > this context do you think this macro with better comments make sense? No, it still does too many disparate things. In particular it's a bad idea to embed a blr inside a macro unless the name makes it very clear that the macro can cause a return (e.g. the macro name is RETURN_IF_<something>). Yours would need to be called MAKE_STACK_FRAME_AND_SAVE_SPRS_AND_HARD_DISABLE_AND_RETURN_IF_IRQ_OCCURRED or something. :) Paul.
On 03/19/2016 05:51 AM, Paul Mackerras wrote: > On Fri, Mar 18, 2016 at 08:23:24PM +0530, Shreyas B Prabhu wrote: >> Hi Paul, >> >> On 03/17/2016 04:45 PM, Paul Mackerras wrote: >>> On Mon, Feb 29, 2016 at 05:52:59PM +0530, Shreyas B. Prabhu wrote: >>>> Before entering any idle state which can result in a state loss >>>> we currently save the context in the stack before entering idle. >>>> Encapsulate these steps in a macro IDLE_STATE_PREP. Move this >>>> and other macros to commonly accessible location. >>> >>> There are two problems with this. First, your new macro does much >>> more than create a stack frame and save some registers. It also >>> messes with interrupts and potentially executes a blr instruction. >>> That is not what people would expect from the name of the macro or the >>> comments around it. It also means that it would be hard to reuse the >>> macro in another place. >>> >>> Secondly, I don't think this change helps readability. Since the >>> macro is only used in one place, it doesn't reduce the total number of >>> lines of code, in fact it increases it slightly. >> >> This patch was in preparation for support for new POWER ISA v3 idle >> states. The idea was to have the common idle preparation steps in a >> macro which be reused while adding support for the new idle states. With >> this context do you think this macro with better comments make sense? > > No, it still does too many disparate things. In particular it's a bad > idea to embed a blr inside a macro unless the name makes it very clear > that the macro can cause a return (e.g. the macro name is > RETURN_IF_<something>). Yours would need to be called > MAKE_STACK_FRAME_AND_SAVE_SPRS_AND_HARD_DISABLE_AND_RETURN_IF_IRQ_OCCURRED > or something. :) > Ok :) . I'll drop this patch and work this differently. Thanks, Shreyas
diff --git a/arch/powerpc/include/asm/cpuidle.h b/arch/powerpc/include/asm/cpuidle.h index d2f99ca1e3a6..6c678a779e8e 100644 --- a/arch/powerpc/include/asm/cpuidle.h +++ b/arch/powerpc/include/asm/cpuidle.h @@ -15,6 +15,74 @@ extern u32 pnv_fastsleep_workaround_at_entry[]; extern u32 pnv_fastsleep_workaround_at_exit[]; #endif +/* + * IDLE_STATE_PREP - Will be called in preparation for entering + * any hardware idle state. Since idle states can result in a + * state loss, we create a regs frame on the stack, fill it up + * with the state we care about and stick a pointer to it in + * PACAR1. Use interrupt stack frame for this purpose. + * r3 - contains idle state to be entered + * r13 - PACA pointer + */ +#define IDLE_STATE_PREP \ + mflr r0; \ + std r0,16(r1); \ + stdu r1,-INT_FRAME_SIZE(r1); \ + std r0,_LINK(r1); \ + std r0,_NIP(r1); \ + \ + /* Hard disable interrupts */ \ + mfmsr r9; \ + rldicl r9,r9,48,1; \ + rotldi r9,r9,16; \ + mtmsrd r9,1; /* hard-disable interrupts */ \ + \ + /* Check if something happened while soft-disabled */ \ + lbz r0,PACAIRQHAPPENED(r13); \ + andi. r0,r0,~PACA_IRQ_HARD_DIS@l; \ + beq 1f; \ + cmpwi cr0,r4,0; \ + beq 1f; \ + addi r1,r1,INT_FRAME_SIZE; \ + ld r0,16(r1); \ + li r3,0; /* Return 0 (no nap) */ \ + mtlr r0; \ + blr; \ +1: /* We mark irqs hard disabled as this is the state \ + * we'll be in when returning and we need to tell \ + * arch_local_irq_restore() about it */ \ + li r0,PACA_IRQ_HARD_DIS; \ + stb r0,PACAIRQHAPPENED(r13); \ + \ + /* We haven't lost state ... yet */ \ + li r0,0; \ + stb r0,PACA_NAPSTATELOST(r13); \ + \ + /* Continue saving state */ \ + SAVE_GPR(2, r1); \ + SAVE_NVGPRS(r1); \ + mfcr r4; \ + std r4,_CCR(r1); \ + std r9,_MSR(r1); \ + std r1,PACAR1(r13); \ + +/* + * We use interrupt stack to save and restore few registers like + * PC, CR, LR and NVGPRs while enter/exiting idle states. Since + * volatile registers don't need to be saved/restored use that space + * instead to save/restore sprs which lose state during winkle. + */ +#define _SDR1 GPR3 +#define _RPR GPR4 +#define _SPURR GPR5 +#define _PURR GPR6 +#define _TSCR GPR7 +#define _DSCR GPR8 +#define _AMOR GPR9 +#define _WORT GPR10 +#define _WORC GPR11 + + #endif #endif diff --git a/arch/powerpc/kernel/idle_power7.S b/arch/powerpc/kernel/idle_power7.S index abc53e88a5b4..6af57e292848 100644 --- a/arch/powerpc/kernel/idle_power7.S +++ b/arch/powerpc/kernel/idle_power7.S @@ -24,20 +24,6 @@ #undef DEBUG -/* - * Use unused space in the interrupt stack to save and restore - * registers for winkle support. - */ -#define _SDR1 GPR3 -#define _RPR GPR4 -#define _SPURR GPR5 -#define _PURR GPR6 -#define _TSCR GPR7 -#define _DSCR GPR8 -#define _AMOR GPR9 -#define _WORT GPR10 -#define _WORC GPR11 - /* Idle state entry routines */ #define IDLE_STATE_ENTER_SEQ(IDLE_INST) \ @@ -77,55 +63,8 @@ core_idle_lock_held: * 1 - check */ _GLOBAL(power7_powersave_common) - /* Use r3 to pass state nap/sleep/winkle */ - /* NAP is a state loss, we create a regs frame on the - * stack, fill it up with the state we care about and - * stick a pointer to it in PACAR1. We really only - * need to save PC, some CR bits and the NV GPRs, - * but for now an interrupt frame will do. - */ - mflr r0 - std r0,16(r1) - stdu r1,-INT_FRAME_SIZE(r1) - std r0,_LINK(r1) - std r0,_NIP(r1) - - /* Hard disable interrupts */ - mfmsr r9 - rldicl r9,r9,48,1 - rotldi r9,r9,16 - mtmsrd r9,1 /* hard-disable interrupts */ - - /* Check if something happened while soft-disabled */ - lbz r0,PACAIRQHAPPENED(r13) - andi. r0,r0,~PACA_IRQ_HARD_DIS@l - beq 1f - cmpwi cr0,r4,0 - beq 1f - addi r1,r1,INT_FRAME_SIZE - ld r0,16(r1) - li r3,0 /* Return 0 (no nap) */ - mtlr r0 - blr - -1: /* We mark irqs hard disabled as this is the state we'll - * be in when returning and we need to tell arch_local_irq_restore() - * about it - */ - li r0,PACA_IRQ_HARD_DIS - stb r0,PACAIRQHAPPENED(r13) - - /* We haven't lost state ... yet */ - li r0,0 - stb r0,PACA_NAPSTATELOST(r13) - - /* Continue saving state */ - SAVE_GPR(2, r1) - SAVE_NVGPRS(r1) - mfcr r4 - std r4,_CCR(r1) - std r9,_MSR(r1) - std r1,PACAR1(r13) + /* Save PC, CR, LR and NVGPRs in stack */ + IDLE_STATE_PREP; /* * Go to real mode to do the nap, as required by the architecture.
Before entering any idle state which can result in a state loss we currently save the context in the stack before entering idle. Encapsulate these steps in a macro IDLE_STATE_PREP. Move this and other macros to commonly accessible location. Signed-off-by: Shreyas B. Prabhu <shreyas@linux.vnet.ibm.com> --- arch/powerpc/include/asm/cpuidle.h | 68 ++++++++++++++++++++++++++++++++++++++ arch/powerpc/kernel/idle_power7.S | 65 ++---------------------------------- 2 files changed, 70 insertions(+), 63 deletions(-)