doubts about switch_mm

doubts about switch_mm

[Gilles Chanteperdrix]

Hi, 

I know that switch_mm has been like this for years, and that it seems 
to work, but still, something seems wrong:

switch_mm code, as of 3.5 contains:

1        if (!cpumask_test_and_set_cpu(cpu, mm_cpumask(next)) || prev != next) {  
2                check_and_switch_context(next, tsk);                             
3                if (cache_is_vivt())                                             
4                        cpumask_clear_cpu(cpu, mm_cpumask(prev));                
5        }                                                                        

Line 1 seems to mean that maybe switch_mm is called with prev == next.
But then, what line 4 does is certainly wrong if prev == next.

Regards.

-- 
                                                                Gilles.

doubts about switch_mm

[Russell King - ARM Linux]

On Thu, Aug 02, 2012 at 09:08:19PM +0200, Gilles Chanteperdrix wrote:
> switch_mm code, as of 3.5 contains:
> 
> 1   if (!cpumask_test_and_set_cpu(cpu, mm_cpumask(next)) || prev != next) {  
> 2           check_and_switch_context(next, tsk);                             
> 3           if (cache_is_vivt())                                             
> 4                   cpumask_clear_cpu(cpu, mm_cpumask(prev));                
> 5   }                                                                        
> 
> Line 1 seems to mean that maybe switch_mm is called with prev == next.
> But then, what line 4 does is certainly wrong if prev == next.

Look at it more carefully.

If prev == next, then we're already running with *this* mm.  The bit
in the CPU mask for this CPU will be set.

So, cpumask_test_and_set_cpu(cpu, mm_cpumask(next)) will be true, and
because the condition is inverted, the first half of the if condition
is false.

The second half is false, because prev == next.  So lines 2-4 will not
be executed.

doubts about switch_mm

[Gilles Chanteperdrix]

On 08/02/2012 09:49 PM, Russell King - ARM Linux wrote:

> On Thu, Aug 02, 2012 at 09:08:19PM +0200, Gilles Chanteperdrix wrote:
>> switch_mm code, as of 3.5 contains:
>>
>> 1   if (!cpumask_test_and_set_cpu(cpu, mm_cpumask(next)) || prev != next) {  
>> 2           check_and_switch_context(next, tsk);                             
>> 3           if (cache_is_vivt())                                             
>> 4                   cpumask_clear_cpu(cpu, mm_cpumask(prev));                
>> 5   }                                                                        
>>
>> Line 1 seems to mean that maybe switch_mm is called with prev == next.
>> But then, what line 4 does is certainly wrong if prev == next.
> 
> Look at it more carefully.
> 
> If prev == next, then we're already running with *this* mm.  The bit
> in the CPU mask for this CPU will be set.
> 
> So, cpumask_test_and_set_cpu(cpu, mm_cpumask(next)) will be true, and
> because the condition is inverted, the first half of the if condition
> is false.
> 
> The second half is false, because prev == next.  So lines 2-4 will not
> be executed.
>


So, prev != next on line 1 is useless ?


-- 
                                                                Gilles.

doubts about switch_mm

[Gilles Chanteperdrix]

On 08/02/2012 10:02 PM, Gilles Chanteperdrix wrote:

> On 08/02/2012 09:49 PM, Russell King - ARM Linux wrote:
> 
>> On Thu, Aug 02, 2012 at 09:08:19PM +0200, Gilles Chanteperdrix wrote:
>>> switch_mm code, as of 3.5 contains:
>>>
>>> 1   if (!cpumask_test_and_set_cpu(cpu, mm_cpumask(next)) || prev != next) {  
>>> 2           check_and_switch_context(next, tsk);                             
>>> 3           if (cache_is_vivt())                                             
>>> 4                   cpumask_clear_cpu(cpu, mm_cpumask(prev));                
>>> 5   }                                                                        
>>>
>>> Line 1 seems to mean that maybe switch_mm is called with prev == next.
>>> But then, what line 4 does is certainly wrong if prev == next.
>>
>> Look at it more carefully.
>>
>> If prev == next, then we're already running with *this* mm.  The bit
>> in the CPU mask for this CPU will be set.
>>
>> So, cpumask_test_and_set_cpu(cpu, mm_cpumask(next)) will be true, and
>> because the condition is inverted, the first half of the if condition
>> is false.
>>
>> The second half is false, because prev == next.  So lines 2-4 will not
>> be executed.
>>
> 
> 
> So, prev != next on line 1 is useless ?
> 
> 

Ok, got it, sorry for the noise.

-- 
                                                                Gilles.