From mboxrd@z Thu Jan 1 00:00:00 1970 References: <20160614155740.GE23680@hermes.click-hack.org> <57602CA7.5050108@siemens.com> <20160614170443.GH23680@hermes.click-hack.org> <57603EB6.9040605@siemens.com> <20160614184022.GI23680@hermes.click-hack.org> From: Jan Kiszka Message-ID: <57605662.9060106@siemens.com> Date: Tue, 14 Jun 2016 21:09:22 +0200 MIME-Version: 1.0 In-Reply-To: <20160614184022.GI23680@hermes.click-hack.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Xenomai] [Xenomai-git] Jan Kiszka : cobalt/kernel: Allow to restart clock_nanosleep and select after signal processing List-Id: Discussions about the Xenomai project List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Gilles Chanteperdrix Cc: xenomai@xenomai.org On 2016-06-14 20:40, Gilles Chanteperdrix wrote: > On Tue, Jun 14, 2016 at 07:28:22PM +0200, Jan Kiszka wrote: >> On 2016-06-14 19:04, Gilles Chanteperdrix wrote: >>> On Tue, Jun 14, 2016 at 06:11:19PM +0200, Jan Kiszka wrote: >>>> On 2016-06-14 17:57, Gilles Chanteperdrix wrote: >>>>> On Fri, May 27, 2016 at 08:36:43AM +0200, git repository hosting wrote: >>>>>> diff --git a/kernel/cobalt/include/asm-generic/xenomai/wrappers.h b/kernel/cobalt/include/asm-generic/xenomai/wrappers.h >>>>>> index 060ce85..0f9ab14 100644 >>>>>> --- a/kernel/cobalt/include/asm-generic/xenomai/wrappers.h >>>>>> +++ b/kernel/cobalt/include/asm-generic/xenomai/wrappers.h >>>>>> @@ -133,4 +133,10 @@ devm_hwmon_device_register_with_groups(struct device *dev, const char *name, >>>>>> #error "Xenomai/cobalt requires Linux kernel 3.10 or above" >>>>>> #endif /* < 3.10 */ >>>>>> >>>>>> +#if LINUX_VERSION_CODE < KERNEL_VERSION(4,0,0) >>>>>> +#define cobalt_get_restart_block(p) (&task_thread_info(p)->restart_block) >>>>>> +#else >>>>>> +#define cobalt_get_restart_block(p) (&(p)->restart_block) >>>>>> +#endif >>>>>> + >>>>> >>>>> This is bad. First off as explained in the comment heading >>>>> wrappers.h the wrappers are ordered by kernel version and the most >>>>> recent is first. Second, no other wrapper has a #else clause, the >>>>> idea is that we want to be able to remove some old wrappers from >>>>> time to time, and removing the #if completely should be enough. >>>>> Obviously, if you put a #else, this does not work. I agree that in >>>>> that case it is going to be hard, but please try anyway... >>>> >>>> I'm open for concrete ideas. >>> >>> There are examples in wrapper.h, with COBALT_BACKPORT. Maybe this >>> can be used? The thing is that wrappers.h is supposed to contain >>> implementation of new services for older kernels; >>> cobalt_get_restart_block does not fit that definition. >> >> Generally a good pattern, but the problem here is that the location of >> the structure completely changed. If there were an accessor for the >> struct in newer kernels, we could use and warp that. But also upstream >> does direct access, and I had to introduce this particular accessor for >> backporting purposes. > > Yes, OK, but for instance, you could put the definition for old > kernels in wrappers.h then in another header (like, says, > ancillaries.h), include wrappers.h and if the symbol has not be > defined in wrappers.h, define it in ancillaries.h. This way, it will > continue to work if we carelessly remove the #ifdef in wrappers.h. > This will also make things a bit easier to maintain if the way to > access the restart_block changes again. Basically, constructs like: > > #if version > 4.0 > definition 1 > #elif version > 3.99 > definition 2 > #else > definition 3 > #endif > > Will not work with the new wrappers.h organization. Because we want > a #ifdef for each kernel version. So that they can be put in > descending kernel versions order. OK, can try this. > > >>> Ok, but much of the code you add runs under nklock, I am not sure I >> >> Just as the code tells you: everything that was under nklock before, >> still is (+ some additional time calculation for nanosleep), and >> everything that was not is also not with the patch. >> >> What are your concerns? > > My concern is: the XNRESTART bit or the restart block contents are > not going to be modified under the feet of the thread calling > nanosleep, these are synchronously set by that same thread, so there > is no reason, a priori, for that test and the timeout calculation to > be under nklock. Yes, we should be able to pull the nanosleep prologue safely out of nklock. The epilogue requires protection for xntimer_get_timeout_stopped. > And in fact, I do not see why this could not be > done prior to calling nanosleep, by the cobalt core. Which function should handle these rather specific things instead? Sorry, cannot follow yet. > > Another solution, is for the kernel side to only implement an > absolute delay (so only clock_nanosleep(TIMER_ABSTIME)) and have the > user-space take care of the rest. This way, the syscall can be > restarted without having to recalculate its arguments, and in fact, > the computation of the remaining time in case the syscall is > interrupted by a signal will be more accurate if done in user-space. > The same goes for select. Since in 3.x we do not have to support the > POSIX API in kernel space, there is no reason to want to implement > POSIX completely in kernel space. That is a different story. We can refactor such things on top later on if there is a value. > > This, of course, means an ABI change, so would only go in 3.1.x, but > is not changing the behaviour of nanosleep with signals an ABI > change already? An ABI fix - to comply with POSIX again. > >>> see the use for this. Also, this code reuses an existing status bit >>> (XNLBALERT) for a different purpose, I foresee unforeseen >> >> No, it introduces a new status bit XNRESTART - or did I mess something >> up? Argh, I did: That XNLBALERT should be XNRESTART, of course. >> Copy&pasted from the header, working fine in practice, there I didn't >> notice. Will fix. > > BTW, XNRESTART is a bad name. It does not say at all what this bit > means. The art of finding a telling but not too lengthy name. Would XNSYSRESTART be sufficient already? Again: proposals welcome. > >> >>> consequences with that. Also the case you handle is a corner case, >>> and with your patch, handling that corner case ends-up taking the >>> majority of the nanosleep code. And finally, maybe some changes >>> could be moved in the I-pipe patch, if that helps (reading the >>> commit message, I believe it could help). >>> >>> So, all and all, I do not think this patch is acceptable as is. >>> >> >> Again, I'm open for better, concrete, less invasive, whatever design >> proposals. >> >> The code solves a generic problem that we share with Linux in a way that >> is very similar to Linux, even reuses a lot of Linux so that our I-pipe >> patch doesn't grow, and that even per arch. It may not look very >> friendly, but neither does the Linux code. > > For such a core, generic feature as syscall restarting, I see no > problem with moving some stuff to the I-pipe patch. I do: patch maintenance. If we did our own stuff, there would be much more to do, e.g. hook into each architectural signal injection path. This comes for free now. Jan -- Siemens AG, Corporate Technology, CT RDA ITP SES-DE Corporate Competence Center Embedded Linux