MIME-Version: 1.0
References: <20211122211327.5931-1-posk@google.com> <20211122211327.5931-4-posk@google.com>
 <20211124200822.GF721624@worktop.programming.kicks-ass.net>
In-Reply-To: <20211124200822.GF721624@worktop.programming.kicks-ass.net>
From:   Peter Oskolkov <posk@posk.io>
Date:   Thu, 25 Nov 2021 09:28:49 -0800
Message-ID: <CAFTs51Uka8VRCHuGidw7mRwATufp87U6S8SWUVod_kU-h6T3ew@mail.gmail.com>
Subject: Re: [PATCH v0.9.1 3/6] sched/umcg: implement UMCG syscalls
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     Ingo Molnar <mingo@redhat.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Andrew Morton <akpm@linux-foundation.org>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        Andy Lutomirski <luto@kernel.org>,
        Linux Memory Management List <linux-mm@kvack.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        linux-api@vger.kernel.org, Paul Turner <pjt@google.com>,
        Ben Segall <bsegall@google.com>,
        Peter Oskolkov <posk@google.com>,
        Andrei Vagin <avagin@google.com>, Jann Horn <jannh@google.com>,
        Thierry Delisle <tdelisle@uwaterloo.ca>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Thanks, Peter, for the review!

Some of your comments, like ratelimiting pr_warn and removing gotos,
are obvious in how to address them, so I'll just do that and won't
mention them here. Some comments are less clear re: what should be
done about them, so I have them below with my own comments/questions.

At a higher level, I get that the uaccess patch is bad and needs
serious changes. But based on your comments on this main patch so far,
it looks like the overall approach did not raise many objections - is
it so? Have you finished reviewing the patch?

Please also look at my questions/comments below.

Thanks,
Peter

[...]
> > +struct umcg_task {
[...]

>
> The thing is; I really don't see how this is supposed to be used. Where
> did the blocked and runnable list go ?
>
> I also don't see why the kernel cares about idle workers at all; that
> seems something userspace can sort itself just fine.
>
> The whole next_tid thing seems confused too, how can it be the next task
> when it must be the server? Also, what if there isn't an idle server?
>
> This just all isn't making any sense to me.

Based on your later comments I assume it is clearer now. The doc patch
5 has a lot of extra explanations and examples. Please let me know if
something is still unclear here.

> I'm still very hesitant to use ktime (fear the HPET); but I suppose it
> makes sense to use a time base that's accessible to userspace. Was
> MONOTONIC_RAW considered?

I believe it was considered. I'll re-consider it, and add a comment if
the new consideration arrives at the same conclusion.

> Using CLOCK_REALTIME timers while the rest of the thing runs off of
> CLOCK_MONOTONIC doesn't seem to make sense to me. Why would you want to
> have timeouts subject to DST shifts and crap like that?

Yes, these should be the same if at all possible. I'll definitely
reconsider what clock to use in both timeouts and state timestamps.

> Oooh, someone made things super confusing by doing s/runnable/idle/ on
> the whole thing :-( That only took me most of the day to figure out.
> Naming is important, don't mess about with stuff like this.

I clearly remember I had four states: blocked, pending, runnable,
running (I still believe that four states better reflect what is going
on here). The current blocked/idle/running is the result of an early
discussion. Something along the lines of:

<start of a recollection>
pending workers (=unblocked workers that the userspace still thinks
are blocked) are better named as idle; also the kernel does not really
care about what userspace thinks, so idle workers and runnable workers
are the same from the kernel point of view, so let's have one state
for these workers, not two.
<end of the recollection>

Please let me know if you want me to change anything here. I'll gladly
name workers on the idle worker list as idle (or whatever you prefer),
and workers that the userspace took out of the list as "runnable".
Just as a FYI, workers blocked in umcg_wait() will also be called
"runnable" then, as they are sitting in umcg_idle_loop() and can be
woken or swapped into.