Re: [PATCH] MIPS: change type of asid_cache to unsigned long

[David Daney <ddaney.cavm@gmail.com>]

Regarding this patch (commit e5eb925a1804c4a52994ba57f4f68ee7a9132905), 
the fix is fine for 64-bit systems, as it is impossible to overflow a 
64-bit ASID value.

For 32-bit systems, there is still a problem, we don't see the type 
truncation issue that was present on 64-bit systems, but there can still 
be badness on ASID generation wrap.


Scenario:

  o Long live process (p0) that sleeps for a long time.  It acquires 
what we will call ASID_0 and then is scheduled off the CPU

  o We cycle through 2^32 ASIDs, and the asid_cache wraps around  (not 
difficult to do, just write a program that does nothing but mmap() 
munmap() in a loop).  We have seen this happen every 6 days with ebizzy 
benchmark program.

  o Start new program (p1) that happens to also get ASID_0

  o p0 wakes up, and is now sharing tlb entries with p1, chaos ensues.

A workaround for this would be to use u64 for both 32-bit and 64-bit for 
all ASID related variables.  I have a patch for this, is it worth 
testing on 32-bit systems, and sending it in?

David Daney


On 05/22/2014 06:42 AM, Ralf Baechle wrote:
> On Thu, May 22, 2014 at 10:06:11AM +0800, Yong Zhang wrote:
>
>> On Wed, May 21, 2014 at 01:29:36PM +0200, Ralf Baechle wrote:
>>> On Wed, May 21, 2014 at 01:38:53PM +0800, Yong Zhang wrote:
>>>
>>>> Please check the V2 in which I add the reporter.
>>>> And thanks libin for reporting it :)
>>>
>>> The bug was introduced in 5636919b5c909fee54a6ef5226475ecae012ad02
>>> [MIPS: Outline udelay and fix a few issues.] in 2009 btw.  I think
>>> the intension was to avoid holes in the structure and minimize
>>> the bloat.  I instead applied aptch
>>
>> Could you please show the patch?
>>
>>> which also moves another member
>>> of the struct arond such that no hole will be created in the struct.
>>> This is important because the strcture it accessed fairly frequently
>>> so we want to fit the most important members into as few cache
>>> lines as possible.
>>
>> I have tried to move the struct member around, but I found that the
>> hole cann't be avoided completely because for exampe struct cache_desc
>> is a bit special.
>
> Yes, struct cache_desc is still a problem.  Easily solvable though -
> some of it's members are excessivly large; by using smaller data types
> both the struct and its required alignment will shrink.  But that's
> for another patch; as for this patch my goal to just not make things
> any worse.
>
>    Ralf
>
> ---
>   arch/mips/include/asm/cpu-info.h | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/mips/include/asm/cpu-info.h b/arch/mips/include/asm/cpu-info.h
> index dc2135b..ff2707a 100644
> --- a/arch/mips/include/asm/cpu-info.h
> +++ b/arch/mips/include/asm/cpu-info.h
> @@ -39,14 +39,14 @@ struct cache_desc {
>   #define MIPS_CACHE_PINDEX	0x00000020	/* Physically indexed cache */
>
>   struct cpuinfo_mips {
> -	unsigned int		udelay_val;
> -	unsigned int		asid_cache;
> +	unsigned long		asid_cache;
>
>   	/*
>   	 * Capability and feature descriptor structure for MIPS CPU
>   	 */
>   	unsigned long		options;
>   	unsigned long		ases;
> +	unsigned int		udelay_val;
>   	unsigned int		processor_id;
>   	unsigned int		fpu_id;
>   	unsigned int		msa_id;
>
>