Re: [Xenomai-core] [PATCH] xnpipe: fix state tracking of Linux side
Philippe Gerum wrote: > Jan Kiszka wrote: >> Jan Kiszka wrote: >>> The state tracking of Linux tasks queue on xnpipe events was fairly >>> broken. An easy way to corrupt some xnpipe_state_t object was to kill a >>> Linux task blocked on opening a /dev/rtpX device (this left the state >>> object queued in xnpipe_sleep, and hell broke loose on next queuing). >>> >>> Another problem that popped up after reworking xnpipe queuing was a >>> stalled XNPIPE_USER_CONN after receiving a signal in xnpipe_open. >>> >>> The following patch addresses both issues, appears to run fine so far >>> (at least the test case no longer triggers), and also cleans up some >>> redundant nklock acquisitions/releases. Nevertheless, I may miss some >>> corner case that might have triggered the original design. So please >>> check carefully. >> While fighting over a single bit elsewhere, let's not forget this open >> issue: > > I did not forget this issue. It is close to the top of stack now. > > Customer asks for a solution of the hard lock-up he faced, and >> /me would like to know if this fix comes with no obvious regressions. >> > > The patch looks sane, but I want to run a few tests that have been developed > over time to chase bugs in that code, before giving the patch a go. ETA: > tomorrow. > Great, thanks. Jan -- Siemens AG, Corporate Technology, CT SE 2 Corporate Competence Center Embedded Linux ___ Xenomai-core mailing list Xenomai-core@gna.org https://mail.gna.org/listinfo/xenomai-core
Re: [Xenomai-core] [PATCH] xnpipe: fix state tracking of Linux side
Jan Kiszka wrote:
> Jan Kiszka wrote:
>> The state tracking of Linux tasks queue on xnpipe events was fairly
>> broken. An easy way to corrupt some xnpipe_state_t object was to kill a
>> Linux task blocked on opening a /dev/rtpX device (this left the state
>> object queued in xnpipe_sleep, and hell broke loose on next queuing).
>>
>> Another problem that popped up after reworking xnpipe queuing was a
>> stalled XNPIPE_USER_CONN after receiving a signal in xnpipe_open.
>>
>> The following patch addresses both issues, appears to run fine so far
>> (at least the test case no longer triggers), and also cleans up some
>> redundant nklock acquisitions/releases. Nevertheless, I may miss some
>> corner case that might have triggered the original design. So please
>> check carefully.
>
> While fighting over a single bit elsewhere, let's not forget this open
> issue:
I did not forget this issue. It is close to the top of stack now.
Customer asks for a solution of the hard lock-up he faced, and
> /me would like to know if this fix comes with no obvious regressions.
>
The patch looks sane, but I want to run a few tests that have been developed
over time to chase bugs in that code, before giving the patch a go. ETA:
tomorrow.
> TIA,
> Jan
>
>> ---
>> ksrc/nucleus/pipe.c | 64
>> +++-
>> 1 file changed, 24 insertions(+), 40 deletions(-)
>>
>> Index: b/ksrc/nucleus/pipe.c
>> ===
>> --- a/ksrc/nucleus/pipe.c
>> +++ b/ksrc/nucleus/pipe.c
>> @@ -88,38 +88,39 @@ static inline void xnpipe_minor_free(int
>>
>> static inline void xnpipe_enqueue_wait(xnpipe_state_t *state, int mask)
>> {
>> -spl_t s;
>> -
>> -xnlock_get_irqsave(&nklock, s);
>> -
>> if (state->wcount++ == 0)
>> appendq(&xnpipe_sleepq, &state->slink);
>>
>> __setbits(state->status, mask);
>> +}
>>
>> -xnlock_put_irqrestore(&nklock, s);
>> +static inline void xnpipe_dequeue_wait(xnpipe_state_t *state, int mask)
>> +{
>> +if (testbits(state->status, mask)) {
>> +if (--state->wcount == 0)
>> +removeq(&xnpipe_sleepq, &state->slink);
>> +__clrbits(state->status, mask);
>> +}
>> }
>>
>> /* Must be entered with nklock held. */
>> -#define xnpipe_wait(__state, __mask, __s, __cond) \
>> -({ \
>> -wait_queue_head_t *__waitq; \
>> -DEFINE_WAIT(__wait);\
>> -int __sigpending; \
>> -\
>> -if ((__mask) & XNPIPE_USER_WREAD) \
>> -__waitq = &(__state)->readq;\
>> -else\
>> -__waitq = &(__state)->syncq;\
>> -\
>> -xnpipe_enqueue_wait(__state, __mask); \
>> -xnlock_put_irqrestore(&nklock, __s);\
>> +#define xnpipe_wait(__state, __mask, __s, __cond) \
>> +({ \
>> +wait_queue_head_t *__waitq; \
>> +DEFINE_WAIT(__wait);\
>> +int __sigpending; \
>> +\
>> +if ((__mask) & XNPIPE_USER_WREAD) \
>> +__waitq = &(__state)->readq;\
>> +else\
>> +__waitq = &(__state)->syncq;\
>> +\
>> +xnpipe_enqueue_wait(__state, __mask); \
>> +xnlock_put_irqrestore(&nklock, __s);\
>> \
>> prepare_to_wait_exclusive(__waitq, &__wait, TASK_INTERRUPTIBLE);\
>> \
>> -if (__cond) \
>> -xnpipe_dequeue_wait(__state, __mask); \
>> -else\
>> +if (!__cond)\
>> schedule(); \
>> \
>> finish_wait(__waitq, &__wait); \
>> @@ -127,25 +128,11 @@ static inline void xnpipe_enqueue_wait(x
>> \
>> /* Restore the interrupt state initially set by the caller. */ \
>>
Re: [Xenomai-core] [PATCH] xnpipe: fix state tracking of Linux side
Jan Kiszka wrote:
> The state tracking of Linux tasks queue on xnpipe events was fairly
> broken. An easy way to corrupt some xnpipe_state_t object was to kill a
> Linux task blocked on opening a /dev/rtpX device (this left the state
> object queued in xnpipe_sleep, and hell broke loose on next queuing).
>
> Another problem that popped up after reworking xnpipe queuing was a
> stalled XNPIPE_USER_CONN after receiving a signal in xnpipe_open.
>
> The following patch addresses both issues, appears to run fine so far
> (at least the test case no longer triggers), and also cleans up some
> redundant nklock acquisitions/releases. Nevertheless, I may miss some
> corner case that might have triggered the original design. So please
> check carefully.
While fighting over a single bit elsewhere, let's not forget this open
issue: Customer asks for a solution of the hard lock-up he faced, and
/me would like to know if this fix comes with no obvious regressions.
TIA,
Jan
> ---
> ksrc/nucleus/pipe.c | 64
> +++-
> 1 file changed, 24 insertions(+), 40 deletions(-)
>
> Index: b/ksrc/nucleus/pipe.c
> ===
> --- a/ksrc/nucleus/pipe.c
> +++ b/ksrc/nucleus/pipe.c
> @@ -88,38 +88,39 @@ static inline void xnpipe_minor_free(int
>
> static inline void xnpipe_enqueue_wait(xnpipe_state_t *state, int mask)
> {
> - spl_t s;
> -
> - xnlock_get_irqsave(&nklock, s);
> -
> if (state->wcount++ == 0)
> appendq(&xnpipe_sleepq, &state->slink);
>
> __setbits(state->status, mask);
> +}
>
> - xnlock_put_irqrestore(&nklock, s);
> +static inline void xnpipe_dequeue_wait(xnpipe_state_t *state, int mask)
> +{
> + if (testbits(state->status, mask)) {
> + if (--state->wcount == 0)
> + removeq(&xnpipe_sleepq, &state->slink);
> + __clrbits(state->status, mask);
> + }
> }
>
> /* Must be entered with nklock held. */
> -#define xnpipe_wait(__state, __mask, __s, __cond)\
> -({ \
> - wait_queue_head_t *__waitq; \
> - DEFINE_WAIT(__wait);\
> - int __sigpending; \
> - \
> - if ((__mask) & XNPIPE_USER_WREAD) \
> - __waitq = &(__state)->readq;\
> - else\
> - __waitq = &(__state)->syncq;\
> - \
> - xnpipe_enqueue_wait(__state, __mask); \
> - xnlock_put_irqrestore(&nklock, __s);\
> +#define xnpipe_wait(__state, __mask, __s, __cond)\
> +({ \
> + wait_queue_head_t *__waitq; \
> + DEFINE_WAIT(__wait);\
> + int __sigpending; \
> + \
> + if ((__mask) & XNPIPE_USER_WREAD) \
> + __waitq = &(__state)->readq;\
> + else\
> + __waitq = &(__state)->syncq;\
> + \
> + xnpipe_enqueue_wait(__state, __mask); \
> + xnlock_put_irqrestore(&nklock, __s);\
> \
> prepare_to_wait_exclusive(__waitq, &__wait, TASK_INTERRUPTIBLE);\
> \
> - if (__cond) \
> - xnpipe_dequeue_wait(__state, __mask); \
> - else\
> + if (!__cond)\
> schedule(); \
> \
> finish_wait(__waitq, &__wait); \
> @@ -127,25 +128,11 @@ static inline void xnpipe_enqueue_wait(x
> \
> /* Restore the interrupt state initially set by the caller. */ \
> xnlock_get_irqsave(&nklock, __s); \
> + xnpipe_dequeue_wait(__state, __mask); \
> \
> __sigpending; \
> })
>
>
[Xenomai-core] [PATCH] xnpipe: fix state tracking of Linux side
The state tracking of Linux tasks queue on xnpipe events was fairly
broken. An easy way to corrupt some xnpipe_state_t object was to kill a
Linux task blocked on opening a /dev/rtpX device (this left the state
object queued in xnpipe_sleep, and hell broke loose on next queuing).
Another problem that popped up after reworking xnpipe queuing was a
stalled XNPIPE_USER_CONN after receiving a signal in xnpipe_open.
The following patch addresses both issues, appears to run fine so far
(at least the test case no longer triggers), and also cleans up some
redundant nklock acquisitions/releases. Nevertheless, I may miss some
corner case that might have triggered the original design. So please
check carefully.
Jan
---
ksrc/nucleus/pipe.c | 64 +++-
1 file changed, 24 insertions(+), 40 deletions(-)
Index: b/ksrc/nucleus/pipe.c
===
--- a/ksrc/nucleus/pipe.c
+++ b/ksrc/nucleus/pipe.c
@@ -88,38 +88,39 @@ static inline void xnpipe_minor_free(int
static inline void xnpipe_enqueue_wait(xnpipe_state_t *state, int mask)
{
- spl_t s;
-
- xnlock_get_irqsave(&nklock, s);
-
if (state->wcount++ == 0)
appendq(&xnpipe_sleepq, &state->slink);
__setbits(state->status, mask);
+}
- xnlock_put_irqrestore(&nklock, s);
+static inline void xnpipe_dequeue_wait(xnpipe_state_t *state, int mask)
+{
+ if (testbits(state->status, mask)) {
+ if (--state->wcount == 0)
+ removeq(&xnpipe_sleepq, &state->slink);
+ __clrbits(state->status, mask);
+ }
}
/* Must be entered with nklock held. */
-#define xnpipe_wait(__state, __mask, __s, __cond) \
-({ \
- wait_queue_head_t *__waitq; \
- DEFINE_WAIT(__wait);\
- int __sigpending; \
- \
- if ((__mask) & XNPIPE_USER_WREAD) \
- __waitq = &(__state)->readq;\
- else\
- __waitq = &(__state)->syncq;\
- \
- xnpipe_enqueue_wait(__state, __mask); \
- xnlock_put_irqrestore(&nklock, __s);\
+#define xnpipe_wait(__state, __mask, __s, __cond) \
+({ \
+ wait_queue_head_t *__waitq; \
+ DEFINE_WAIT(__wait);\
+ int __sigpending; \
+ \
+ if ((__mask) & XNPIPE_USER_WREAD) \
+ __waitq = &(__state)->readq;\
+ else\
+ __waitq = &(__state)->syncq;\
+ \
+ xnpipe_enqueue_wait(__state, __mask); \
+ xnlock_put_irqrestore(&nklock, __s);\
\
prepare_to_wait_exclusive(__waitq, &__wait, TASK_INTERRUPTIBLE);\
\
- if (__cond) \
- xnpipe_dequeue_wait(__state, __mask); \
- else\
+ if (!__cond)\
schedule(); \
\
finish_wait(__waitq, &__wait); \
@@ -127,25 +128,11 @@ static inline void xnpipe_enqueue_wait(x
\
/* Restore the interrupt state initially set by the caller. */ \
xnlock_get_irqsave(&nklock, __s); \
+ xnpipe_dequeue_wait(__state, __mask); \
\
__sigpending; \
})
-static inline void xnpipe_dequeue_wait(xnpipe_state_t *state, int mask)
-{
- spl_t s;
-
- xnlock_get_irqsave(&nklock, s);
-
- if (testbits(state->status, mask)) {
- if (--state->wcount == 0)
- removeq(&xnpipe_sleepq, &state->slink);
- __clrbits(st
