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

[Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>]

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.

	-ss

--
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>