Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <29deabdb-5317-d64d-e05f-9bbe4438711d@efficios.com>
Date:   Tue, 11 Apr 2023 08:38:17 -0400
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.0
Subject: Re: [RFC PATCH v3] sched: Fix performance regression introduced by
 mm_cid
Content-Language: en-US
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     linux-kernel@vger.kernel.org, Aaron Lu <aaron.lu@intel.com>,
        Olivier Dion <odion@efficios.com>
References: <20230405162635.225245-1-mathieu.desnoyers@efficios.com>
 <386a6e32-a746-9eb1-d5ae-e5bedaa8fc75@efficios.com>
 <20230406095122.GF386572@hirez.programming.kicks-ass.net>
 <fdaa7242-4ddd-fbe2-bc0e-6c62054dbde8@efficios.com>
 <20230411084634.GA576825@hirez.programming.kicks-ass.net>
From:   Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
In-Reply-To: <20230411084634.GA576825@hirez.programming.kicks-ass.net>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 2023-04-11 04:46, Peter Zijlstra wrote:
> On Fri, Apr 07, 2023 at 07:50:42PM -0400, Mathieu Desnoyers wrote:
> 
>> Let's looks at the relevant combinations of TSA/TSB, and TMA transitions.
>>
>> Scenario A) (TSA)+(TMA) (from next task perspective)
>>
>> CPU0                                                 CPU1
>>
>> Context switch CS-1                                  Migrate-from
>>    - store to rq->curr: (N)->(Y) (TSA)                - cmpxchg to *pcpu_id to LAZY (TMA)
>>       *** missing barrier ?? ***                        (implied barrier after cmpxchg)
>>    - prepare_task_switch()
>>      - switch_mm_cid()
>>        - mm_cid_get (next)
>>          - READ_ONCE(*pcpu_cid)                       - rcu_dereference(src_rq->curr)
>>
>> This Dekker ensures that either task (Y) is observed by the rcu_dereference() or the LAZY
>> flag is observed by READ_ONCE(), or both are observed.
>>
>> If task (Y) store is observed by rcu_dereference(), it means that there is still
>> an active task on the cpu. Migrate-from will therefore not transition to UNSET, which
>> fulfills property (1). That observed task will itself eventually need a migrate-from
>> to be migrated away from that cpu, which fulfills property (2).
>>
>> If task (Y) is not observed, but the lazy flag is observed by READ_ONCE(), it will
>> move its state to UNSET, which clears the percpu cid perhaps uselessly (which is not
>> an issue for correctness). Because task (Y) is not observed, CPU1 can move ahead to
>> set the state to UNSET. Because moving state to UNSET is done with a cmpxchg expecting
>> that the old state has the LAZY flag set, only one thread will successfully UNSET.
>>
>> If both states (LAZY flag and task (Y)) are observed, the thread on CPU0 will observe
>> the LAZY flag and transition to UNSET (perhaps uselessly), and CPU1 will observe task
>> (Y) and do nothing more, which is fine.
>>
>> What we are effectively preventing with this Dekker is a scenario where neither LAZY
>> flag nor store (Y) are observed, which would fail property (1) because this would
>> UNSET a cid which is actively used.
> 
> OK, this I'll buy. Let me go stare at this more.
> 
>> Scenario B) (TSB)+(TMA) (from prev task perspective)
>>
>> CPU0                                                 CPU1
>>
>> Context switch CS-1                                  Migrate-from
>>    - store to rq->curr: (Y)->(N) (TSB)                - cmpxchg to *pcpu_id to LAZY (TMA)
>>      *** missing barrier ?? ***                         (implied barrier after cmpxchg)
>>    - prepare_task_switch()
>>      - switch_mm_cid()
>>        - cid_put_lazy() (prev)
>>          - READ_ONCE(*pcpu_cid)                       - rcu_dereference(src_rq->curr)
>>
> 
> This I'm conflicted about, if we're running Y, then how the heck do we
> get to setting LAZY in the first place?
> 
> For this scenario there must be at least an N->Y->N transition, such
> that the first:
> 
>    if (src_task->mm_cid_active && src_task->mm == mm) {
> 
> can observe N and proceed to setting LAZY. But that then leads us to the
> scenario above.

Remember that migrate-from does not hold any rq lock. Therefore, it's 
very much possible that the first check:

   if (src_task->mm_cid_active && src_task->mm == mm) {

observes (N), then gets delayed for a while, and then only sets the
LAZY flag when (Y) has been scheduled, leading us to Scenario B).

Thanks,

Mathieu


-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com