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?
