Jan Kiszka wrote: > Gilles Chanteperdrix wrote: >> 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. > > As are individual memory accesses. The sum makes the difference.
Ok. If you find before 2.5.3 a platform where this makes a difference of more than 5% of this platform's latency, I do not remove the __thread stuff. Deal? > Once someone starts testing mutexes or other stuff from TSD destructors, > not earlier. And this is so special that anyone of us interested in this > test case will naturally be aware of its specifics. As you said to err is human. And having a single point for handling an exceptional case is a good idea. And this shortens your patch. >> 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. > > As you may know, complete test coverage is near to impossible. That is no excuse for not making any test. > On unlucky days, I happen to send out stuff that broke while continuing to > type after the last test ran. But specifically these things were tested > in various combinations. > > It is not very polite to assume the contrary. When you send a patch every 5 minutes, it is obvious that it was not tested. No politeness involved. Everybody can check the timestamps in the mailing lists and do the math. I'm trying to avoid the > same assumption about other people as well, even when things look > differently from time to time. To err is human, to always write correct > code is divine. > > But as you already mentioned in another mail, there is room for > improvement /wrt testability. On step forward would be an automated > build and run of some git tree that we can check into. One board per > arch would be enough to avoid issues we face from time to time when we > only stress our favorite archs with new features or fixes. We have the automated build. I plan seriously to work on the automated run for 2.5.3. But it is too late for 2.5.2. Also note that I gave you access to my server where several targets are configured and usable. If you forgot the password, I can reset it. > >> 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. > > I can only say that, as a developer in this business, I trust code > having been discussed to the extreme _way_ more than stuff a single > clever person checked in even after meditating over it for weeks. I've > seen enough of this, produced by non-clever /me as well as way more > clever people in all kinds of projects. > >> 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. > > Well, you have to decide which piece to take, or to request it to be > rewritten, or to write yourself. Yes. That is the plan. Tomorrow. > >> 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. > > And that's the point of sending patches for discussion. > > It's regs->ARM_r7 == __NR_SYSCALL_BASE + __NR_MY_SYSCALL, right? If > __xn_reg_mux is supposed to be only a mux value for Xenomai, we need a > separate accessor for the Linux syscall number on all archs. or __NR_OABI_SYSCALL_BASE + __NR_MY_SYSCALL depending on CONFIG_OABI_COMPAT. > > Jan > -- Gilles. _______________________________________________ Xenomai-core mailing list [email protected] https://mail.gna.org/listinfo/xenomai-core
