lkml.org 
[lkml]   [2006]   [Jun]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [patch 2/5] [PREEMPT_RT] Changing interrupt handlers from running in thread to hardirq and back runtime.
From
On Sat, 3 Jun 2006, Steven Rostedt wrote:

> Disclaimer: I haven't read all your patches yet. I'm going one at a
> time to comment, and then I will probably send more emails about the
> overall response. So now, my comments are not on the big picture, but
> simple atomic views.
>
> On Fri, 2006-06-02 at 23:23 +0100, Esben Nielsen wrote:
>> This patch makes it possible to change which context the interrupt handlers
>> for each interrupt run in, hard-irq or threaded if CONFIG_PREEMPT_HARDIRQS is
>> set.
>>
>> The interface is the file
>> /proc/irq/<irq number>/threaded
>> You can read it to see what the context is now or write one of
>>
>> irq
>> fifo <rt priority>
>> rr <rt priority>
>> normal <nice value>
>> batch <nice value>
>>
>> where one of the latter makes the interrupt handler threaded.
>>
>> A replacement for request_irq(), called request_irq2(), is added. When a driver
>
> request_irq2 ... yuck! Perhaps request_irq_convertible() or something
> similar? But irq2, no way!

I know...
The problem is:
1) Don't change existing drivers.
2) The change_irq_context() call-back must be set at request_irq(), so it
it is not enough to just make a new function where the driver can set it's
the callback after the request_irq().

request_irq_convertible() might be an ok name.


>
>> uses this to install it's irq-handler it can also give a change_irq_context
>> call-back. This call-back is called whenever the irq-context is changed or
>> going to be changed. The call-back must be of the form
>>
>> int driver_change_context(int irq, void *dev_id, enum change_context_cmd cmd)
>
> Eeee, looks like a big change to make on drivers, and something that can
> keep -rt from mainline forever. But I'll see more as I read. This
> looks optional but still it will make things more complex.

It is only optional, but the function is very easy to make.

>
>>
>> where
>>
>> enum change_context_cmd {
>> IRQ_TO_HARDIRQ,
>> IRQ_CAN_THREAD,
>> IRQ_TO_THREADED
>> };
>>
>> The call-back is supposed to do the following on
>> IRQ_TO_HARDIRQ: make sure everything in the interrupt handler is non-blocking
>> or return a non-zero error code.
>> IRQ_CAN_THREAD: Return 0 if it is ok for the interrupt handler to be run in
>> thread context. Return a non-zero error code otherwise.
>> IRQ_TO_THREAD: Now the interrupt handler does run in thread context. The
>> driver can now change it's locks to being mutexes. The return
>> value is ignored as the driver already got a chance to protest
>> above.
>>
>> Index: linux-2.6.16-rt23.spin_mutex/include/linux/interrupt.h
>> ===================================================================
>> --- linux-2.6.16-rt23.spin_mutex.orig/include/linux/interrupt.h
>> +++ linux-2.6.16-rt23.spin_mutex/include/linux/interrupt.h
>> @@ -34,21 +34,38 @@ typedef int irqreturn_t;
>> #define IRQ_HANDLED (1)
>> #define IRQ_RETVAL(x) ((x) != 0)
>>
>> +enum change_context_cmd {
>> + IRQ_TO_HARDIRQ,
>> + IRQ_CAN_THREAD,
>> + IRQ_TO_THREADED
>> +};
>> +
>> struct irqaction {
>> irqreturn_t (*handler)(int, void *, struct pt_regs *);
>> unsigned long flags;
>> cpumask_t mask;
>> const char *name;
>> void *dev_id;
>> +#ifdef CONFIG_CHANGE_IRQ_CONTEXT
>> + int (*change_context)(int, void *,
>> + enum change_context_cmd);
>> +#endif
>> struct irqaction *next;
>> int irq;
>> - struct proc_dir_entry *dir, *threaded;
>> + struct proc_dir_entry *dir;
>> + struct rcu_head rcu;
>> };
>>
>> extern irqreturn_t no_action(int cpl, void *dev_id, struct pt_regs *regs);
>> extern int request_irq(unsigned int,
>> irqreturn_t (*handler)(int, void *, struct pt_regs *),
>> unsigned long, const char *, void *);
>> +extern int request_irq2(unsigned int irq,
>> + irqreturn_t (*handler)(int, void *, struct pt_regs *),
>> + unsigned long irqflags, const char * devname,
>> + void *dev_id,
>> + int (*change_context)(int, void *,
>> + enum change_context_cmd));
>> extern void free_irq(unsigned int, void *);
>>
>>
>> Index: linux-2.6.16-rt23.spin_mutex/include/linux/irq.h
>> ===================================================================
>> --- linux-2.6.16-rt23.spin_mutex.orig/include/linux/irq.h
>> +++ linux-2.6.16-rt23.spin_mutex/include/linux/irq.h
>> @@ -47,6 +47,18 @@
>> # define SA_NODELAY 0x01000000
>> #endif
>>
>> +#ifndef SA_MUST_THREAD
>> +# define SA_MUST_THREAD 0x02000000
>> +#endif
>> +
>> +/* Set this flag if the irq handler must thread under preempt-rt otherwise not
>> + */
>> +#ifdef CONFIG_PREEMPT_RT
>> +# define SA_MUST_THREAD_RT SA_MUST_THREAD
>> +#else
>> +# define SA_MUST_THREAD_RT 0
>> +#endif
>> +
>> /*
>> * IRQ types
>> */
>> @@ -147,12 +159,13 @@ struct irq_type {
>> * @chip: low level hardware access functions - comes from type
>> * @action: the irq action chain
>> * @status: status information
>> + * (protected by the spinlock )
>> * @depth: disable-depth, for nested irq_disable() calls
>> * @irq_count: stats field to detect stalled irqs
>> * @irqs_unhandled: stats field for spurious unhandled interrupts
>> * @thread: Thread pointer for threaded preemptible irq handling
>> * @wait_for_handler: Waitqueue to wait for a running preemptible handler
>> - * @lock: locking for SMP
>> + * @lock: lock around the action list
>> * @move_irq: Flag need to re-target interrupt destination
>> *
>> * Pad this out to 32 bytes for cache and indexing reasons.
>> Index: linux-2.6.16-rt23.spin_mutex/kernel/Kconfig.preempt
>> ===================================================================
>> --- linux-2.6.16-rt23.spin_mutex.orig/kernel/Kconfig.preempt
>> +++ linux-2.6.16-rt23.spin_mutex/kernel/Kconfig.preempt
>> @@ -119,6 +119,17 @@ config PREEMPT_HARDIRQS
>>
>> Say N if you are unsure.
>>
>> +config CHANGE_IRQ_CONTEXT
>> + bool "Change the irq context runtime"
>> + depends on PREEMPT_HARDIRQS && (PREEMPT_RCU || !SPIN_MUTEXES)
>> + help
>> + You can change wether the IRQ handler(s) for each IRQ number is
>> + running in hardirq context or as threaded by writing to
>> + /proc/irq/<number>/threaded
>> + If PREEMPT_RT is selected the drivers involved must be ready for it,
>> + though, or the write will fail. Remember to switch on SPIN_MUTEXES as
>> + well in that case as the drivers which are ready uses spin-mutexes.
>> +
>> config SPINLOCK_BKL
>> bool "Old-Style Big Kernel Lock"
>> depends on (PREEMPT || SMP) && !PREEMPT_RT
>> Index: linux-2.6.16-rt23.spin_mutex/kernel/irq/internals.h
>> ===================================================================
>> --- linux-2.6.16-rt23.spin_mutex.orig/kernel/irq/internals.h
>> +++ linux-2.6.16-rt23.spin_mutex/kernel/irq/internals.h
>> @@ -22,6 +22,10 @@ static inline void end_irq(irq_desc_t *d
>> }
>>
>> #ifdef CONFIG_PROC_FS
>> +#ifdef CONFIG_CHANGE_IRQ_CONTEXT
>> +extern int make_irq_nodelay(int irq, struct irq_desc *desc);
>> +extern int make_irq_threaded(int irq, struct irq_desc *desc);
>> +#endif
>> extern void register_irq_proc(unsigned int irq);
>> extern void register_handler_proc(unsigned int irq, struct irqaction *action);
>> extern void unregister_handler_proc(unsigned int irq, struct irqaction *action);
>> Index: linux-2.6.16-rt23.spin_mutex/kernel/irq/manage.c
>> ===================================================================
>> --- linux-2.6.16-rt23.spin_mutex.orig/kernel/irq/manage.c
>> +++ linux-2.6.16-rt23.spin_mutex/kernel/irq/manage.c
>> @@ -206,6 +206,153 @@ int can_request_irq(unsigned int irq, un
>> return !action;
>> }
>>
>> +
>> +#ifdef CONFIG_CHANGE_IRQ_CONTEXT
>> +int make_action_nodelay(int irq, struct irqaction *act)
>> +{
>> + unsigned long flags;
>> +
>> + if(!(act->flags & SA_MUST_THREAD)) {
>> + return 0;
>> + }
>
> Remove the brackets.
>
> Also, let me get this straight. If the action does not have
> SA_MUST_THREAD, we return? Or does it mean that if SA_MUST_THREAD is
> not set, then SA_NODELAY must already be set? If that is the case, then
> why are we not checking for SA_NODELAY here?

If SA_MUST_THREAD is not set, there is no blocking inside the action
handler and therefore it is ok to do this in hardirq aka "nodelay".
Notice SA_MUST_THREAD is set on all handlers in without SA_NODELAY under
PREEMPT_RT.

>
>> +
>> + if( act->change_context ) {
>> + int ret =
>> + act->change_context(irq, act->dev_id, IRQ_TO_HARDIRQ);
>> + if(ret) {
>> + printk(KERN_ERR "Failed to change irq handler "
>> + "for dev %s on IRQ%d to hardirq "
>> + "context (ret: %d)\n", act->name, irq, ret);
>> + return ret;
>> + }
>> + spin_lock_irqsave(&irq_desc[irq].lock, flags);
>> + act->flags &=~SA_MUST_THREAD;
>
> Both flags are zero here (see below about the WARN_ON)
>
The WARN_ON is a warning on both flags set (or supposed to be...:-)
It doesn't make sense to have SA_MUST_THREAD and SA_NODELAY both set.

>> + act->flags |= SA_NODELAY;
>> + spin_unlock_irqrestore(&irq_desc[irq].lock, flags);
>> + return 0;
>> + }
>> + else {
>> + printk(KERN_ERR "Irq handler "
>> + "for dev %s on IRQ%d can not be changed"
>> + " to hardirq context\n", act->name, irq);
>> + return -ENOSYS;
>> + }
>> +
>> +}
>> +
>> +
>> +static int __make_irq_threaded(int irq, struct irq_desc *desc);
>> +
>> +int make_irq_nodelay(int irq, struct irq_desc *desc)
>> +{
>> + int ret = 0;
>> + struct irqaction *act;
>> + unsigned long flags;
>> +
>> + rcu_read_lock();
>> + for(act = desc->action; act; act = act->next) {
>> + WARN_ON(((~act->flags) & (SA_MUST_THREAD|SA_NODELAY)) == 0);
>
> This WARN_ON is not protected by the descriptor lock, so if this
> function is run on two CPUs at the same time, then the act->flags can
> have both of these zero by the above code.

Both be set :-) But bug noticed.

>
>> + if(act->flags & SA_MUST_THREAD) {
>> + ret = make_action_nodelay(irq, act);
>> + if(ret) {
>> + printk(KERN_ERR "Could not make irq %d "
>> + "nodelay (errno %d)\n",
>> + irq, ret);
>
> We printk on failure from above, and then printk again here?

Well might be too verbose.

>
>> + goto failed;
>> + }
>> + }
>> + }
>> + spin_lock_irqsave(&desc->lock, flags);
>> + desc->status |= IRQ_NODELAY;
>> + spin_unlock_irqrestore(&desc->lock, flags);
>> + rcu_read_unlock();
>> +
>> + return 0;
>> + failed:
>> + __make_irq_threaded(irq, desc);
>> + rcu_read_unlock();
>> + return ret;
>> +}
>> +
>> +int action_may_thread(int irq, struct irqaction *act)
>> +{
>> + if(!act->change_context)
>> + return !(act->flags & SA_NODELAY);
>> +
>> + return act->change_context(irq, act->dev_id, IRQ_CAN_THREAD) == 0;
>
> This IRQ_CAN_THREAD just looks out of place of the other two. It feels
> very "hacky" to check if it can thread. But what do I know?

The problem is that the step to make a handler be threaded has to be split
in two: First ask, then remove IRQ_NODELAY, then change the locks to
mutexes. So the driver need to be involved twice. I could skip this, but I
put it in for generallity.

>
>> +}
>> +
>> +
>> +static int __make_irq_threaded(int irq, struct irq_desc *desc)
>> +{
>> + struct irqaction *act;
>> +
>> + for(act = desc->action; act; act = act->next) {
>> + WARN_ON(((~act->flags) & (SA_MUST_THREAD|SA_NODELAY)) == 0);
>> + if(!action_may_thread(irq, act)) {
>> + return -EINVAL;
>> + }
>> + }
>> +
>> + if (start_irq_thread(irq, desc))
>> + return -ENOMEM;
>
> I know that currently start_irq_thread can only return -ENOMEM on
> failure, but it may be more robust to capture the return code and return
> that anyway.

*nod*
This was copied from the original code, so the same thing is elsewher,
too.

>
>> +
>> + spin_lock_irq(&desc->lock);
>> + desc->status &= ~IRQ_NODELAY;
>> + spin_unlock_irq(&desc->lock);
>> +
>> + /* We can't make irq handlers change their context before we
>> + are sure no CPU is running them in hard irq context */
>> + synchronize_irq(irq);
>> +
>> + for(act = desc->action; act; act = act->next) {
>> + WARN_ON(((~act->flags) & (SA_MUST_THREAD|SA_NODELAY)) == 0);
>> + if(act->change_context) {
>> + /* This callback can't fail */
>
> Will all device drivers know that the callback can't fail?

Well, it is in the documentation.

>
>> + act->change_context(irq, act->dev_id, IRQ_TO_THREADED);
>> + spin_lock_irq(&desc->lock);
>> + act->flags &=~SA_NODELAY;
>> + act->flags |= SA_MUST_THREAD;
>> + spin_unlock_irq(&desc->lock);
>> + }
>> + }
>> +
>> + return 0;
>> +}
>
> [snipped the rest]
>
> -- Steve
>
>
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2006-06-04 18:35    [W:0.058 / U:1.920 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site