Sergey Senozhatsky <sergey.senozhatsky.work@xxxxxxxxx> 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