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