Message-ID: <3e5e0120-50fd-51c0-d817-5b1dc4c14e97@asahilina.net>
Date:   Thu, 9 Mar 2023 15:30:17 +0900
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.1
Subject: Re: [PATCH RFC 10/18] drm/scheduler: Add can_run_job callback
Content-Language: en-US
To:     =?UTF-8?Q?Christian_K=c3=b6nig?= <christian.koenig@amd.com>,
        Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        Maxime Ripard <mripard@kernel.org>,
        Thomas Zimmermann <tzimmermann@suse.de>,
        David Airlie <airlied@gmail.com>,
        Daniel Vetter <daniel@ffwll.ch>,
        Miguel Ojeda <ojeda@kernel.org>,
        Alex Gaynor <alex.gaynor@gmail.com>,
        Wedson Almeida Filho <wedsonaf@gmail.com>,
        Boqun Feng <boqun.feng@gmail.com>, Gary Guo <gary@garyguo.net>,
        =?UTF-8?Q?Bj=c3=b6rn_Roy_Baron?= <bjorn3_gh@protonmail.com>,
        Sumit Semwal <sumit.semwal@linaro.org>,
        Luben Tuikov <luben.tuikov@amd.com>,
        Jarkko Sakkinen <jarkko@kernel.org>,
        Dave Hansen <dave.hansen@linux.intel.com>
Cc:     Alyssa Rosenzweig <alyssa@rosenzweig.io>,
        Karol Herbst <kherbst@redhat.com>,
        Ella Stanforth <ella@iglunix.org>,
        Faith Ekstrand <faith.ekstrand@collabora.com>,
        Mary <mary@mary.zone>, linux-kernel@vger.kernel.org,
        dri-devel@lists.freedesktop.org, rust-for-linux@vger.kernel.org,
        linux-media@vger.kernel.org, linaro-mm-sig@lists.linaro.org,
        linux-sgx@vger.kernel.org, asahi@lists.linux.dev
References: <20230307-rust-drm-v1-0-917ff5bc80a8@asahilina.net>
 <20230307-rust-drm-v1-10-917ff5bc80a8@asahilina.net>
 <cd788ccf-0cf1-85d5-1bf8-efc259bd7e11@amd.com>
 <a075d886-0820-b6fb-fcd0-45bfdc75e37d@asahilina.net>
 <2b1060e9-86ba-7e16-14f1-5b5fa63de719@amd.com>
 <9f76bb68-b462-b138-d0ad-d27c972530d4@asahilina.net>
 <a39c6b40-f190-002d-ae1c-8b58c6442df2@amd.com>
 <4bbfc1a3-cfc3-87f4-897b-b6637bac3bd0@asahilina.net>
 <b0aa78b2-b432-200a-8953-a80c462fa6ee@amd.com>
 <c0624252-070e-bd44-2116-93a1d63a1359@asahilina.net>
 <d1fccceb-ca77-f653-17fc-63168e0da884@amd.com>
 <9c3dc2ad-11e4-6004-7230-8ca752e3d9f7@asahilina.net>
 <d544748c-8a2b-7c08-f199-182a56af22be@amd.com>
From:   Asahi Lina <lina@asahilina.net>
In-Reply-To: <d544748c-8a2b-7c08-f199-182a56af22be@amd.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 09/03/2023 05.14, Christian König wrote:
>> I think you mean wake_up_interruptible(). That would be
>> drm_sched_job_done(), on the fence callback when a job completes, which
>> as I keep saying is the same logic used for
>> hw_rq_count/hw_submission_limit tracking.
> 
> As the documentation to wait_event says:
> 
>   * wake_up() has to be called after changing any variable that could
>   * change the result of the wait condition.
> 
> So what you essentially try to do here is to skip that and say 
> drm_sched_job_done() would call that anyway, but when you read any 
> variable to determine that state then as far as I can see nothing is 
> guarantying that order.

The driver needs to guarantee that any changes to that state precede a
job completion fence signal of course, that's the entire idea of the
API. It's supposed to represent a check for per-scheduler (or more
specific, but not more global) resources that are released on job
completion. Of course if you misuse the API you could cause a problem,
but what I'm trying to say is that the API as designed and when used as
intended does work properly.

Put another way: job completions always need to cause the sched main
loop to run an iteration anyway (otherwise we wouldn't make forward
progress), and job completions are exactly the signal that the
can_run_job() condition may have changed.

> The only other possibility how you could use the callback correctly 
> would be to call drm_fence_is_signaled() to query the state of your hw 
> submission from the same fence which is then signaled. But then the 
> question is once more why you don't give that fence directly to the 
> scheduler?

But the driver is supposed to guarantee that the ordering is always 1.
resources freed, 2. fence signaled. So you don't need to check for the
fence, you can just check for the resource state. If the callback
returns false then by definition the fence wasn't yet signaled at some
point during its execution (because the resources weren't yet freed),
and since it would be in the wait_event_interruptible() check path, by
definition the fence signaling at any point during or after the check
would cause the thread to wake up again and re-check.

Thread 1                                          Thread 2
1. wait_event_interruptible() arms wq             1. Free resources
2. can_run_job() checks resources                 2. Signal fence
3. wait_event_interruptible() sleeps on wq        3. Fence wakes up wq
4. loop

There is no possible interleaving of those sequences that leads to a
lost event and the thread not waking up:
- If T2.3 happens before T1.1, that means T2.1 happened earlier and T1.2
must return true.
- If T2.3 happens after T1.1 but before T1.3, the wq code will ensure
the wq does not sleep (or immediately wakes up) at T1.3 since it was
signaled during the condition check, after the wq was armed. At the next
check loop, T1.2 will then return true, since T2.1 already happened
before T2.3.
- If T2.3 happens during T1.3, the wq wakes up normally and does another
check, and at that point T1.2 returns true.

QED.

~~ Lina