On Thu, Oct 8, 2020 at 10:39 PM Johannes Berg [off-list ref] wrote:

On Thu, 2020-10-08 at 21:54 +0300, Octavian Purdila wrote:

quoted

On Wed, Oct 7, 2020 at 9:57 PM Johannes Berg [off-list ref] wrote:

quoted

On Tue, 2020-10-06 at 18:44 +0900, Hajime Tazaki wrote:

quoted

nommu mode does not support user processes

I find this really confusing. I'm not sure why you ended up calling this
"nommu mode", but there *are* (still) (other) nommu arches, and they
*do* support userspace processes.

Isn't this really just "LKL mode" or something like that?

This is a very good point, while some other patches make sense in the
nommu mode, this one does not - it is rather needed because of the
"library mode".

Not sure what we can do other than creating a new "library mode" in
addition to the "nommu mode". Any suggestions?

Well there's no "nommu mode" in UML other than what you're doing here,
so as I said on some other patch, it sort of makes sense to have "LKL ==
NOMMU", but the equation doesn't make sense everywhere, since it's not
fundamentally NOMMU that drives the need for things (like here no
userspace, elsewhere the ifdefs, etc.), but LKL-mode.

So I don't think it would be *in addition* to "nommu mode" since such a
thing doesn't exist on UML (only on other architectures), but mostly be
a rename of "nommu mode" to "lkl mode" or so?

Don't really have any other suggestions, or maybe I'm not understanding
your question right.

OK, I agree, renaming "nommu mode" to "lkl mode" looks like the right
thing to do for now.

quoted

quoted

IOW, why isn't this just

void lkl_sem_free(struct lkl_sem *sem);
void lkl_sem_up(struct lkl_sem *sem);
...

and then posix-host.c just includes the header file and implements those
functions?

I don't see any reason for this to be allowed to have multiple variants
linked and then picking them at runtime?

We could try that and see how it goes. This was baked liked this long
time ago, when we wanted to support Windows and there was no proper
support for weak functions in mingw for PE/COFF (it still not
supported but at least we do have a few patches that fix that).

You've required weak functions elsewhere, but in this case you don't
even need them since you don't need things to link without an
implementation? At least I don't see why you'd want to be able to link a
binary that doesn't have an implementation of the ops required to run?

Yeah, all good points :) I'll discuss it more with Hajime to make sure
I haven't missed anything and we will try it in the next patch series.

quoted

quoted

Yeah, what? That's an incomprehensible piece of code. At least add
comments, if it _really_ is necessary?

Yeah, sorry about that. We missed adding a bunch of comments in the
commit message. It got this complicated because of optimizations to
avoid context switching between the native thread running the
application and the kernel thread running the system call or interrupt
handler.

Maybe we should revert to the initial simpler implementation for now
and add it later?

Perhaps? Not really sure. Could the optimisations be added in steps so
they're something that can be explained/followed? If not, well, perhaps
to ease review for now it'd make sense to start simpler, but I guess
eventually it'd still want some better explanation of what's going on.

OK, I'll discuss more with Hajime, at this point I think we might want
to focus on getting the basics merged first. In either case we will
make sure to have it properly explained.