Re: [PATCH v2 07/10] x86/msr: Switch rdmsr_safe_on_cpu() users to rdmsrq_safe_on_cpu()

[Ingo Molnar]

* Jürgen Groß <jgross@xxxxxxxx> wrote:

> Okay, fine with me.
> 
> I'll send V3 after rc1 of 7.2 as Dave suggested.

Since these are really trivial with little value in postponing
them over a full cycle and generating conflicts, a -rc7 base
might work as well, as long as you send them in the next 1-2 days
or so. (Unless Dave feels strongly about the timing.)

> One final question (not directly for this series, but for a future one):
> 
> I'd like to switch rdmsrq() from a macro to an inline function, as a macro
> using one of its parameters as the destination for an assignment isn't
> nice.

So basically:

  - rdmsrq(MSR_CORE_PERF_GLOBAL_CTRL, ctrl);
  + ctrl = rdmsrq(MSR_CORE_PERF_GLOBAL_CTRL);

? Agreed with that.

> Additionally I'd like to replace rdmsr*() use cases with rdmsrq*()
> and wrmsr*() with wrmsrq*() (similar to the replacements done is this
> series).

Yeah.

> The rdmsrq() transformation into a function can easily be done via a
> coccinelle script. What would be your choice regarding the sequence doing
> this?
> 
> a) Start with rdmsrq() transformation into a function.
> b) First do the rdmsr/wrmsr -> rdmsrq/wrmsrq replacements, then do the
>    rdmsrq() transformation into a function (this will touch more code, as
>    the initial rdmsr() use cases will be affected, too).
> c) Leave rdmsrq() as a macro.

I'd prefer (b), because while it's technically a bit more work,
the flow is easier to review: the rdmsr/wrmsr replacements
basically do a:

  macro(u32, u32) =>> macro(u64)

replacement, while with (a) we do a:

  macro(u32, u32) => u64 = function()

replacement that is a bit more complex transformation and bit
harder to review/validate at a glance. So I think it's better
to do the two transformations separately.

Thanks,

	Ingo