[PATCH v2 1/4] powerpc/tm: Remove msr_tm_active()

[Breno Leitao]

Hi Michael,

On 08/16/2018 09:49 PM, Michael Ellerman wrote:
> Michael Neuling <mikey at neuling.org> writes:
> 
>> On Mon, 2018-06-18 at 19:59 -0300, Breno Leitao wrote:
>>> Currently msr_tm_active() is a wrapper around MSR_TM_ACTIVE() if
>>> CONFIG_PPC_TRANSACTIONAL_MEM is set, or it is just a function that
>>> returns false if CONFIG_PPC_TRANSACTIONAL_MEM is not set.
>>>
>>> This function is not necessary, since MSR_TM_ACTIVE() just do the same,
>>> checking for the TS bits and does not require any TM facility.
>>>
>>> This patchset remove every instance of msr_tm_active() and replaced it
>>> by MSR_TM_ACTIVE().
>>>
>>> Signed-off-by: Breno Leitao <leitao at debian.org>
>>>
>>
>> Patch looks good... one minor nit below...
>>
>>>  
>>> -	if (!msr_tm_active(regs->msr) &&
>>> -		!current->thread.load_fp && !loadvec(current->thread))
>>> +	if (!current->thread.load_fp && !loadvec(current->thread)) {
>>> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>>> +		if (!MSR_TM_ACTIVE(regs->msr))
>>> +			return;
>>
>> Can you make a MSR_TM_ACTIVE() that returns false when
>> !CONFIG_PPC_TRANSACTIONAL_MEM. Then you don't need this inline #ifdef.
> 
> Is that safe?
> 
> I see ~50 callers of MSR_TM_ACTIVE(), are they all inside #ifdef TM ?

I checked all of them, and the only two that are not called inside a #ifdef
are at kvm/book3s_hv_tm.c. They are:

  kvm/book3s_hv_tm.c:		if (!MSR_TM_ACTIVE(msr)) {
  kvm/book3s_hv_tm.c:		if (MSR_TM_ACTIVE(msr) || !(vcpu->arch.texasr & TEXASR_FS)) {

All the others are being called inside the #ifdef

Other than that, I do not see why it would be a problem in the way I
implemented it, since it will return false for the two cases above, which
seems correct. Take a look on how the definition became:

  #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
  #define MSR_TM_ACTIVE(x) (((x) & MSR_TS_MASK) != 0) /* Transaction active? */
  #else
  #define MSR_TM_ACTIVE(x) 0
  #endif

I also tested it with different config files, and I didn't see any complain.
These are the platforms I built for.

* powernv_defconfig
* pseries_le_defconfig
* pseries_defconfig
* ppc64_defconfig
* ppc64e_defconfig
* pmac32_defconfig
* ppc44x_defconfig
* mpc85xx_smp_defconfig
* mpc85xx_defconfig
* ps3_defconfig

Anyway, if you have any other suggestion I can follow in order to guarantee
that I am not causing any regression, I would be happy. Touching these core
kernel macros is scary!

Thanks!