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;
MIME-Version: 1.0
References: <0029af41-bf24-9972-10ac-f52e1bdcbf08@linux.intel.com>
 <CAHmME9o25v0kZUV-7qEY=6XXAyOA7q0sG8gpQfxHgr3sSVdsWw@mail.gmail.com>
 <41455798-1dcb-135f-516d-25ab9a8082f5@linux.intel.com> <Y1A+9kN6bwfXeqVt@zx2c4.com>
 <8acc3e4a-abbc-32bc-626e-7a216f6755c3@linux.intel.com> <Y1Bby6FEEWiFIjjD@zx2c4.com>
 <d47b30e9-5619-c631-aa92-f5d89e88a909@linux.intel.com> <87a64brlao.fsf@email.froward.int.ebiederm.org>
In-Reply-To: <87a64brlao.fsf@email.froward.int.ebiederm.org>
From:   "Jason A. Donenfeld" <Jason@zx2c4.com>
Date:   Mon, 28 Nov 2022 19:27:18 +0100
Message-ID: <CAHmME9pShsMrXwMhA+4FJKkc-nqCE74UQMXYy9fuEsoiDS2G=A@mail.gmail.com>
Subject: Re: [Intel-gfx] signal: break out of wait loops on kthread_stop()
To:     "Eric W. Biederman" <ebiederm@xmission.com>
Cc:     Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>,
        "Intel-gfx@lists.freedesktop.org" <Intel-gfx@lists.freedesktop.org>,
        linux-kernel@vger.kernel.org, sultan@kerneltoast.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Eric,

On Mon, Nov 28, 2022 at 7:22 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
>
> Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> writes:
>
> > On 19/10/2022 21:19, Jason A. Donenfeld wrote:
> >> On Wed, Oct 19, 2022 at 09:09:28PM +0100, Tvrtko Ursulin wrote:
> >>> Hm why is kthread_stop() after kthread_run() abuse? I don't see it in
> >>> kerneldoc that it must not be used for stopping threads.
> >> Because you don't want it to stop. You want to wait until it's done. If
> >> you call stop right after run, it will even stop it before it even
> >> begins to run. That's why you wind up sprinkling your msleeps
> >> everywhere, indicating that clearly this is not meant to work that way.
> > Not after kthread_run which wakes it up already. If the kerneldoc for
> > kthread_stop() is correct at least... In which case I really do think
> > that the yields are pointless/red herring. Perhaps they predate kthread_run and
> > then they were even wrong.
> >
> >>> Yep the yields and sleeps are horrible and will go. But they are also
> >>> not relevant for the topic at hand.
> >> Except they very much are. The reason you need these is because you're
> >> using kthread_stop() for something it's not meant to do.
> >
> > It is supposed to assert kthread_should_stop() which thread can look at as when
> > to exit. Except that now it can fail to get to that controlled exit
> > point. Granted that argument is moot since it implies incomplete error handling
> > in the thread anyway.
> >
> > Btw there are actually two use cases in our code base. One is thread controls
> > the exit, second is caller controls the exit. Anyway...
> >
> >>> Never mind, I was not looking for anything more than a suggestion on how
> >>> to maybe work around it in piece as someone is dealing with the affected
> >>> call sites.
> >> Sultan's kthread_work idea is probably the right direction. This would
> >> seem to have what you need.
> >
> > ... yes, it can be converted. Even though for one of the two use cases we need
> > explicit signalling. There now isn't anything which would assert
> > kthread_should_stop() without also asserting the signal, right?. Neither
> > I found that the thread work API can do it.
> >
> > Fingers crossed we were the only "abusers" of the API. There's a quite a number
> > of kthread_stop callers and it would be a large job to audit them all.
>
>
> I have been out and am coming to this late.   Did this get resolved?
>
>
> I really don't expect this affected much of anything else as the code
> sat in linux-next for an entire development cycle before being merged.
>
> But I would like to make certain problems with this change were resolved.

I just checked drm-next, and it looks like the i915 people resolved
their issue, and also got rid of those pesky yield()s in the process:
https://cgit.freedesktop.org/drm/drm/commit/?id=6407cf533217e09dfd895e64984c3f1ee3802373

Jason