Re: [PATCH v3 1/2] mm: migrate: prevent racy access to tlb_flush_pending

[Nadav Amit <nadav.amit@gmail.com>]

Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> wrote:

> just my 5 silly cents,
> 
> On (07/27/17 04:40), Nadav Amit wrote:
> [..]
>> static inline void set_tlb_flush_pending(struct mm_struct *mm)
>> {
>> -	mm->tlb_flush_pending = true;
>> +	atomic_inc(&mm->tlb_flush_pending);
>> 
>> 	/*
>> 	 * Guarantee that the tlb_flush_pending store does not leak into the
>> @@ -544,7 +544,7 @@ static inline void set_tlb_flush_pending(struct mm_struct *mm)
>> static inline void clear_tlb_flush_pending(struct mm_struct *mm)
>> {
>> 	barrier();
>> -	mm->tlb_flush_pending = false;
>> +	atomic_dec(&mm->tlb_flush_pending);
>> }
> 
> so, _technically_, set_tlb_flush_pending() can be nested, right? IOW,
> 
> 	set_tlb_flush_pending()
> 	set_tlb_flush_pending()
> 	flush_tlb_range()
> 	clear_tlb_flush_pending()
> 	clear_tlb_flush_pending()  // if we miss this one, then
> 				   // ->tlb_flush_pending is !clear,
> 				   // even though we called
> 				   // clear_tlb_flush_pending()
> 
> if so then set_ and clear_ are a bit misleading names for something
> that does atomic_inc()/atomic_dec() internally.
> 
> especially when one sees this part
> 
>> -	clear_tlb_flush_pending(mm);
>> +#if defined(CONFIG_NUMA_BALANCING) || defined(CONFIG_COMPACTION)
>> +	atomic_set(&mm->tlb_flush_pending, 0);
>> +#endif
> 
> so we have clear_tlb_flush_pending() function which probably should
> set it to 0 as the name suggests (I see what you did tho), yet still
> do atomic_set() under ifdef-s.
> 
> well, just nitpicks.

I see your point. Initially, I tried to keep exactly the same interface to
reduce the number of changes, but it might be misleading. I will change the
names (inc/dec_tlb_flush_pending). I will create init_tlb_flush_pending()
for initialization since you ask, but Minchan's changes would likely remove
the ifdef’s, making it a bit strange for a single use.

Anyhow, I’ll wait to see if there any other comments and then do it for v4.

Thanks,
Nadav


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>