From mboxrd@z Thu Jan 1 00:00:00 1970 Message-ID: Date: Mon, 9 Jan 2006 22:14:27 +0200 From: Dmitry Adamushko In-Reply-To: <43C0F792.3000300@domain.hid> MIME-Version: 1.0 Content-Type: multipart/alternative; boundary="----=_Part_25763_10438636.1136837667817" References: <43B69CEC.1040606@domain.hid> <43C0F792.3000300@domain.hid> Subject: [Xenomai-core] Re: [PATCH, RFC] nucleus:shared irq and possible ipipe problem List-Id: "Xenomai life and development \(bug reports, patches, discussions\)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Philippe Gerum Cc: xenomai@xenomai.org ------=_Part_25763_10438636.1136837667817 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Content-Disposition: inline On 08/01/06, Philippe Gerum wrote: > > > [skiped] > In ipipe_sync_stage() : > > ... > > if (--cpudata->irq_pending_hits[irq] =3D=3D 0) { > > __clear_bit(rank, > > &cpudata->irq_pending_lo[level]); > > ipipe_mark_irq_delivery(ipd,irq,cpuid); > > } > > (*) > > > > // run the corresponding isr handler > > > > This code drops the irq_pending_hits to 0 before running the handler. > > > > > Doesn't it make the following code wrong ? > > > > for (_cpuid =3D 0; _cpuid < nr_cpus; _cpuid++) > > while (ipd->cpudata[_cpuid].irq_pending_hits[irq] > 0) > > cpu_relax(); > > > > > > as the handler still can be running / or even about to be run when > > ipd->cpudata[_cpuid].irq_pending_hits[irq] is already 0. > > > > We can remove the (*) code at the end of ipipe_sync_stage() so that > > irq_pending_hits[irq] may become 0 only after the corresponding isr > > handler has completed. > > > > I don't like the idea of having the interrupt bookkeeping stuff > incompletely updated while calling a handler that might sleep and/or > migrate to another CPU. > > Another option would be to check for SYNC flag; the syncer sets it > before entering the dispatch loop. It's unset for all domains but root when the corresponding irq handler is running. ipipe_sync_stage() : ... __clear_bit(IPIPE_SYNC_FLAG, &cpudata->status); ipd->irqs[irq].handler(irq, ipd->irqs[irq].cookie); __set_bit(IPIPE_SYNC_FLAG, &cpudata->status); So it doesn't help here as it is used now. Ok, so far I have built all synchronization completely on the nucleus layer= , provided that the "cookie" param is not used as it is passed to the handler but is taken via the rthal_irq_cookie() service being in a protected section. The disadvantage so far is that the recently extended adeos irq interface i= s not used as it is supposed to be (the "cookie" argument of the handler). I'll post the current version of the patch in the separate mail. -- > > Philippe. > -- Best regards, Dmitry Adamushko ------=_Part_25763_10438636.1136837667817 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Content-Disposition: inline
On 08/01/06, Philippe Gerum <rpm@xenomai.org.= org> wrote:
> [skiped]
> In ipipe_sync_stage() :
> ...
>&= nbsp;            if = (--cpudata->irq_pending_hits[irq] =3D=3D 0) {
>   &= nbsp;           &nbs= p;  __clear_bit(rank,
>           &= nbsp;           &nbs= p;  &cpudata->irq_pending_lo[level]);
>  &= nbsp;           &nbs= p;   ipipe_mark_irq_delivery(ipd,irq,cpuid);
> &n= bsp;           }
> = (*)
>
> // run the corresponding isr handler
>
> This code drops the irq_pending_hits to 0 before running t= he handler.
>

> Doesn't it make the following code wrong ?<= br>>
>     for (_cpuid =3D 0; _cpuid < nr_c= pus; _cpuid++)
>            = ; while (ipd->cpudata[_cpuid].irq_pending_hits[irq] > 0)
> = ;            &n= bsp;       cpu_relax();
>
>
> as the handler still can be running / = or even about to be run when
> ipd->cpudata[_cpuid].irq_pending_hi= ts[irq] is already 0.
>
> We can remove the (*) code at the end= of ipipe_sync_stage() so that
> irq_pending_hits[irq] may become 0 only after the corresponding is= r
> handler has completed.
>

I don't like the idea of ha= ving the interrupt bookkeeping stuff
incompletely updated while calling = a handler that might sleep and/or
migrate to another CPU.

Another option would be to check for SYN= C flag; the syncer sets it
before entering the dispatch loop.

It's unset for all domains but root when the corresponding irq handler is r= unning.

ipipe_sync_stage() :
...
            __clear_bit(IPIPE_= SYNC_FLAG, &cpudata->status);
            ipd->irqs[irq].= handler(irq, ipd->irqs[irq].cookie);
            __set_bit(IPIPE_SY= NC_FLAG, &cpudata->status);

So it doesn't help here as it is used now.

Ok, so far I have built all synchronization completely on the nucleus layer, provided that the "cookie" param is not used as it is pass= ed to the handler but is taken via the rthal_irq_cookie() service being in a protected section.

The disadvantage so far is that the recently extended adeos irq interface is not used as it is supposed to be (the "cookie" argum= ent of the handler).

I'll post the current version of the patch in the separate mail.

--
<= br>Philippe.



--
Best re= gards,
Dmitry Adamushko ------=_Part_25763_10438636.1136837667817--