* Re: [Xenomai] [Xenomai-git] Jan Kiszka : cobalt/kernel: Fix locking for xnthread info manipulations
[not found] <E1Z8UN2-0004n4-Gc@sd-51317.xenomai.org>
@ 2015-06-26 16:01 ` Gilles Chanteperdrix
2015-06-26 20:17 ` Jan Kiszka
0 siblings, 1 reply; 9+ messages in thread
From: Gilles Chanteperdrix @ 2015-06-26 16:01 UTC (permalink / raw)
To: xenomai; +Cc: xenomai-git
On Fri, Jun 26, 2015 at 04:12:44PM +0200, git repository hosting wrote:
> Module: xenomai-jki
> Branch: for-forge
> Commit: 67a64e164b737759cc171d3b04b05770e1450cb6
> URL: http://git.xenomai.org/?p=xenomai-jki.git;a=commit;h=67a64e164b737759cc171d3b04b05770e1450cb6
>
> Author: Jan Kiszka <jan.kiszka@siemens.com>
> Date: Fri Jun 26 15:11:42 2015 +0200
>
> cobalt/kernel: Fix locking for xnthread info manipulations
>
> nklock must be held when manipulating bits of xnthread::info. Not all
> callsites of xnthread_set/clear_info follow this rule so far, directly
> or indirectly, fix them (and possibly some other races along this).
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>
> ---
>
> include/cobalt/kernel/rtdm/driver.h | 8 ++++----
> kernel/cobalt/posix/syscall.c | 5 +++++
> kernel/cobalt/synch.c | 2 ++
> kernel/cobalt/thread.c | 5 +++++
> 4 files changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/include/cobalt/kernel/rtdm/driver.h b/include/cobalt/kernel/rtdm/driver.h
> index c14198b..de476ca 100644
> --- a/include/cobalt/kernel/rtdm/driver.h
> +++ b/include/cobalt/kernel/rtdm/driver.h
> @@ -553,7 +553,7 @@ static inline void rtdm_lock_get(rtdm_lock_t *lock)
> {
> XENO_BUG_ON(COBALT, !spltest());
> spin_lock(lock);
> - __xnsched_lock();
> + xnsched_lock();
> }
>
> /**
> @@ -566,7 +566,7 @@ static inline void rtdm_lock_get(rtdm_lock_t *lock)
> static inline void rtdm_lock_put(rtdm_lock_t *lock)
> {
> spin_unlock(lock);
> - __xnsched_unlock();
> + xnsched_unlock();
> }
>
> /**
> @@ -584,7 +584,7 @@ static inline rtdm_lockctx_t __rtdm_lock_get_irqsave(rtdm_lock_t *lock)
>
> context = ipipe_test_and_stall_head();
> spin_lock(lock);
> - __xnsched_lock();
> + xnsched_lock();
>
> return context;
> }
> @@ -603,7 +603,7 @@ static inline
> void rtdm_lock_put_irqrestore(rtdm_lock_t *lock, rtdm_lockctx_t context)
> {
> spin_unlock(lock);
> - __xnsched_unlock();
> + xnsched_unlock();
> ipipe_restore_head(context);
> }
These changes are not without risk: is not there a risk of deadlock
if one thread calls rtdm_lock_get without holding the nklock and
another calls it while holding it?
--
Gilles.
https://click-hack.org
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Xenomai] [Xenomai-git] Jan Kiszka : cobalt/kernel: Fix locking for xnthread info manipulations
2015-06-26 16:01 ` [Xenomai] [Xenomai-git] Jan Kiszka : cobalt/kernel: Fix locking for xnthread info manipulations Gilles Chanteperdrix
@ 2015-06-26 20:17 ` Jan Kiszka
2015-06-27 10:03 ` Gilles Chanteperdrix
0 siblings, 1 reply; 9+ messages in thread
From: Jan Kiszka @ 2015-06-26 20:17 UTC (permalink / raw)
To: Gilles Chanteperdrix, xenomai; +Cc: xenomai-git
On 2015-06-26 18:01, Gilles Chanteperdrix wrote:
> On Fri, Jun 26, 2015 at 04:12:44PM +0200, git repository hosting wrote:
>> Module: xenomai-jki
>> Branch: for-forge
>> Commit: 67a64e164b737759cc171d3b04b05770e1450cb6
>> URL: http://git.xenomai.org/?p=xenomai-jki.git;a=commit;h=67a64e164b737759cc171d3b04b05770e1450cb6
>>
>> Author: Jan Kiszka <jan.kiszka@siemens.com>
>> Date: Fri Jun 26 15:11:42 2015 +0200
>>
>> cobalt/kernel: Fix locking for xnthread info manipulations
>>
>> nklock must be held when manipulating bits of xnthread::info. Not all
>> callsites of xnthread_set/clear_info follow this rule so far, directly
>> or indirectly, fix them (and possibly some other races along this).
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> ---
>>
>> include/cobalt/kernel/rtdm/driver.h | 8 ++++----
>> kernel/cobalt/posix/syscall.c | 5 +++++
>> kernel/cobalt/synch.c | 2 ++
>> kernel/cobalt/thread.c | 5 +++++
>> 4 files changed, 16 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/cobalt/kernel/rtdm/driver.h b/include/cobalt/kernel/rtdm/driver.h
>> index c14198b..de476ca 100644
>> --- a/include/cobalt/kernel/rtdm/driver.h
>> +++ b/include/cobalt/kernel/rtdm/driver.h
>> @@ -553,7 +553,7 @@ static inline void rtdm_lock_get(rtdm_lock_t *lock)
>> {
>> XENO_BUG_ON(COBALT, !spltest());
>> spin_lock(lock);
>> - __xnsched_lock();
>> + xnsched_lock();
>> }
>>
>> /**
>> @@ -566,7 +566,7 @@ static inline void rtdm_lock_get(rtdm_lock_t *lock)
>> static inline void rtdm_lock_put(rtdm_lock_t *lock)
>> {
>> spin_unlock(lock);
>> - __xnsched_unlock();
>> + xnsched_unlock();
>> }
>>
>> /**
>> @@ -584,7 +584,7 @@ static inline rtdm_lockctx_t __rtdm_lock_get_irqsave(rtdm_lock_t *lock)
>>
>> context = ipipe_test_and_stall_head();
>> spin_lock(lock);
>> - __xnsched_lock();
>> + xnsched_lock();
>>
>> return context;
>> }
>> @@ -603,7 +603,7 @@ static inline
>> void rtdm_lock_put_irqrestore(rtdm_lock_t *lock, rtdm_lockctx_t context)
>> {
>> spin_unlock(lock);
>> - __xnsched_unlock();
>> + xnsched_unlock();
>> ipipe_restore_head(context);
>> }
>
> These changes are not without risk: is not there a risk of deadlock
> if one thread calls rtdm_lock_get without holding the nklock and
> another calls it while holding it?
That could happen, although the "normal" pattern does not involve
explicit nklock acquisition if nesting is used, rather rtdm_lock before
calling nklock-acquiring services. Not a reasonable ordering either, but
common practice now.
We could also try to rework the whole scheduler lock so that it avoids
unsafe thread fields, ideally converting it to something more
lightweight as Linux' preempt_disable/enable().
BTW, xnsched has those two types of flags fields: status requires
nklock, lflags must only be locally modified, thus are fine with
interrupt disabling as protection.
Jan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: OpenPGP digital signature
URL: <http://xenomai.org/pipermail/xenomai/attachments/20150626/e986a9f2/attachment.sig>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Xenomai] [Xenomai-git] Jan Kiszka : cobalt/kernel: Fix locking for xnthread info manipulations
2015-06-26 20:17 ` Jan Kiszka
@ 2015-06-27 10:03 ` Gilles Chanteperdrix
2015-06-27 10:50 ` Philippe Gerum
0 siblings, 1 reply; 9+ messages in thread
From: Gilles Chanteperdrix @ 2015-06-27 10:03 UTC (permalink / raw)
To: Jan Kiszka; +Cc: xenomai-git, xenomai
On Fri, Jun 26, 2015 at 10:17:21PM +0200, Jan Kiszka wrote:
> On 2015-06-26 18:01, Gilles Chanteperdrix wrote:
> > On Fri, Jun 26, 2015 at 04:12:44PM +0200, git repository hosting wrote:
> >> Module: xenomai-jki
> >> Branch: for-forge
> >> Commit: 67a64e164b737759cc171d3b04b05770e1450cb6
> >> URL: http://git.xenomai.org/?p=xenomai-jki.git;a=commit;h=67a64e164b737759cc171d3b04b05770e1450cb6
> >>
> >> Author: Jan Kiszka <jan.kiszka@siemens.com>
> >> Date: Fri Jun 26 15:11:42 2015 +0200
> >>
> >> cobalt/kernel: Fix locking for xnthread info manipulations
> >>
> >> nklock must be held when manipulating bits of xnthread::info. Not all
> >> callsites of xnthread_set/clear_info follow this rule so far, directly
> >> or indirectly, fix them (and possibly some other races along this).
> >>
> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >>
> >> ---
> >>
> >> include/cobalt/kernel/rtdm/driver.h | 8 ++++----
> >> kernel/cobalt/posix/syscall.c | 5 +++++
> >> kernel/cobalt/synch.c | 2 ++
> >> kernel/cobalt/thread.c | 5 +++++
> >> 4 files changed, 16 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/include/cobalt/kernel/rtdm/driver.h b/include/cobalt/kernel/rtdm/driver.h
> >> index c14198b..de476ca 100644
> >> --- a/include/cobalt/kernel/rtdm/driver.h
> >> +++ b/include/cobalt/kernel/rtdm/driver.h
> >> @@ -553,7 +553,7 @@ static inline void rtdm_lock_get(rtdm_lock_t *lock)
> >> {
> >> XENO_BUG_ON(COBALT, !spltest());
> >> spin_lock(lock);
> >> - __xnsched_lock();
> >> + xnsched_lock();
> >> }
> >>
> >> /**
> >> @@ -566,7 +566,7 @@ static inline void rtdm_lock_get(rtdm_lock_t *lock)
> >> static inline void rtdm_lock_put(rtdm_lock_t *lock)
> >> {
> >> spin_unlock(lock);
> >> - __xnsched_unlock();
> >> + xnsched_unlock();
> >> }
> >>
> >> /**
> >> @@ -584,7 +584,7 @@ static inline rtdm_lockctx_t __rtdm_lock_get_irqsave(rtdm_lock_t *lock)
> >>
> >> context = ipipe_test_and_stall_head();
> >> spin_lock(lock);
> >> - __xnsched_lock();
> >> + xnsched_lock();
> >>
> >> return context;
> >> }
> >> @@ -603,7 +603,7 @@ static inline
> >> void rtdm_lock_put_irqrestore(rtdm_lock_t *lock, rtdm_lockctx_t context)
> >> {
> >> spin_unlock(lock);
> >> - __xnsched_unlock();
> >> + xnsched_unlock();
> >> ipipe_restore_head(context);
> >> }
> >
> > These changes are not without risk: is not there a risk of deadlock
> > if one thread calls rtdm_lock_get without holding the nklock and
> > another calls it while holding it?
>
> That could happen, although the "normal" pattern does not involve
> explicit nklock acquisition if nesting is used, rather rtdm_lock before
> calling nklock-acquiring services. Not a reasonable ordering either, but
> common practice now.
>
> We could also try to rework the whole scheduler lock so that it avoids
> unsafe thread fields, ideally converting it to something more
> lightweight as Linux' preempt_disable/enable().
>
> BTW, xnsched has those two types of flags fields: status requires
> nklock, lflags must only be locally modified, thus are fine with
> interrupt disabling as protection.
the sched lock uses lflags. The only point where we need to know
that a thread had the scheduler lock, is when it is switched in
after having voluntarily suspended. So, maybe a solution would be to
set that state bit only when a thread with the scheduler lock
voluntarily suspends. That would make the scheduler lock quite
lightweight (refcount + scheduler lflags bit).
--
Gilles.
https://click-hack.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: not available
URL: <http://xenomai.org/pipermail/xenomai/attachments/20150627/a5c80cd2/attachment.sig>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Xenomai] [Xenomai-git] Jan Kiszka : cobalt/kernel: Fix locking for xnthread info manipulations
2015-06-27 10:03 ` Gilles Chanteperdrix
@ 2015-06-27 10:50 ` Philippe Gerum
2015-06-27 10:59 ` Gilles Chanteperdrix
0 siblings, 1 reply; 9+ messages in thread
From: Philippe Gerum @ 2015-06-27 10:50 UTC (permalink / raw)
To: Gilles Chanteperdrix, Jan Kiszka; +Cc: xenomai-git, xenomai
On 06/27/2015 12:03 PM, Gilles Chanteperdrix wrote:
> On Fri, Jun 26, 2015 at 10:17:21PM +0200, Jan Kiszka wrote:
>> On 2015-06-26 18:01, Gilles Chanteperdrix wrote:
>>> On Fri, Jun 26, 2015 at 04:12:44PM +0200, git repository hosting wrote:
>>>> Module: xenomai-jki
>>>> Branch: for-forge
>>>> Commit: 67a64e164b737759cc171d3b04b05770e1450cb6
>>>> URL: http://git.xenomai.org/?p=xenomai-jki.git;a=commit;h=67a64e164b737759cc171d3b04b05770e1450cb6
>>>>
>>>> Author: Jan Kiszka <jan.kiszka@siemens.com>
>>>> Date: Fri Jun 26 15:11:42 2015 +0200
>>>>
>>>> cobalt/kernel: Fix locking for xnthread info manipulations
>>>>
>>>> nklock must be held when manipulating bits of xnthread::info. Not all
>>>> callsites of xnthread_set/clear_info follow this rule so far, directly
>>>> or indirectly, fix them (and possibly some other races along this).
>>>>
>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>
>>>> ---
>>>>
>>>> include/cobalt/kernel/rtdm/driver.h | 8 ++++----
>>>> kernel/cobalt/posix/syscall.c | 5 +++++
>>>> kernel/cobalt/synch.c | 2 ++
>>>> kernel/cobalt/thread.c | 5 +++++
>>>> 4 files changed, 16 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/include/cobalt/kernel/rtdm/driver.h b/include/cobalt/kernel/rtdm/driver.h
>>>> index c14198b..de476ca 100644
>>>> --- a/include/cobalt/kernel/rtdm/driver.h
>>>> +++ b/include/cobalt/kernel/rtdm/driver.h
>>>> @@ -553,7 +553,7 @@ static inline void rtdm_lock_get(rtdm_lock_t *lock)
>>>> {
>>>> XENO_BUG_ON(COBALT, !spltest());
>>>> spin_lock(lock);
>>>> - __xnsched_lock();
>>>> + xnsched_lock();
>>>> }
>>>>
>>>> /**
>>>> @@ -566,7 +566,7 @@ static inline void rtdm_lock_get(rtdm_lock_t *lock)
>>>> static inline void rtdm_lock_put(rtdm_lock_t *lock)
>>>> {
>>>> spin_unlock(lock);
>>>> - __xnsched_unlock();
>>>> + xnsched_unlock();
>>>> }
>>>>
>>>> /**
>>>> @@ -584,7 +584,7 @@ static inline rtdm_lockctx_t __rtdm_lock_get_irqsave(rtdm_lock_t *lock)
>>>>
>>>> context = ipipe_test_and_stall_head();
>>>> spin_lock(lock);
>>>> - __xnsched_lock();
>>>> + xnsched_lock();
>>>>
>>>> return context;
>>>> }
>>>> @@ -603,7 +603,7 @@ static inline
>>>> void rtdm_lock_put_irqrestore(rtdm_lock_t *lock, rtdm_lockctx_t context)
>>>> {
>>>> spin_unlock(lock);
>>>> - __xnsched_unlock();
>>>> + xnsched_unlock();
>>>> ipipe_restore_head(context);
>>>> }
>>>
>>> These changes are not without risk: is not there a risk of deadlock
>>> if one thread calls rtdm_lock_get without holding the nklock and
>>> another calls it while holding it?
>>
>> That could happen, although the "normal" pattern does not involve
>> explicit nklock acquisition if nesting is used, rather rtdm_lock before
>> calling nklock-acquiring services. Not a reasonable ordering either, but
>> common practice now.
>>
>> We could also try to rework the whole scheduler lock so that it avoids
>> unsafe thread fields, ideally converting it to something more
>> lightweight as Linux' preempt_disable/enable().
>>
>> BTW, xnsched has those two types of flags fields: status requires
>> nklock, lflags must only be locally modified, thus are fine with
>> interrupt disabling as protection.
>
> the sched lock uses lflags. The only point where we need to know
> that a thread had the scheduler lock, is when it is switched in
> after having voluntarily suspended. So, maybe a solution would be to
> set that state bit only when a thread with the scheduler lock
> voluntarily suspends. That would make the scheduler lock quite
> lightweight (refcount + scheduler lflags bit).
>
- XNINLOCK is overkill, it is only tested for skipping the resched
procedure as an optimization from xnsched_run(). As we may access
sched->lflags in that situation, we may also access
sched->curr->lock_count to figure this out.
- XNLOCK and xnthread->lock_count are redundant when combined to track
the lock state internally. This is part of what makes xnsched_lock() and
friends uselessly complex (the potential for deadlocks you mentioned
about the RTDM locking changes is definitely a concern, dropping the
requirement for grabbing the nklock there is very desirable in that
sense). However, we still need XNLOCK as part of the ABI to convey the
state bit information between kernel and userland. But, referring to
curr->lock_count should be enough for implementing that logic in the
scheduler code.
- there are only two places where XNLOCK is accessed within the state
word of a potentially non-current thread, i.e: from
__xnthread_set_schedparam(), and xnthread_set_mode(). But XNLOCK is only
tested there, and testing thread->lock_count would work exactly the same
way.
So, yes, I agree that such code direly needs simplification.
--
Philippe.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Xenomai] [Xenomai-git] Jan Kiszka : cobalt/kernel: Fix locking for xnthread info manipulations
2015-06-27 10:50 ` Philippe Gerum
@ 2015-06-27 10:59 ` Gilles Chanteperdrix
2015-06-27 13:05 ` Philippe Gerum
0 siblings, 1 reply; 9+ messages in thread
From: Gilles Chanteperdrix @ 2015-06-27 10:59 UTC (permalink / raw)
To: Philippe Gerum; +Cc: xenomai-git, Jan Kiszka, xenomai
On Sat, Jun 27, 2015 at 12:50:35PM +0200, Philippe Gerum wrote:
> On 06/27/2015 12:03 PM, Gilles Chanteperdrix wrote:
> > On Fri, Jun 26, 2015 at 10:17:21PM +0200, Jan Kiszka wrote:
> >> On 2015-06-26 18:01, Gilles Chanteperdrix wrote:
> >>> On Fri, Jun 26, 2015 at 04:12:44PM +0200, git repository hosting wrote:
> >>>> Module: xenomai-jki
> >>>> Branch: for-forge
> >>>> Commit: 67a64e164b737759cc171d3b04b05770e1450cb6
> >>>> URL: http://git.xenomai.org/?p=xenomai-jki.git;a=commit;h=67a64e164b737759cc171d3b04b05770e1450cb6
> >>>>
> >>>> Author: Jan Kiszka <jan.kiszka@siemens.com>
> >>>> Date: Fri Jun 26 15:11:42 2015 +0200
> >>>>
> >>>> cobalt/kernel: Fix locking for xnthread info manipulations
> >>>>
> >>>> nklock must be held when manipulating bits of xnthread::info. Not all
> >>>> callsites of xnthread_set/clear_info follow this rule so far, directly
> >>>> or indirectly, fix them (and possibly some other races along this).
> >>>>
> >>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >>>>
> >>>> ---
> >>>>
> >>>> include/cobalt/kernel/rtdm/driver.h | 8 ++++----
> >>>> kernel/cobalt/posix/syscall.c | 5 +++++
> >>>> kernel/cobalt/synch.c | 2 ++
> >>>> kernel/cobalt/thread.c | 5 +++++
> >>>> 4 files changed, 16 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/include/cobalt/kernel/rtdm/driver.h b/include/cobalt/kernel/rtdm/driver.h
> >>>> index c14198b..de476ca 100644
> >>>> --- a/include/cobalt/kernel/rtdm/driver.h
> >>>> +++ b/include/cobalt/kernel/rtdm/driver.h
> >>>> @@ -553,7 +553,7 @@ static inline void rtdm_lock_get(rtdm_lock_t *lock)
> >>>> {
> >>>> XENO_BUG_ON(COBALT, !spltest());
> >>>> spin_lock(lock);
> >>>> - __xnsched_lock();
> >>>> + xnsched_lock();
> >>>> }
> >>>>
> >>>> /**
> >>>> @@ -566,7 +566,7 @@ static inline void rtdm_lock_get(rtdm_lock_t *lock)
> >>>> static inline void rtdm_lock_put(rtdm_lock_t *lock)
> >>>> {
> >>>> spin_unlock(lock);
> >>>> - __xnsched_unlock();
> >>>> + xnsched_unlock();
> >>>> }
> >>>>
> >>>> /**
> >>>> @@ -584,7 +584,7 @@ static inline rtdm_lockctx_t __rtdm_lock_get_irqsave(rtdm_lock_t *lock)
> >>>>
> >>>> context = ipipe_test_and_stall_head();
> >>>> spin_lock(lock);
> >>>> - __xnsched_lock();
> >>>> + xnsched_lock();
> >>>>
> >>>> return context;
> >>>> }
> >>>> @@ -603,7 +603,7 @@ static inline
> >>>> void rtdm_lock_put_irqrestore(rtdm_lock_t *lock, rtdm_lockctx_t context)
> >>>> {
> >>>> spin_unlock(lock);
> >>>> - __xnsched_unlock();
> >>>> + xnsched_unlock();
> >>>> ipipe_restore_head(context);
> >>>> }
> >>>
> >>> These changes are not without risk: is not there a risk of deadlock
> >>> if one thread calls rtdm_lock_get without holding the nklock and
> >>> another calls it while holding it?
> >>
> >> That could happen, although the "normal" pattern does not involve
> >> explicit nklock acquisition if nesting is used, rather rtdm_lock before
> >> calling nklock-acquiring services. Not a reasonable ordering either, but
> >> common practice now.
> >>
> >> We could also try to rework the whole scheduler lock so that it avoids
> >> unsafe thread fields, ideally converting it to something more
> >> lightweight as Linux' preempt_disable/enable().
> >>
> >> BTW, xnsched has those two types of flags fields: status requires
> >> nklock, lflags must only be locally modified, thus are fine with
> >> interrupt disabling as protection.
> >
> > the sched lock uses lflags. The only point where we need to know
> > that a thread had the scheduler lock, is when it is switched in
> > after having voluntarily suspended. So, maybe a solution would be to
> > set that state bit only when a thread with the scheduler lock
> > voluntarily suspends. That would make the scheduler lock quite
> > lightweight (refcount + scheduler lflags bit).
> >
>
> - XNINLOCK is overkill, it is only tested for skipping the resched
> procedure as an optimization from xnsched_run(). As we may access
> sched->lflags in that situation, we may also access
> sched->curr->lock_count to figure this out.
>
> - XNLOCK and xnthread->lock_count are redundant when combined to track
> the lock state internally. This is part of what makes xnsched_lock() and
> friends uselessly complex (the potential for deadlocks you mentioned
> about the RTDM locking changes is definitely a concern, dropping the
> requirement for grabbing the nklock there is very desirable in that
> sense). However, we still need XNLOCK as part of the ABI to convey the
> state bit information between kernel and userland. But, referring to
> curr->lock_count should be enough for implementing that logic in the
> scheduler code.
>
> - there are only two places where XNLOCK is accessed within the state
> word of a potentially non-current thread, i.e: from
> __xnthread_set_schedparam(), and xnthread_set_mode(). But XNLOCK is only
> tested there, and testing thread->lock_count would work exactly the same
> way.
>
> So, yes, I agree that such code direly needs simplification.
Ok. I was thinking about using XNINLOCK in place of XNLOCK, but
using lock_count may be even simpler.
The nklock seems to also be needed for handling the XNLBALERT bit.
--
Gilles.
https://click-hack.org
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Xenomai] [Xenomai-git] Jan Kiszka : cobalt/kernel: Fix locking for xnthread info manipulations
2015-06-27 10:59 ` Gilles Chanteperdrix
@ 2015-06-27 13:05 ` Philippe Gerum
2015-06-30 18:40 ` Jan Kiszka
0 siblings, 1 reply; 9+ messages in thread
From: Philippe Gerum @ 2015-06-27 13:05 UTC (permalink / raw)
To: Gilles Chanteperdrix; +Cc: xenomai-git, Jan Kiszka, xenomai
On 06/27/2015 12:59 PM, Gilles Chanteperdrix wrote:
> On Sat, Jun 27, 2015 at 12:50:35PM +0200, Philippe Gerum wrote:
>> On 06/27/2015 12:03 PM, Gilles Chanteperdrix wrote:
>>> On Fri, Jun 26, 2015 at 10:17:21PM +0200, Jan Kiszka wrote:
>>>> On 2015-06-26 18:01, Gilles Chanteperdrix wrote:
>>>>> On Fri, Jun 26, 2015 at 04:12:44PM +0200, git repository hosting wrote:
>>>>>> Module: xenomai-jki
>>>>>> Branch: for-forge
>>>>>> Commit: 67a64e164b737759cc171d3b04b05770e1450cb6
>>>>>> URL: http://git.xenomai.org/?p=xenomai-jki.git;a=commit;h=67a64e164b737759cc171d3b04b05770e1450cb6
>>>>>>
>>>>>> Author: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>> Date: Fri Jun 26 15:11:42 2015 +0200
>>>>>>
>>>>>> cobalt/kernel: Fix locking for xnthread info manipulations
>>>>>>
>>>>>> nklock must be held when manipulating bits of xnthread::info. Not all
>>>>>> callsites of xnthread_set/clear_info follow this rule so far, directly
>>>>>> or indirectly, fix them (and possibly some other races along this).
>>>>>>
>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>
>>>>>> ---
>>>>>>
>>>>>> include/cobalt/kernel/rtdm/driver.h | 8 ++++----
>>>>>> kernel/cobalt/posix/syscall.c | 5 +++++
>>>>>> kernel/cobalt/synch.c | 2 ++
>>>>>> kernel/cobalt/thread.c | 5 +++++
>>>>>> 4 files changed, 16 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/include/cobalt/kernel/rtdm/driver.h b/include/cobalt/kernel/rtdm/driver.h
>>>>>> index c14198b..de476ca 100644
>>>>>> --- a/include/cobalt/kernel/rtdm/driver.h
>>>>>> +++ b/include/cobalt/kernel/rtdm/driver.h
>>>>>> @@ -553,7 +553,7 @@ static inline void rtdm_lock_get(rtdm_lock_t *lock)
>>>>>> {
>>>>>> XENO_BUG_ON(COBALT, !spltest());
>>>>>> spin_lock(lock);
>>>>>> - __xnsched_lock();
>>>>>> + xnsched_lock();
>>>>>> }
>>>>>>
>>>>>> /**
>>>>>> @@ -566,7 +566,7 @@ static inline void rtdm_lock_get(rtdm_lock_t *lock)
>>>>>> static inline void rtdm_lock_put(rtdm_lock_t *lock)
>>>>>> {
>>>>>> spin_unlock(lock);
>>>>>> - __xnsched_unlock();
>>>>>> + xnsched_unlock();
>>>>>> }
>>>>>>
>>>>>> /**
>>>>>> @@ -584,7 +584,7 @@ static inline rtdm_lockctx_t __rtdm_lock_get_irqsave(rtdm_lock_t *lock)
>>>>>>
>>>>>> context = ipipe_test_and_stall_head();
>>>>>> spin_lock(lock);
>>>>>> - __xnsched_lock();
>>>>>> + xnsched_lock();
>>>>>>
>>>>>> return context;
>>>>>> }
>>>>>> @@ -603,7 +603,7 @@ static inline
>>>>>> void rtdm_lock_put_irqrestore(rtdm_lock_t *lock, rtdm_lockctx_t context)
>>>>>> {
>>>>>> spin_unlock(lock);
>>>>>> - __xnsched_unlock();
>>>>>> + xnsched_unlock();
>>>>>> ipipe_restore_head(context);
>>>>>> }
>>>>>
>>>>> These changes are not without risk: is not there a risk of deadlock
>>>>> if one thread calls rtdm_lock_get without holding the nklock and
>>>>> another calls it while holding it?
>>>>
>>>> That could happen, although the "normal" pattern does not involve
>>>> explicit nklock acquisition if nesting is used, rather rtdm_lock before
>>>> calling nklock-acquiring services. Not a reasonable ordering either, but
>>>> common practice now.
>>>>
>>>> We could also try to rework the whole scheduler lock so that it avoids
>>>> unsafe thread fields, ideally converting it to something more
>>>> lightweight as Linux' preempt_disable/enable().
>>>>
>>>> BTW, xnsched has those two types of flags fields: status requires
>>>> nklock, lflags must only be locally modified, thus are fine with
>>>> interrupt disabling as protection.
>>>
>>> the sched lock uses lflags. The only point where we need to know
>>> that a thread had the scheduler lock, is when it is switched in
>>> after having voluntarily suspended. So, maybe a solution would be to
>>> set that state bit only when a thread with the scheduler lock
>>> voluntarily suspends. That would make the scheduler lock quite
>>> lightweight (refcount + scheduler lflags bit).
>>>
>>
>> - XNINLOCK is overkill, it is only tested for skipping the resched
>> procedure as an optimization from xnsched_run(). As we may access
>> sched->lflags in that situation, we may also access
>> sched->curr->lock_count to figure this out.
>>
>> - XNLOCK and xnthread->lock_count are redundant when combined to track
>> the lock state internally. This is part of what makes xnsched_lock() and
>> friends uselessly complex (the potential for deadlocks you mentioned
>> about the RTDM locking changes is definitely a concern, dropping the
>> requirement for grabbing the nklock there is very desirable in that
>> sense). However, we still need XNLOCK as part of the ABI to convey the
>> state bit information between kernel and userland. But, referring to
>> curr->lock_count should be enough for implementing that logic in the
>> scheduler code.
>>
>> - there are only two places where XNLOCK is accessed within the state
>> word of a potentially non-current thread, i.e: from
>> __xnthread_set_schedparam(), and xnthread_set_mode(). But XNLOCK is only
>> tested there, and testing thread->lock_count would work exactly the same
>> way.
>>
>> So, yes, I agree that such code direly needs simplification.
>
> Ok. I was thinking about using XNINLOCK in place of XNLOCK, but
> using lock_count may be even simpler.
>
> The nklock seems to also be needed for handling the XNLBALERT bit.
>
Correct, but I'm looking for a way to move this elsewhere.
--
Philippe.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Xenomai] [Xenomai-git] Jan Kiszka : cobalt/kernel: Fix locking for xnthread info manipulations
2015-06-27 13:05 ` Philippe Gerum
@ 2015-06-30 18:40 ` Jan Kiszka
2015-06-30 18:55 ` Philippe Gerum
0 siblings, 1 reply; 9+ messages in thread
From: Jan Kiszka @ 2015-06-30 18:40 UTC (permalink / raw)
To: Philippe Gerum, Gilles Chanteperdrix; +Cc: xenomai
On 2015-06-27 15:05, Philippe Gerum wrote:
> On 06/27/2015 12:59 PM, Gilles Chanteperdrix wrote:
>> On Sat, Jun 27, 2015 at 12:50:35PM +0200, Philippe Gerum wrote:
>>> On 06/27/2015 12:03 PM, Gilles Chanteperdrix wrote:
>>>> On Fri, Jun 26, 2015 at 10:17:21PM +0200, Jan Kiszka wrote:
>>>>> On 2015-06-26 18:01, Gilles Chanteperdrix wrote:
>>>>>> On Fri, Jun 26, 2015 at 04:12:44PM +0200, git repository hosting wrote:
>>>>>>> Module: xenomai-jki
>>>>>>> Branch: for-forge
>>>>>>> Commit: 67a64e164b737759cc171d3b04b05770e1450cb6
>>>>>>> URL: http://git.xenomai.org/?p=xenomai-jki.git;a=commit;h=67a64e164b737759cc171d3b04b05770e1450cb6
>>>>>>>
>>>>>>> Author: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>> Date: Fri Jun 26 15:11:42 2015 +0200
>>>>>>>
>>>>>>> cobalt/kernel: Fix locking for xnthread info manipulations
>>>>>>>
>>>>>>> nklock must be held when manipulating bits of xnthread::info. Not all
>>>>>>> callsites of xnthread_set/clear_info follow this rule so far, directly
>>>>>>> or indirectly, fix them (and possibly some other races along this).
>>>>>>>
>>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>>
>>>>>>> ---
>>>>>>>
>>>>>>> include/cobalt/kernel/rtdm/driver.h | 8 ++++----
>>>>>>> kernel/cobalt/posix/syscall.c | 5 +++++
>>>>>>> kernel/cobalt/synch.c | 2 ++
>>>>>>> kernel/cobalt/thread.c | 5 +++++
>>>>>>> 4 files changed, 16 insertions(+), 4 deletions(-)
>>>>>>>
>>>>>>> diff --git a/include/cobalt/kernel/rtdm/driver.h b/include/cobalt/kernel/rtdm/driver.h
>>>>>>> index c14198b..de476ca 100644
>>>>>>> --- a/include/cobalt/kernel/rtdm/driver.h
>>>>>>> +++ b/include/cobalt/kernel/rtdm/driver.h
>>>>>>> @@ -553,7 +553,7 @@ static inline void rtdm_lock_get(rtdm_lock_t *lock)
>>>>>>> {
>>>>>>> XENO_BUG_ON(COBALT, !spltest());
>>>>>>> spin_lock(lock);
>>>>>>> - __xnsched_lock();
>>>>>>> + xnsched_lock();
>>>>>>> }
>>>>>>>
>>>>>>> /**
>>>>>>> @@ -566,7 +566,7 @@ static inline void rtdm_lock_get(rtdm_lock_t *lock)
>>>>>>> static inline void rtdm_lock_put(rtdm_lock_t *lock)
>>>>>>> {
>>>>>>> spin_unlock(lock);
>>>>>>> - __xnsched_unlock();
>>>>>>> + xnsched_unlock();
>>>>>>> }
>>>>>>>
>>>>>>> /**
>>>>>>> @@ -584,7 +584,7 @@ static inline rtdm_lockctx_t __rtdm_lock_get_irqsave(rtdm_lock_t *lock)
>>>>>>>
>>>>>>> context = ipipe_test_and_stall_head();
>>>>>>> spin_lock(lock);
>>>>>>> - __xnsched_lock();
>>>>>>> + xnsched_lock();
>>>>>>>
>>>>>>> return context;
>>>>>>> }
>>>>>>> @@ -603,7 +603,7 @@ static inline
>>>>>>> void rtdm_lock_put_irqrestore(rtdm_lock_t *lock, rtdm_lockctx_t context)
>>>>>>> {
>>>>>>> spin_unlock(lock);
>>>>>>> - __xnsched_unlock();
>>>>>>> + xnsched_unlock();
>>>>>>> ipipe_restore_head(context);
>>>>>>> }
>>>>>>
>>>>>> These changes are not without risk: is not there a risk of deadlock
>>>>>> if one thread calls rtdm_lock_get without holding the nklock and
>>>>>> another calls it while holding it?
>>>>>
>>>>> That could happen, although the "normal" pattern does not involve
>>>>> explicit nklock acquisition if nesting is used, rather rtdm_lock before
>>>>> calling nklock-acquiring services. Not a reasonable ordering either, but
>>>>> common practice now.
>>>>>
>>>>> We could also try to rework the whole scheduler lock so that it avoids
>>>>> unsafe thread fields, ideally converting it to something more
>>>>> lightweight as Linux' preempt_disable/enable().
>>>>>
>>>>> BTW, xnsched has those two types of flags fields: status requires
>>>>> nklock, lflags must only be locally modified, thus are fine with
>>>>> interrupt disabling as protection.
>>>>
>>>> the sched lock uses lflags. The only point where we need to know
>>>> that a thread had the scheduler lock, is when it is switched in
>>>> after having voluntarily suspended. So, maybe a solution would be to
>>>> set that state bit only when a thread with the scheduler lock
>>>> voluntarily suspends. That would make the scheduler lock quite
>>>> lightweight (refcount + scheduler lflags bit).
>>>>
>>>
>>> - XNINLOCK is overkill, it is only tested for skipping the resched
>>> procedure as an optimization from xnsched_run(). As we may access
>>> sched->lflags in that situation, we may also access
>>> sched->curr->lock_count to figure this out.
>>>
>>> - XNLOCK and xnthread->lock_count are redundant when combined to track
>>> the lock state internally. This is part of what makes xnsched_lock() and
>>> friends uselessly complex (the potential for deadlocks you mentioned
>>> about the RTDM locking changes is definitely a concern, dropping the
>>> requirement for grabbing the nklock there is very desirable in that
>>> sense). However, we still need XNLOCK as part of the ABI to convey the
>>> state bit information between kernel and userland. But, referring to
>>> curr->lock_count should be enough for implementing that logic in the
>>> scheduler code.
>>>
>>> - there are only two places where XNLOCK is accessed within the state
>>> word of a potentially non-current thread, i.e: from
>>> __xnthread_set_schedparam(), and xnthread_set_mode(). But XNLOCK is only
>>> tested there, and testing thread->lock_count would work exactly the same
>>> way.
>>>
>>> So, yes, I agree that such code direly needs simplification.
>>
>> Ok. I was thinking about using XNINLOCK in place of XNLOCK, but
>> using lock_count may be even simpler.
>>
>> The nklock seems to also be needed for handling the XNLBALERT bit.
>>
>
> Correct, but I'm looking for a way to move this elsewhere.
>
BTW, let me know how we should proceed with this issue. Are you looking
into the scheduler lock? I could split off the RTDM thing from the rest
if those bits are fine.
Jan
--
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Xenomai] [Xenomai-git] Jan Kiszka : cobalt/kernel: Fix locking for xnthread info manipulations
2015-06-30 18:40 ` Jan Kiszka
@ 2015-06-30 18:55 ` Philippe Gerum
0 siblings, 0 replies; 9+ messages in thread
From: Philippe Gerum @ 2015-06-30 18:55 UTC (permalink / raw)
To: Jan Kiszka, Gilles Chanteperdrix; +Cc: xenomai
On 06/30/2015 08:40 PM, Jan Kiszka wrote:
> On 2015-06-27 15:05, Philippe Gerum wrote:
>> On 06/27/2015 12:59 PM, Gilles Chanteperdrix wrote:
>>> On Sat, Jun 27, 2015 at 12:50:35PM +0200, Philippe Gerum wrote:
>>>> On 06/27/2015 12:03 PM, Gilles Chanteperdrix wrote:
>>>>> On Fri, Jun 26, 2015 at 10:17:21PM +0200, Jan Kiszka wrote:
>>>>>> On 2015-06-26 18:01, Gilles Chanteperdrix wrote:
>>>>>>> On Fri, Jun 26, 2015 at 04:12:44PM +0200, git repository hosting wrote:
>>>>>>>> Module: xenomai-jki
>>>>>>>> Branch: for-forge
>>>>>>>> Commit: 67a64e164b737759cc171d3b04b05770e1450cb6
>>>>>>>> URL: http://git.xenomai.org/?p=xenomai-jki.git;a=commit;h=67a64e164b737759cc171d3b04b05770e1450cb6
>>>>>>>>
>>>>>>>> Author: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>>> Date: Fri Jun 26 15:11:42 2015 +0200
>>>>>>>>
>>>>>>>> cobalt/kernel: Fix locking for xnthread info manipulations
>>>>>>>>
>>>>>>>> nklock must be held when manipulating bits of xnthread::info. Not all
>>>>>>>> callsites of xnthread_set/clear_info follow this rule so far, directly
>>>>>>>> or indirectly, fix them (and possibly some other races along this).
>>>>>>>>
>>>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>>>
>>>>>>>> ---
>>>>>>>>
>>>>>>>> include/cobalt/kernel/rtdm/driver.h | 8 ++++----
>>>>>>>> kernel/cobalt/posix/syscall.c | 5 +++++
>>>>>>>> kernel/cobalt/synch.c | 2 ++
>>>>>>>> kernel/cobalt/thread.c | 5 +++++
>>>>>>>> 4 files changed, 16 insertions(+), 4 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/include/cobalt/kernel/rtdm/driver.h b/include/cobalt/kernel/rtdm/driver.h
>>>>>>>> index c14198b..de476ca 100644
>>>>>>>> --- a/include/cobalt/kernel/rtdm/driver.h
>>>>>>>> +++ b/include/cobalt/kernel/rtdm/driver.h
>>>>>>>> @@ -553,7 +553,7 @@ static inline void rtdm_lock_get(rtdm_lock_t *lock)
>>>>>>>> {
>>>>>>>> XENO_BUG_ON(COBALT, !spltest());
>>>>>>>> spin_lock(lock);
>>>>>>>> - __xnsched_lock();
>>>>>>>> + xnsched_lock();
>>>>>>>> }
>>>>>>>>
>>>>>>>> /**
>>>>>>>> @@ -566,7 +566,7 @@ static inline void rtdm_lock_get(rtdm_lock_t *lock)
>>>>>>>> static inline void rtdm_lock_put(rtdm_lock_t *lock)
>>>>>>>> {
>>>>>>>> spin_unlock(lock);
>>>>>>>> - __xnsched_unlock();
>>>>>>>> + xnsched_unlock();
>>>>>>>> }
>>>>>>>>
>>>>>>>> /**
>>>>>>>> @@ -584,7 +584,7 @@ static inline rtdm_lockctx_t __rtdm_lock_get_irqsave(rtdm_lock_t *lock)
>>>>>>>>
>>>>>>>> context = ipipe_test_and_stall_head();
>>>>>>>> spin_lock(lock);
>>>>>>>> - __xnsched_lock();
>>>>>>>> + xnsched_lock();
>>>>>>>>
>>>>>>>> return context;
>>>>>>>> }
>>>>>>>> @@ -603,7 +603,7 @@ static inline
>>>>>>>> void rtdm_lock_put_irqrestore(rtdm_lock_t *lock, rtdm_lockctx_t context)
>>>>>>>> {
>>>>>>>> spin_unlock(lock);
>>>>>>>> - __xnsched_unlock();
>>>>>>>> + xnsched_unlock();
>>>>>>>> ipipe_restore_head(context);
>>>>>>>> }
>>>>>>>
>>>>>>> These changes are not without risk: is not there a risk of deadlock
>>>>>>> if one thread calls rtdm_lock_get without holding the nklock and
>>>>>>> another calls it while holding it?
>>>>>>
>>>>>> That could happen, although the "normal" pattern does not involve
>>>>>> explicit nklock acquisition if nesting is used, rather rtdm_lock before
>>>>>> calling nklock-acquiring services. Not a reasonable ordering either, but
>>>>>> common practice now.
>>>>>>
>>>>>> We could also try to rework the whole scheduler lock so that it avoids
>>>>>> unsafe thread fields, ideally converting it to something more
>>>>>> lightweight as Linux' preempt_disable/enable().
>>>>>>
>>>>>> BTW, xnsched has those two types of flags fields: status requires
>>>>>> nklock, lflags must only be locally modified, thus are fine with
>>>>>> interrupt disabling as protection.
>>>>>
>>>>> the sched lock uses lflags. The only point where we need to know
>>>>> that a thread had the scheduler lock, is when it is switched in
>>>>> after having voluntarily suspended. So, maybe a solution would be to
>>>>> set that state bit only when a thread with the scheduler lock
>>>>> voluntarily suspends. That would make the scheduler lock quite
>>>>> lightweight (refcount + scheduler lflags bit).
>>>>>
>>>>
>>>> - XNINLOCK is overkill, it is only tested for skipping the resched
>>>> procedure as an optimization from xnsched_run(). As we may access
>>>> sched->lflags in that situation, we may also access
>>>> sched->curr->lock_count to figure this out.
>>>>
>>>> - XNLOCK and xnthread->lock_count are redundant when combined to track
>>>> the lock state internally. This is part of what makes xnsched_lock() and
>>>> friends uselessly complex (the potential for deadlocks you mentioned
>>>> about the RTDM locking changes is definitely a concern, dropping the
>>>> requirement for grabbing the nklock there is very desirable in that
>>>> sense). However, we still need XNLOCK as part of the ABI to convey the
>>>> state bit information between kernel and userland. But, referring to
>>>> curr->lock_count should be enough for implementing that logic in the
>>>> scheduler code.
>>>>
>>>> - there are only two places where XNLOCK is accessed within the state
>>>> word of a potentially non-current thread, i.e: from
>>>> __xnthread_set_schedparam(), and xnthread_set_mode(). But XNLOCK is only
>>>> tested there, and testing thread->lock_count would work exactly the same
>>>> way.
>>>>
>>>> So, yes, I agree that such code direly needs simplification.
>>>
>>> Ok. I was thinking about using XNINLOCK in place of XNLOCK, but
>>> using lock_count may be even simpler.
>>>
>>> The nklock seems to also be needed for handling the XNLBALERT bit.
>>>
>>
>> Correct, but I'm looking for a way to move this elsewhere.
>>
>
> BTW, let me know how we should proceed with this issue. Are you looking
> into the scheduler lock? I could split off the RTDM thing from the rest
> if those bits are fine.
>
Yes, I'm dropping XNLOCK as a runtime bit, keeping it only for ABI
purpose between lib/cobalt and pthread_setmode_np basically. I'll push
the changes tomorrow. Next step will be to get rid of nklock for
xnsched_lock() entirely, still needs a bit more thought though.
--
Philippe.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Xenomai] [Xenomai-git] Jan Kiszka : cobalt/kernel: Fix locking for xnthread info manipulations
[not found] <E1ZG585-0002oE-L7@sd-51317.xenomai.org>
@ 2015-07-17 13:13 ` Philippe Gerum
0 siblings, 0 replies; 9+ messages in thread
From: Philippe Gerum @ 2015-07-17 13:13 UTC (permalink / raw)
To: xenomai, xenomai-git
On 07/17/2015 02:52 PM, git repository hosting wrote:
>
> diff --git a/kernel/cobalt/synch.c b/kernel/cobalt/synch.c
> index 7142fd2..ae4ceef 100644
> --- a/kernel/cobalt/synch.c
> +++ b/kernel/cobalt/synch.c
> @@ -443,7 +443,9 @@ redo:
> if (likely(h == XN_NO_HANDLE)) {
> xnsynch_set_owner(synch, curr);
> xnthread_get_resource(curr);
> + xnlock_get_irqsave(&nklock, s);
> xnthread_clear_info(curr, XNRMID | XNTIMEO | XNBREAK);
> + xnlock_put_irqrestore(&nklock, s);
> return 0;
> }
>
Instead of serializing in the fast path only for clearing those bits, we
could convert the remaining call sites to use xnsynch_acquire's return
value instead. At any rate, those info bits are meant to be tested
immediately upon return from the blocking call. I did most of the
conversions already, except __cobalt_mutex_acquire_unchecked(). Fixing
this one should allow us to drop the requirement for clearing the bits
in the info word, and the need for serializing in the same move.
> diff --git a/kernel/cobalt/thread.c b/kernel/cobalt/thread.c
> index 4cec6e7..7c8cb3a 100644
> --- a/kernel/cobalt/thread.c
> +++ b/kernel/cobalt/thread.c
> @@ -1989,6 +1989,7 @@ void xnthread_relax(int notify, int reason)
> struct task_struct *p = current;
> int cpu __maybe_unused;
> siginfo_t si;
> + spl_t s;
>
> primary_mode_only();
>
> @@ -2045,7 +2046,9 @@ void xnthread_relax(int notify, int reason)
> si.si_int = reason | sigdebug_marker;
> send_sig_info(SIGDEBUG, &si, p);
> }
> + xnlock_get_irqsave(&nklock, s);
> xnsynch_detect_claimed_relax(thread);
> + xnlock_put_irqrestore(&nklock, s);
> }
Sigh, this was a bad one, scanning two queues with preemption enabled:
scary. I would suggest to move the serialization to
xnsynch_detect_claimed_relax() directly, since that code can be
optimized out when !XENO_DEBUG(MUTEX_RELAXED).
--
Philippe.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2015-07-17 13:13 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <E1Z8UN2-0004n4-Gc@sd-51317.xenomai.org>
2015-06-26 16:01 ` [Xenomai] [Xenomai-git] Jan Kiszka : cobalt/kernel: Fix locking for xnthread info manipulations Gilles Chanteperdrix
2015-06-26 20:17 ` Jan Kiszka
2015-06-27 10:03 ` Gilles Chanteperdrix
2015-06-27 10:50 ` Philippe Gerum
2015-06-27 10:59 ` Gilles Chanteperdrix
2015-06-27 13:05 ` Philippe Gerum
2015-06-30 18:40 ` Jan Kiszka
2015-06-30 18:55 ` Philippe Gerum
[not found] <E1ZG585-0002oE-L7@sd-51317.xenomai.org>
2015-07-17 13:13 ` Philippe Gerum
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.