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

From: Michael Ellerman <[email-masked]>
Date: Tue, 21 Aug 2018 16:33:06 +1000

Breno Leitao <[email-masked]> writes:
> On 08/16/2018 09:49 PM, Michael Ellerman wrote:
>> Michael Neuling <[email-masked]> 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 <[email-masked]>
>>>>
>>>
>>> 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)) {

That whole file is only built if TM=y:

kvm-hv-$(CONFIG_PPC_TRANSACTIONAL_MEM) += \
	book3s_hv_tm.o

> 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

Imagine we had some code somewhere that checked for TM being active in a
non-TM aware kernel, that would break with this change, because now the
MSR check does nothing when TM=n.

eg. we might check at boot time that we're not transactional, eg. in
case we came from a kdump kernel that was in a transaction.

So if all the call-sites are already inside an #ifdef I'd be inclined to
not add the #ifdef around the MSR_TM_ACTIVE macro.

That way the macro can always be used to check the MSR value, whether TM
is compiled in or out.

cheers