Re: [Xenomai-core] [PATCH, RFC] nucleus:shared irq, draft v.2

[Jan Kiszka <jan.kiszka@domain.hid>]

[-- Attachment #1: Type: text/plain, Size: 4616 bytes --]

Dmitry Adamushko wrote:
> Hi everybody,
> 
> I'm presenting it one last time as a draft, so it's a right time to give all
> the remarks concerning design/implementation issues for all interested
> parties.
> 
> Any feedback is wellcome.
> 
> --
> Best regards,
> Dmitry Adamushko
> 

My mail client unfortunately refused to inline your patches, so I have to:

> --- intr-cvs.c	2005-12-02 19:56:20.000000000 +0100
> +++ intr.c	2006-01-09 21:26:44.000000000 +0100
[...]
> @@ -204,11 +242,66 @@
>  int xnintr_attach (xnintr_t *intr,
>  		   void *cookie)
>  {
> +    rthal_irq_handler_t irq_handler;
> +    unsigned irq = intr->irq;
> +    xnshared_irq_t *shirq;
> +    int err = 0;
> +    spl_t s;
> +
>      intr->hits = 0;
>      intr->cookie = cookie;
> -    return xnarch_hook_irq(intr->irq,&xnintr_irq_handler,intr->iack,intr);
> +
> +    xnlock_get_irqsave(&nklock,s);
> +
> +    irq_handler = rthal_irq_handler(&rthal_domain,irq);
> +    
> +    if (irq_handler)
> +	{
> +	xnintr_t *old;
> +	shirq = rthal_irq_cookie(&rthal_domain,irq);
> +	
> +	if (irq_handler != &xnintr_irq_handler)
> +	    {
> +	    err = -EBUSY;
> +	    goto unlock_and_exit;
> +	    }
> +	
> +	old = link2intr(getheadq(&shirq->handlers));
> +
> +	if (!(old->flags & intr->flags & XN_ISR_SHARED))
> +	    {
> +	    err = -EBUSY;
> +	    goto unlock_and_exit;
> +	    }
> +	
> +	appendq(&shirq->handlers,&intr->link);
> +	}
> +    else
> +	{
> +	shirq = xnmalloc(sizeof(*shirq));

Why not allocating that piece of memory before taking the global lock?
If you don't need it, you can drop it again afterward. If you need but
the returned pointer is NULL, you can still check at the same place you
do now. Just an idea to avoid complex functions inside the global lock
for new xeno-code.

> +	
> +	if (!shirq)
> +	    {
> +	    err = -ENOMEM;
> +	    goto unlock_and_exit;
> +	    }
> +
> +	initq(&shirq->handlers);
> +	appendq(&shirq->handlers,&intr->link);
> +
> +	err = xnarch_hook_irq(irq,&xnintr_irq_handler,intr->iack,shirq);
> +	
> +	if (err)
> +	    xnfree(shirq);
> +	}
> +
> +unlock_and_exit:
> +    
> +    xnlock_put_irqrestore(&nklock,s);
> +    return err;
>  }
>  
> +
>  /*! 
>   * \fn int xnintr_detach (xnintr_t *intr)
>   * \brief Detach an interrupt object.
> @@ -241,7 +334,32 @@
>  int xnintr_detach (xnintr_t *intr)
>  
>  {
> -    return xnarch_release_irq(intr->irq);
> +    unsigned irq = intr->irq;
> +    xnshared_irq_t *shirq;
> +    int cleanup = 0;
> +    int err = 0;
> +    spl_t s;
> +
> +    xnlock_get_irqsave(&nklock,s);
> +
> +    shirq = rthal_irq_cookie(&rthal_domain,irq);
> +
> +    removeq(&shirq->handlers,&intr->link);
> +    
> +    if (!countq(&shirq->handlers))
> +	{
> +	err = xnarch_release_irq(irq);
> +	cleanup = 1;
> +	}
> +
> +    xnlock_put_irqrestore(&nklock,s);
> +
> +    xnintr_shirq_spin(shirq);
> +
> +    if (cleanup)
> +	xnfree(shirq);
> +
> +    return err;
>  }
>  
>  /*! 
> @@ -350,17 +468,45 @@

I guess here starts the new IRQ handler. BTW, diff -p helps a lot when
reading patches as a human being and not as a patch tool. ;)

>  
>  {
>      xnsched_t *sched = xnpod_current_sched();
> -    xnintr_t *intr = (xnintr_t *)cookie;
> -    int s;
> +    xnshared_irq_t *shirq = (xnshared_irq_t *)cookie;
> +    xnholder_t holder;
> +    spl_t flags;
> +    int s = 0;
>  
>      xnarch_memory_barrier();
>  
>      xnltt_log_event(xeno_ev_ienter,irq);
>  
> +    xnlock_get_irqsave(&nklock,flags);
> +
> +    shirq = rthal_irq_cookie(&rthal_domain,irq);
> +    xnintr_shirq_lock(shirq);
> +
> +    xnlock_put_irqrestore(&nklock,flags);

Why "_irqsave" in IRQ context?

Regarding this locking isssue in general, I first had some RCU-mechanism
in mind to avoid the reader-side lock in the IRQ handler. But as IRQs
typically touch some nucleus service with its own nklock-acquire anyway,
optimising this here does not seem to be worth the effort now. As long
as we have the global lock, it's ok and much simpler I think.

> +    
> +    if (!shirq)
> +	{
> +	xnintr_shirq_unlock(shirq);
> +	xnltt_log_event(xeno_ev_iexit,irq);
> +    	return;
> +	}
> +    
>      ++sched->inesting;
> -    s = intr->isr(intr);
> +    
> +    holder = getheadq(&shirq->handlers);
> +    
> +    while (holder)
> +	{
> +	xnintr_t *intr = link2intr(holder);
> +	holder = nextq(&shirq->handlers,holder);
> +
> +	s |= intr->isr(intr);
> +	++intr->hits;
> +	}
> +
> +    xnintr_shirq_unlock(shirq);
> +
>      --sched->inesting;
> -    ++intr->hits;
>  
>      if (s & XN_ISR_ENABLE)
>  	xnarch_enable_irq(irq);

Looking forward to see this in Xenomai - at letting some real tests run
with it. :)

Jan

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 254 bytes --]