Jan Kiszka wrote: > Gilles Chanteperdrix wrote: >> Jan Kiszka wrote: >>> This adds __xn_sys_drop_u_mode, a syscall to deregister the u_mode user >>> space address passed on shadow creation for the current thread. We need >>> this in order to safely shut down a thread that wants to release its >>> u_mode memory (either malloc'ed or TLS-allocated). So far, the kernel >>> might have touched this memory even after the release which caused >>> subtle corruptions. >>> >>> After we released u_mode, we can no longer make reliable decisions based >>> on it. For the fast mutex use case, we then assume the worst case, ie. >>> we enforce syscall-based acquisitions unconditionally. For the >>> assert_nrt case, we simply acquire the required information via >>> __xn_sys_current_info. >> As usual, you are firing patches too fast: >> - you are lacking the exit syscall trap. > > Don't worry, my memory is not yet _that_ bad. :) > >> - the patch is not correct and will cause kernel-space addresses to be >> passed to put_user, which probably yields bugs when the kernel is >> compiled with proper debug flags; > > Right, e.g. ARM requires set_fs() fiddling in addition. But then a simple > NULL-check is way better. Fixed. > >> - I do not see the point where the new syscall is called with __thread, >> but I am not sure it is missing, and if it is missing, you will get the >> rightfull warning when hitting the exit syscall. > > Destructor of (now unconditionally used) xeno_current_mode_key. > >> - the user-space users of the function to get current_mode are still >> cluttered with special cases to handle the invalid state. And >> illustrating nicely the deficiency of this approach, you have forgot one >> user. > > My impression is that you are still a bit biased due to TLS limitations > on ARM.
No, I think the __thread stuff thinks make C code look too much like C++ where the compiler makes things behind your back for such simple code as: void *foo = &bar; This is not what I expect from a C compiler, and I suspect a lot of C developers, including the people who advocate that C, as a simple language, and not C++ should be used for free software development. Yes, there is no gain on ARM, where this generates a function call as pthread_getspecific and causes bugs with gcc >= 4.3, but not much gain on x86 either where a function call is so cheap. And using __thread in Xenomai source code adds an awful lot of code, and what is more, an awful lot of conditionnaly compiled code, which inevitably gets less testing. So, from a maintenance point of view, this looks like a bad call. > > And nope, I didn't forget to address it, I just forgot to mention that > there is nothing to address (the torture test does not use > xeno_get_current_mode from within a TSD destructor, thus potentially > after drop_u_mode). Expect that if someone changes the test code, he will have not to forget about this. As soon as other users exist, we will have to think about updating them. > >> I am not going to make my version before tomorrow, so you have plenty of >> time to send me an at least correct version which would take into >> account all of our discussions. > > Well... what should I reply? That starting from now, you are going to test your patches before sending them. > >> Please also consider that your "patch machinegun" way of working is not >> really sane. When there are so many often untested patches to review >> coming from you, we sometime let some errors slip through. And from a >> user point of view, seeing so many buggy patches refused is not really >> reassuring. > > Please don't make me cite counter examples for issues with different > workflows we also experienced. > > The key of an open development process is open discussion, and that > ideally based on code, even if it is not yet perfect. I can't imagine > you want Xenomai being developed in closed chambers (or even just a > single one). If you are afraid of imperfect patches being posted or even > merged into some software, you shouldn't use e.g. the Linux kernel as > well. You misunderstand me. What I would like you to do is to avoid sending me patches which are a 5 minutes, untested hack, to fix something which took much longer to think and nevertheless get wrong. Because most of these patches get it inevitably wrong too. I think every sane free-software projects ask people to test their patches/pull requests before sending them, even the Linux kernel. See for instance: http://lwn.net/Articles/377091/ I am not asking you for one week. 24 hours would be enough. In fact, the mistake is partly mine too, I am replying to your patches as fast as you send them. We were going crazy tuesday with the condition variable patches, I do not think it gives a reassuring image to the users. So that at some point, I stopped reviewing your patches. I have not even read the last 8 you sent. Without going to the other extreme of a "closed door" development model, I think we can find a more reasonable trade off than the current one. Our objective now, is to put 2.5.2 in a stable state. > +substitute_linux_syscall(xnthread_t *thread, struct pt_regs *regs) > +{ > + if (unlikely(__xn_reg_mux(regs) == __NR_exit)) > + handle_shadow_exit(thread); > + Not your fault, but this does not work on ARM. Do not know if __xn_reg_mux needs fixing there or if we need to add a new macro to get the roots syscall number. -- Gilles. _______________________________________________ Xenomai-core mailing list [email protected] https://mail.gna.org/listinfo/xenomai-core
