From mboxrd@z Thu Jan 1 00:00:00 1970 Message-ID: Date: Mon, 30 Jan 2006 15:02:18 +0200 From: Dmitry Adamushko Subject: Re: [Xenomai-core] [BUG] racy xnshadow_harden under CONFIG_PREEMPT In-Reply-To: <43DDFD03.5030104@domain.hid> MIME-Version: 1.0 Content-Type: multipart/alternative; boundary="----=_Part_1950_11775726.1138626138069" References: <43D21144.8040005@domain.hid> <43D62B07.3030703@domain.hid> <43DDFD03.5030104@domain.hid> List-Id: "Xenomai life and development \(bug reports, patches, discussions\)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jan Kiszka Cc: xenomai@xenomai.org ------=_Part_1950_11775726.1138626138069 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Content-Disposition: inline On 30/01/06, Jan Kiszka wrote: > > Dmitry Adamushko wrote: > >>> ... > > > >> I have not checked it yet but my presupposition that something as easy > as > >> : > >>> preempt_disable() > >>> > >>> wake_up_interruptible_sync(); > >>> schedule(); > >>> > >>> preempt_enable(); > >> It's a no-go: "scheduling while atomic". One of my first attempts to > >> solve it. > > > > > > My fault. I meant the way preempt_schedule() and preempt_irq_schedule() > call > > schedule() while being non-preemptible. > > To this end, ACTIVE_PREEMPT is set up. > > The use of preempt_enable/disable() here is wrong. > > > > > > The only way to enter schedule() without being preemptible is via > >> ACTIVE_PREEMPT. But the effect of that flag should be well-known now. > >> Kind of Gordian knot. :( > > > > > > Maybe I have missed something so just for my curiosity : what does > prevent > > the use of PREEMPT_ACTIVE here? > > We don't have a "preempted while atomic" message here as it seems to be > a > > legal way to call schedule() with that flag being set up. > > When PREEMPT_ACTIVE is set, task gets /preempted/ but not removed from > the run queue - independent of its current status. Err... that's exactly the reason I have explained in my first mail for thi= s thread :) Blah.. I wish I was smoking something special before so I would point that as the reason of my forgetfulness. Actually, we could use PREEMPT_ACTIVE indeed + something else (probably another flag) to distinguish between a case when PREEMPT_ACTIVE is set by Linux and another case when it's set by xnshadow_harden(). xnshadow_harden() { struct task_struct *this_task =3D current; ... xnthread_t *thread =3D xnshadow_thread(this_task); if (!thread) return; ... gk->thread =3D thread; + add_preempt_count(PREEMPT_ACTIVE); // should be checked in schedule() + xnthread_set_flags(thread, XNATOMIC_TRANSIT); set_current_state(TASK_INTERRUPTIBLE); wake_up_interruptible_sync(&gk->waitq); + schedule(); + sub_preempt_count(PREEMPT_ACTIVE); ... } Then, something like the following code should be called from schedule() : void ipipe_transit_cleanup(struct task_struct *task, runqueue_t *rq) { xnthread_t *thread =3D xnshadow_thread(task); if (!thread) return; if (xnthread_test_flags(thread, XNATOMIC_TRANSIT)) { xnthread_clear_flags(thread, XNATOMIC_TRANSIT); deactivate_task(task, rq); } } ----- schedule.c : ... switch_count =3D &prev->nivcsw; if (prev->state && !(preempt_count() & PREEMPT_ACTIVE)) switch_count =3D &prev->nvcsw; if (unlikely((prev->state & TASK_INTERRUPTIBLE) && unlikely(signal_pending(prev)) )) prev->state =3D TASK_RUNNING; else { if (prev->state =3D=3D TASK_UNINTERRUPTIBLE) rq->nr_uninterruptible++; deactivate_task(prev, rq); } } // removes a task from the active queue if PREEMPT_ACTIVE + // XNATOMIC_TRANSIT + #ifdef CONFIG_IPIPE + ipipe_transit_cleanup(prev, rq); + #endif /* CONFIG_IPIPE */ ... Not very gracefully maybe, but could work or am I missing something important? -- Best regards, Dmitry Adamushko ------=_Part_1950_11775726.1138626138069 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Content-Disposition: inline
On 30/01/06, Jan Kiszka <jan.kiszka@domain.hid= .de> wrote:
Dmitry Adamushko wrote:
>>> ...
>
>> I have not = checked it yet but my presupposition that something as easy as
>> = :
>>> preempt_disable()
>>>
>>> wake_up= _interruptible_sync();
>>> schedule();
>>>
>>> preempt_enable= ();
>> It's a no-go: "scheduling while atomic". One of m= y first attempts to
>> solve it.
>
>
> My fault.= I meant the way preempt_schedule() and preempt_irq_schedule() call
> schedule() while being non-preemptible.
> To this end, ACTIV= E_PREEMPT is set up.
> The use of preempt_enable/disable() here is wr= ong.
>
>
> The only way to enter schedule() without being= preemptible is via
>> ACTIVE_PREEMPT. But the effect of that flag should be well-kno= wn now.
>> Kind of Gordian knot. :(
>
>
> Maybe = I have missed something so just for my curiosity : what does prevent
> the use of PREEMPT_ACTIVE here?
> We don't have a "preempte= d while atomic" message here as it seems to be a
> legal way to = call schedule() with that flag being set up.

When PREEMPT_ACTIVE is = set, task gets /preempted/ but not removed from
the run queue - independent of its current status.
Err...  that's exactly the reason I have explained in my first mail for this thread :) Blah.. I wish I was smoking something special before so I would point that as the reason of my forgetfulness.

Actually, we could use PREEMPT_ACTIVE indeed + something else (probably another flag) to distinguish between a case when PREEMPT_ACTIVE is set by Linux and another case when it's set by xnshadow_harden().

xnshadow_harden()
{
struct task_struct *this_task =3D current;
...
xnthread_t *thread =3D xnshadow_thread(this_task);

if (!thread)
    return;

...
gk->thread =3D thread;

+ add_preempt_count(PREEMPT_ACTIVE);

// should be checked in schedule()
+ xnthread_set_flags(thread, XNATOMIC_TRANSIT);

set_current_state(TASK_INTERRUPTIBLE);
wake_up_interruptible_sync(&gk->waitq);
+ schedule();

+ sub_preempt_count(PREEMPT_ACTIVE);
...
}

Then, something like the following code should be called from schedule() : =

void ipipe_transit_cleanup(struct task_struct *task, runqueue_t *rq)
{
xnthread_t *thread =3D xnshadow_thread(task);

if (!thread)
    return;

if (xnthread_test_flags(thread, XNATOMIC_TRANSIT))
    {
    xnthread_clear_flags(thread, XNATOMIC_TRANSIT);
    deactivate_task(task, rq);
    }
}

-----

schedule.c :
...
    switch_count =3D &prev->nivcsw;
    if (prev->state && !(preempt_count() & PREEMPT_ACTIVE))         switch_count = =3D &prev->nvcsw;
        if (unlikely((prev->state & TA= SK_INTERRUPTIBLE) &&
               = unlikely(signal_pending(prev))
))
            prev->state =3D= TASK_RUNNING;
        else {
            if (prev->state= =3D=3D TASK_UNINTERRUPTIBLE)
               = rq->nr_uninterruptible++;
            deactivate_task(prev, rq);
        }
    }

// removes a task from the active queue if PREEMPT_ACTIVE + // XNATOMIC_TRA= NSIT

+ #ifdef CONFIG_IPIPE
+ ipipe_transit_cleanup(prev, rq);
+ #endif /* CONFIG_IPIPE */
...

Not very gracefully maybe, but could work or am I missing something importa= nt?

--
Best regards,
Dmitry Adamushko

------=_Part_1950_11775726.1138626138069--