Re: [RFCv2 03/10] x86: Introduce userspace API to handle per-thread features

From: "Edgecombe, Rick P" <rick.p.edgecombe@intel.com>
To: "tglx@linutronix.de" <tglx@linutronix.de>,
	"kirill.shutemov@linux.intel.com"
	<kirill.shutemov@linux.intel.com>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"peterz@infradead.org" <peterz@infradead.org>,
	"hjl.tools@gmail.com" <hjl.tools@gmail.com>,
	"linux-mm@kvack.org" <linux-mm@kvack.org>,
	"dave.hansen@linux.intel.com" <dave.hansen@linux.intel.com>,
	"aryabinin@virtuozzo.com" <aryabinin@virtuozzo.com>,
	"dvyukov@google.com" <dvyukov@google.com>,
	"x86@kernel.org" <x86@kernel.org>,
	"ak@linux.intel.com" <ak@linux.intel.com>,
	"Lutomirski, Andy" <luto@kernel.org>,
	"glider@google.com" <glider@google.com>
Subject: Re: [RFCv2 03/10] x86: Introduce userspace API to handle per-thread features
Date: Sat, 14 May 2022 23:06:58 +0000	[thread overview]
Message-ID: <9e59305039f2c8077ee087313d1df5ff06028cfe.camel@intel.com> (raw)
In-Reply-To: <87k0aose62.ffs@tglx>

On Sat, 2022-05-14 at 10:37 +0200, Thomas Gleixner wrote:
> On Fri, May 13 2022 at 23:50, Edgecombe, Rick P wrote:
> > On Sat, 2022-05-14 at 02:09 +0300, Kirill A. Shutemov wrote:
> > > On Fri, May 13, 2022 at 05:34:12PM +0000, Edgecombe, Rick P
> > > wrote:
> > > > On Fri, 2022-05-13 at 16:09 +0200, Alexander Potapenko wrote:
> > > > > > +
> > > > > > +       /* Handle ARCH_THREAD_FEATURE_ENABLE */
> > > > > > +
> > > > > > +       task->thread.features |= features;
> > > > > > +out:
> > > > > > +       return task->thread.features;
> > > > > 
> > > > > Isn't arch_prctl() supposed to return 0 on success?
> > > > 
> > > > Hmm, good point. Maybe we'll need a struct to pass info in and
> > > > out.
> > > 
> > > But values >0 are unused. I don't see why can't we use them.
> > 
> > Hmm, I don't know what it would break since it is a new "code"
> > argument. But the man page says:
> > "On success, arch_prctl() returns 0; on error, -1 is returned, and
> > errno is set to indicate the error."
> > 
> > So just change the man pages?
> > "On success, arch_prctl() returns positive values; on error, -1 is
> > returned, and errno is set to indicate the error."
> 
> Why?
> 
>         prctl(GET, &out)
> 
> is the pattern used all over the place.

It seems better to me, but we also need to pass something in.

The idea of the "enable" operation is that userspace would pass in all
the features that it wants in one call, and then find out back what was
successfully enabled. So unlike the other arch_prctl()s, it wants to
pass something in AND get a result in one arch_prctl() call. It doesn't
need to check what is supported ahead of time. Since these enabling
operations can fail (OOM, etc), userspace has to handle unexpected per-
feature failure anyway. So it just blindly asks for what it wants.

Any objections to having it write back the result in the same
structure?

Otherwise, the option that used to be used here was a "status"
arch_prctl(), which was called separately to find out what actually got
enabled after an "enable" call. That fit with the GET/SET semantics
already in place.

I guess we could also get rid of the batch enabling idea, and just have
one "enable" call per feature too. But then it is syscall heavy.

Any chance you could weigh in on this?