Re: [Xenomai] [Xenomai-git] Jan Kiszka : cobalt/kernel: Allow to restart clock_nanosleep and select after signal processing

Re: [Xenomai] [Xenomai-git] Jan Kiszka : cobalt/kernel: Allow to restart clock_nanosleep and select after signal processing

[Gilles Chanteperdrix]

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...

Other than that, I see your patch modifies each syscall handler
directly, can not the result be achieved in a different way? For
instance by factoring it in the core? Relying more on Linux syscall
restart mechanism.

-- 
					    Gilles.
https://click-hack.org

Re: [Xenomai] [Xenomai-git] Jan Kiszka : cobalt/kernel: Allow to restart clock_nanosleep and select after signal processing

[Jan Kiszka]

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.

> 
> Other than that, I see your patch modifies each syscall handler
> directly, can not the result be achieved in a different way? For
> instance by factoring it in the core? Relying more on Linux syscall
> restart mechanism.

Linux does it similarly, i.e. requires modifications on a per-syscall
basis. As the logic is widely syscall-specific, I also don't see a
generic way under Xenomai either. The good news is that all cases that
Linux covers (minus futexes which we don't have) are already implemented
in the patch, thus this shouldn't spread.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [Xenomai] [Xenomai-git] Jan Kiszka : cobalt/kernel: Allow to restart clock_nanosleep and select after signal processing

[Gilles Chanteperdrix]

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.

> 
> > 
> > Other than that, I see your patch modifies each syscall handler
> > directly, can not the result be achieved in a different way? For
> > instance by factoring it in the core? Relying more on Linux syscall
> > restart mechanism.
> 
> Linux does it similarly, i.e. requires modifications on a per-syscall
> basis. As the logic is widely syscall-specific, I also don't see a
> generic way under Xenomai either. The good news is that all cases that
> Linux covers (minus futexes which we don't have) are already implemented
> in the patch, thus this shouldn't spread.

Ok, but much of the code you add runs under nklock, I am not sure I
see the use for this. Also, this code reuses an existing status bit
(XNLBALERT) for a different purpose, I foresee unforeseen
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.

-- 
					    Gilles.
https://click-hack.org

Re: [Xenomai] [Xenomai-git] Jan Kiszka : cobalt/kernel: Allow to restart clock_nanosleep and select after signal processing

[Jan Kiszka]

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.

> 
>>
>>>
>>> Other than that, I see your patch modifies each syscall handler
>>> directly, can not the result be achieved in a different way? For
>>> instance by factoring it in the core? Relying more on Linux syscall
>>> restart mechanism.
>>
>> Linux does it similarly, i.e. requires modifications on a per-syscall
>> basis. As the logic is widely syscall-specific, I also don't see a
>> generic way under Xenomai either. The good news is that all cases that
>> Linux covers (minus futexes which we don't have) are already implemented
>> in the patch, thus this shouldn't spread.
> 
> 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?

> 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.

> 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.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [Xenomai] [Xenomai-git] Jan Kiszka : cobalt/kernel: Allow to restart clock_nanosleep and select after signal processing

[Gilles Chanteperdrix]

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, 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. And in fact, I do not see why this could not be
done prior to calling nanosleep, by the cobalt core.

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.

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?

> > 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.

> 
> > 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.

-- 
					    Gilles.
https://click-hack.org

Re: [Xenomai] [Xenomai-git] Jan Kiszka : cobalt/kernel: Allow to restart clock_nanosleep and select after signal processing

[Jan Kiszka]

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

Re: [Xenomai] [Xenomai-git] Jan Kiszka : cobalt/kernel: Allow to restart clock_nanosleep and select after signal processing

[Jan Kiszka]

On 2016-06-14 21:09, Jan Kiszka wrote:
> On 2016-06-14 20:40, Gilles Chanteperdrix wrote:
>> 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.

Or XNSYSRST - to stay within the current length limit. Can lift that as
well, just need a word.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [Xenomai] [Xenomai-git] Jan Kiszka : cobalt/kernel: Allow to restart clock_nanosleep and select after signal processing

[Gilles Chanteperdrix]

On Tue, Jun 14, 2016 at 09:09:22PM +0200, Jan Kiszka wrote:
> > 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.

The point is: if an application relies on the current behaviour, we
are breaking it, and this should not happen in the current stable
branch.

> >>> 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.

Well ok, but having almost the same code that does almost the same
thing but not really in two different syscalls can be a pain to
maintain too.

-- 
					    Gilles.
https://click-hack.org

Re: [Xenomai] [Xenomai-git] Jan Kiszka : cobalt/kernel: Allow to restart clock_nanosleep and select after signal processing

[Jan Kiszka]

On 2016-06-14 21:24, Gilles Chanteperdrix wrote:
> On Tue, Jun 14, 2016 at 09:09:22PM +0200, Jan Kiszka wrote:
>>> 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.
> 
> The point is: if an application relies on the current behaviour, we
> are breaking it, and this should not happen in the current stable
> branch.

If an application relies on a POSIX violation, primarily while that
application is being debugged or otherwise externally stopped? Err, well...

Anyway, I don't mind that much if it goes to stable, helping people with
gdb, or if we keep it here until we move on to 3.1.

> 
>>>>> 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.
> 
> Well ok, but having almost the same code that does almost the same
> thing but not really in two different syscalls can be a pain to
> maintain too.

There are subtle differences (even less than under Linux), and it's only
two syscalls with a pretty high probability that they won't get more
(see Linux).

Jan

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [Xenomai] [Xenomai-git] Jan Kiszka : cobalt/kernel: Allow to restart clock_nanosleep and select after signal processing

[Gilles Chanteperdrix]

On Tue, Jun 14, 2016 at 09:43:09PM +0200, Jan Kiszka wrote:
> On 2016-06-14 21:24, Gilles Chanteperdrix wrote:
> > On Tue, Jun 14, 2016 at 09:09:22PM +0200, Jan Kiszka wrote:
> >>> 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.
> > 
> > The point is: if an application relies on the current behaviour, we
> > are breaking it, and this should not happen in the current stable
> > branch.
> 
> If an application relies on a POSIX violation, primarily while that
> application is being debugged or otherwise externally stopped? Err, well...
> 
> Anyway, I don't mind that much if it goes to stable, helping people with
> gdb, or if we keep it here until we move on to 3.1.
> 
> > 
> >>>>> 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.
> > 
> > Well ok, but having almost the same code that does almost the same
> > thing but not really in two different syscalls can be a pain to
> > maintain too.
> 
> There are subtle differences (even less than under Linux), and it's only
> two syscalls with a pretty high probability that they won't get more
> (see Linux).

That is not the issue. The issue is that if the implementation you
propose has some flaw (which, as the Xenomai project history shows,
is often the case, and I am not pointing any finger here, this is
true for the implementations I propose too), we will have to fix it
in two places, taking the "subtle differences" into account, that
is, if we remember that the same thing is implemented in two places.
A recipe for trouble. Up to now, we managed to avoid duplicating
code, I would prefer to continue doing it.

-- 
					    Gilles.
https://click-hack.org