Message-ID: <ea424a75-c13b-7087-2bfe-a8696b56367d@redhat.com>
Date:   Thu, 2 Mar 2023 11:51:17 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Subject: Re: [PATCH v2 02/11] this_cpu_cmpxchg: ARM64: switch this_cpu_cmpxchg
 to locked, add _local function
Content-Language: en-US
From:   David Hildenbrand <david@redhat.com>
To:     Marcelo Tosatti <mtosatti@redhat.com>,
        Christoph Lameter <cl@linux.com>
Cc:     Aaron Tomlin <atomlin@atomlin.com>,
        Frederic Weisbecker <frederic@kernel.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        linux-kernel@vger.kernel.org, linux-mm@kvack.org
References: <20230209150150.380060673@redhat.com>
 <20230209153204.683821550@redhat.com>
 <a754561a-1d0a-ebfb-952f-054bae6dc533@redhat.com>
Organization: Red Hat
In-Reply-To: <a754561a-1d0a-ebfb-952f-054bae6dc533@redhat.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 02.03.23 11:42, David Hildenbrand wrote:
> On 09.02.23 16:01, Marcelo Tosatti wrote:
>> Goal is to have vmstat_shepherd to transfer from
>> per-CPU counters to global counters remotely. For this,
>> an atomic this_cpu_cmpxchg is necessary.
>>
>> Following the kernel convention for cmpxchg/cmpxchg_local,
>> change ARM's this_cpu_cmpxchg_ helpers to be atomic,
>> and add this_cpu_cmpxchg_local_ helpers which are not atomic.
>>
>> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
>>
>> Index: linux-vmstat-remote/arch/arm64/include/asm/percpu.h
>> ===================================================================
>> --- linux-vmstat-remote.orig/arch/arm64/include/asm/percpu.h
>> +++ linux-vmstat-remote/arch/arm64/include/asm/percpu.h
>> @@ -232,13 +232,23 @@ PERCPU_RET_OP(add, add, ldadd)
>>    	_pcp_protect_return(xchg_relaxed, pcp, val)
>>    
>>    #define this_cpu_cmpxchg_1(pcp, o, n)	\
>> -	_pcp_protect_return(cmpxchg_relaxed, pcp, o, n)
>> +	_pcp_protect_return(cmpxchg, pcp, o, n)
>>    #define this_cpu_cmpxchg_2(pcp, o, n)	\
>> -	_pcp_protect_return(cmpxchg_relaxed, pcp, o, n)
>> +	_pcp_protect_return(cmpxchg, pcp, o, n)
>>    #define this_cpu_cmpxchg_4(pcp, o, n)	\
>> -	_pcp_protect_return(cmpxchg_relaxed, pcp, o, n)
>> +	_pcp_protect_return(cmpxchg, pcp, o, n)
>>    #define this_cpu_cmpxchg_8(pcp, o, n)	\
>> +	_pcp_protect_return(cmpxchg, pcp, o, n)
>> +
>> +#define this_cpu_cmpxchg_local_1(pcp, o, n)	\
>>    	_pcp_protect_return(cmpxchg_relaxed, pcp, o, n)
>> +#define this_cpu_cmpxchg_local_2(pcp, o, n)	\
>> +	_pcp_protect_return(cmpxchg_relaxed, pcp, o, n)
>> +#define this_cpu_cmpxchg_local_4(pcp, o, n)	\
>> +	_pcp_protect_return(cmpxchg_relaxed, pcp, o, n)
>> +#define this_cpu_cmpxchg_local_8(pcp, o, n)	\
>> +	_pcp_protect_return(cmpxchg_relaxed, pcp, o, n)
>> +
> 
> Call me confused (not necessarily your fault :) ).
> 
> We have cmpxchg_local, cmpxchg_relaxed and cmpxchg.
> this_cpu_cmpxchg_local_* now calls ... *drumroll* ... cmpxchg_relaxed.
> 
> IIUC, cmpxchg_local is only guaranteed to be atomic WRO the current CPU
> (especially, protection against interrupts when the operation is
> implemented using multiple instructions). We do have a generic
> implementation that disables/enables interrupts.
> 
> IIUC, cmpxchg_relaxed an atomic update without any memory ordering
> guarantees (in contrast to cmpxchg, cmpxchg_acquire, cmpxchg_acquire).
> We default to arch_cmpxchg if we don't have arch_cmpxchg_relaxed.
> arch_cmpxchg defaults to arch_cmpxchg_local, if not supported.
> 
> 
> Naturally I wonder:
> 
> (a) Should these new variants be rather called
>       this_cpu_cmpxchg_relaxed_* ?
> 
> (b) Should these new variants rather call the "_local" variant?
> 
> 
> Shedding some light on this would be great.

Nevermind, looking at the other patches I realized that this is 
arch-specific. Other archs that have _local variants call the _local 
variants. So I assume we really want the name this_cpu_cmpxchg_local_*, 
and using _relaxed here is just the aarch64 way of implementing _local 
via _relaxed.

Confusing :)

-- 
Thanks,

David / dhildenb