uio drivers with IRQF_NO_THREAD on preempt-rt kernel

uio drivers with IRQF_NO_THREAD on preempt-rt kernel

[Matthias Fuchs]

Hi folks,

I am running stable kernel v4.4.110 with preempt-rt patch rt125 on a AM335x non-SMP system.
There is one thread with hard realtime requirements running on this system. This thread is scheduled
by a hardware interrupt (either AM335x PRUSS or external FPGA).

Latencies from interrupt into process are as expected. Interrupt thread prio has been
bumped to 90. But I want/need even shorter latencies.

So I tried to use IRQF_NO_THREAD in my uio driver to get rid of the scheduling detour through over the interrupt thread. The interrupt handling should be quiet fast - most handling is done in userspace.

Here comes the problem. The uio framework uses wake_up_interruptible() in the isr which does
not work from hard interrupt handlers. I tried to modify uio.c to use wake_up_process() with a limitation
to support a single process having opened the device.

This works fine in most cases :-( Userspace uses select() on the device with timeouts. Sometimes my process is woken
by interrupt event and sometimes by timeout - fine. But it looks like I am missing some interrupts.
I expect some issue with uio poll/waitqueue handling. 

Any idea how to fix this? It would be fine to stay with poll in the uio drivers. I already thought about a special hrtimer handling in my situation. But I hope about something more pretty.

BTW, latencies are shorter with this approach!

Matthias


diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index bcc1fc0..7eb8257 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -394,8 +394,12 @@ void uio_event_notify(struct uio_info *info)
 	struct uio_device *idev = info->uio_dev;
 
 	atomic_inc(&idev->event);
-	wake_up_interruptible(&idev->wait);
-	kill_fasync(&idev->async_queue, SIGIO, POLL_IN);
+	if (idev->rt_consumer) {
+		wake_up_process(idev->rt_consumer);
+	} else {
+		wake_up_interruptible(&idev->wait);
+		kill_fasync(&idev->async_queue, SIGIO, POLL_IN);
+	}
 }
 EXPORT_SYMBOL_GPL(uio_event_notify);
 
@@ -439,6 +443,17 @@ static int uio_open(struct inode *inode, struct file *filep)
 		goto out;
 	}
 
+	if (idev->rt_consumer) {
+		ret = -EBUSY;
+		goto err_alloc_listener;
+	}
+
+#ifdef CONFIG_PREEMPT_RT_FULL
+	if (idev->info->irq_flags & IRQF_NO_THREAD) {
+	  idev->rt_consumer = current;
+	}
+#endif
+
 	listener = kmalloc(sizeof(*listener), GFP_KERNEL);
 	if (!listener) {
 		ret = -ENOMEM;
@@ -480,6 +495,7 @@ static int uio_release(struct inode *inode, struct file *filep)
 	struct uio_listener *listener = filep->private_data;
 	struct uio_device *idev = listener->dev;
 
+	idev->rt_consumer = NULL;
 	if (idev->info->release)
 		ret = idev->info->release(idev->info, inode);
 
diff --git a/include/linux/uio_driver.h b/include/linux/uio_driver.h
index 32c0e83..cf314da 100644
--- a/include/linux/uio_driver.h
+++ b/include/linux/uio_driver.h
@@ -73,6 +73,7 @@ struct uio_device {
         struct uio_info         *info;
         struct kobject          *map_dir;
         struct kobject          *portio_dir;
+        struct task_struct      *rt_consumer;
 };
 

Re: uio drivers with IRQF_NO_THREAD on preempt-rt kernel

[Julia Cartwright]

On Tue, May 08, 2018 at 05:59:27PM +0200, Matthias Fuchs wrote:
> Hi folks,

Hello Matthias-

> I am running stable kernel v4.4.110 with preempt-rt patch rt125 on a AM335x non-SMP system.
> There is one thread with hard realtime requirements running on this system. This thread is scheduled
> by a hardware interrupt (either AM335x PRUSS or external FPGA).
> 
> Latencies from interrupt into process are as expected. Interrupt thread prio has been
> bumped to 90. But I want/need even shorter latencies.
> 
> So I tried to use IRQF_NO_THREAD in my uio driver to get rid of the scheduling detour through over the interrupt thread. The interrupt handling should be quiet fast - most handling is done in userspace.
> 
> Here comes the problem. The uio framework uses wake_up_interruptible() in the isr which does
> not work from hard interrupt handlers. I tried to modify uio.c to use wake_up_process() with a limitation
> to support a single process having opened the device.

I didn't look at your code in detail, but you might consider looking at
the simple waitqueue implementation.  See include/linux/swait.h in the
kernel tree.  In -rt, completions have been reworked to use them, if you
want to look at an example.  swake_up_*() can be used in hardirq context.

Good luck,

   Julia

Re: uio drivers with IRQF_NO_THREAD on preempt-rt kernel

[Sebastian Andrzej Siewior]

On 2018-05-09 12:56:38 [-0500], Julia Cartwright wrote:
> On Tue, May 08, 2018 at 05:59:27PM +0200, Matthias Fuchs wrote:
> > Hi folks,
> 
> Hello Matthias-
> 
> > I am running stable kernel v4.4.110 with preempt-rt patch rt125 on a AM335x non-SMP system.
> > There is one thread with hard realtime requirements running on this system. This thread is scheduled
> > by a hardware interrupt (either AM335x PRUSS or external FPGA).
> > 
> > Latencies from interrupt into process are as expected. Interrupt thread prio has been
> > bumped to 90. But I want/need even shorter latencies.
> > 
> > So I tried to use IRQF_NO_THREAD in my uio driver to get rid of the scheduling detour through over the interrupt thread. The interrupt handling should be quiet fast - most handling is done in userspace.
> > 
> > Here comes the problem. The uio framework uses wake_up_interruptible() in the isr which does
> > not work from hard interrupt handlers. I tried to modify uio.c to use wake_up_process() with a limitation
> > to support a single process having opened the device.
> 
> I didn't look at your code in detail, but you might consider looking at
> the simple waitqueue implementation.  See include/linux/swait.h in the
> kernel tree.  In -rt, completions have been reworked to use them, if you
> want to look at an example.  swake_up_*() can be used in hardirq context.

This can be done but the "normal" waitqueue has to remain. If a process
blocks on read() then you can wake it up via swait() from hardirq
context. You need to keep the waitqueue for a possible poll() user.

> Good luck,
> 
>    Julia

Sebastian

Re: uio drivers with IRQF_NO_THREAD on preempt-rt kernel

[Matthias Fuchs]

Hi,

I updated my modified uio.c code using simple wake queues. See below.
Blocking read on the uio device is fine. But select() with timeout
behaves a little strange. I am still digging to find out what happens,
but it seems that even I should never run into a timeout in my test application,
the event_count of two consecutive select()/read() pairs is not advanced by one.

So is my implementation correct? Does using the normal waitqueue in this
manner satisfy uio_poll(). So in my case irq_flags has IRQF_NO_THREAD always set. This means 
idev->wait never gets a wake_up_interruptible().

diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index bcc1fc0..779dcaf 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -25,6 +25,7 @@
 #include <linux/kobject.h>
 #include <linux/cdev.h>
 #include <linux/uio_driver.h>
+#include <linux/swait.h>
 
 #define UIO_MAX_DEVICES		(1U << MINORBITS)
 
@@ -394,8 +395,12 @@ void uio_event_notify(struct uio_info *info)
 	struct uio_device *idev = info->uio_dev;
 
 	atomic_inc(&idev->event);
-	wake_up_interruptible(&idev->wait);
-	kill_fasync(&idev->async_queue, SIGIO, POLL_IN);
+	if (idev->info->irq_flags & IRQF_NO_THREAD) {
+		swake_up_locked(&idev->swait);
+	} else {
+		wake_up_interruptible(&idev->wait);
+		kill_fasync(&idev->async_queue, SIGIO, POLL_IN);
+	}
 }
 EXPORT_SYMBOL_GPL(uio_event_notify);
 
@@ -508,6 +513,7 @@ static ssize_t uio_read(struct file *filep, char __user *buf,
 	struct uio_listener *listener = filep->private_data;
 	struct uio_device *idev = listener->dev;
 	DECLARE_WAITQUEUE(wait, current);
+	DECLARE_SWAITQUEUE(swait);
 	ssize_t retval;
 	s32 event_count;
 
@@ -520,11 +526,10 @@ static ssize_t uio_read(struct file *filep, char __user *buf,
 	add_wait_queue(&idev->wait, &wait);
 
 	do {
-		set_current_state(TASK_INTERRUPTIBLE);
+		prepare_to_swait(&idev->swait, &swait, TASK_INTERRUPTIBLE);
 
 		event_count = atomic_read(&idev->event);
 		if (event_count != listener->event_count) {
-			__set_current_state(TASK_RUNNING);
 			if (copy_to_user(buf, &event_count, count))
 				retval = -EFAULT;
 			else {
@@ -546,7 +551,7 @@ static ssize_t uio_read(struct file *filep, char __user *buf,
 		schedule();
 	} while (1);
 
-	__set_current_state(TASK_RUNNING);
+	finish_swait(&idev->swait, &swait);
 	remove_wait_queue(&idev->wait, &wait);
 
 	return retval;
@@ -814,6 +819,7 @@ int __uio_register_device(struct module *owner,
 	idev->owner = owner;
 	idev->info = info;
 	init_waitqueue_head(&idev->wait);
+	init_swait_queue_head(&idev->swait);
 	atomic_set(&idev->event, 0);
 
 	ret = uio_get_minor(idev);


Cheers,
Matthias


On 15.05.2018 16:02, Sebastian Andrzej Siewior wrote:
> On 2018-05-09 12:56:38 [-0500], Julia Cartwright wrote:
>> On Tue, May 08, 2018 at 05:59:27PM +0200, Matthias Fuchs wrote:
>>> Hi folks,
>>
>> Hello Matthias-
>>
>>> I am running stable kernel v4.4.110 with preempt-rt patch rt125 on a AM335x non-SMP system.
>>> There is one thread with hard realtime requirements running on this system. This thread is scheduled
>>> by a hardware interrupt (either AM335x PRUSS or external FPGA).
>>>
>>> Latencies from interrupt into process are as expected. Interrupt thread prio has been
>>> bumped to 90. But I want/need even shorter latencies.
>>>
>>> So I tried to use IRQF_NO_THREAD in my uio driver to get rid of the scheduling detour through over the interrupt thread. The interrupt handling should be quiet fast - most handling is done in userspace.
>>>
>>> Here comes the problem. The uio framework uses wake_up_interruptible() in the isr which does
>>> not work from hard interrupt handlers. I tried to modify uio.c to use wake_up_process() with a limitation
>>> to support a single process having opened the device.
>>
>> I didn't look at your code in detail, but you might consider looking at
>> the simple waitqueue implementation.  See include/linux/swait.h in the
>> kernel tree.  In -rt, completions have been reworked to use them, if you
>> want to look at an example.  swake_up_*() can be used in hardirq context.
> 
> This can be done but the "normal" waitqueue has to remain. If a process
> blocks on read() then you can wake it up via swait() from hardirq
> context. You need to keep the waitqueue for a possible poll() user.
> 
>> Good luck,
>>
>>    Julia
> 
> Sebastian
> 

Re: uio drivers with IRQF_NO_THREAD on preempt-rt kernel

[Sebastian Andrzej Siewior]

On 2018-05-28 22:26:55 [+0200], Matthias Fuchs wrote:
> Hi,
> 
> I updated my modified uio.c code using simple wake queues. See below.
> Blocking read on the uio device is fine. But select() with timeout
> behaves a little strange. I am still digging to find out what happens,
> but it seems that even I should never run into a timeout in my test application,
> the event_count of two consecutive select()/read() pairs is not advanced by one.
> 
> So is my implementation correct? Does using the normal waitqueue in this
> manner satisfy uio_poll(). So in my case irq_flags has IRQF_NO_THREAD always set. This means 
> idev->wait never gets a wake_up_interruptible().
>
> diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
> index bcc1fc0..779dcaf 100644
> --- a/drivers/uio/uio.c
> +++ b/drivers/uio/uio.c
> @@ -25,6 +25,7 @@
>  #include <linux/kobject.h>
>  #include <linux/cdev.h>
>  #include <linux/uio_driver.h>
> +#include <linux/swait.h>
>  
>  #define UIO_MAX_DEVICES		(1U << MINORBITS)
>  
> @@ -394,8 +395,12 @@ void uio_event_notify(struct uio_info *info)
>  	struct uio_device *idev = info->uio_dev;
>  
>  	atomic_inc(&idev->event);
> -	wake_up_interruptible(&idev->wait);
> -	kill_fasync(&idev->async_queue, SIGIO, POLL_IN);
> +	if (idev->info->irq_flags & IRQF_NO_THREAD) {
> +		swake_up_locked(&idev->swait);

you want swake_up().

> +	} else {
> +		wake_up_interruptible(&idev->wait);
> +		kill_fasync(&idev->async_queue, SIGIO, POLL_IN);

also you need do this if you have someone is in poll(). You could the
upper part in the primary handler this in the threaded handler.

> +	}
>  }
>  EXPORT_SYMBOL_GPL(uio_event_notify);
>  
> @@ -508,6 +513,7 @@ static ssize_t uio_read(struct file *filep, char __user *buf,
>  	struct uio_listener *listener = filep->private_data;
>  	struct uio_device *idev = listener->dev;
>  	DECLARE_WAITQUEUE(wait, current);
> +	DECLARE_SWAITQUEUE(swait);
>  	ssize_t retval;
>  	s32 event_count;
>  
> @@ -520,11 +526,10 @@ static ssize_t uio_read(struct file *filep, char __user *buf,
>  	add_wait_queue(&idev->wait, &wait);
>  
>  	do {
> -		set_current_state(TASK_INTERRUPTIBLE);
> +		prepare_to_swait(&idev->swait, &swait, TASK_INTERRUPTIBLE);
>  
>  		event_count = atomic_read(&idev->event);
>  		if (event_count != listener->event_count) {
> -			__set_current_state(TASK_RUNNING);
>  			if (copy_to_user(buf, &event_count, count))
>  				retval = -EFAULT;
>  			else {
> @@ -546,7 +551,7 @@ static ssize_t uio_read(struct file *filep, char __user *buf,
>  		schedule();
>  	} while (1);
>  
> -	__set_current_state(TASK_RUNNING);
> +	finish_swait(&idev->swait, &swait);
>  	remove_wait_queue(&idev->wait, &wait);

and ->wait isn't used in ->read() anymore, right? Just in ->poll(). If
so it could go.

>  	return retval;
> @@ -814,6 +819,7 @@ int __uio_register_device(struct module *owner,
>  	idev->owner = owner;
>  	idev->info = info;
>  	init_waitqueue_head(&idev->wait);
> +	init_swait_queue_head(&idev->swait);
>  	atomic_set(&idev->event, 0);
>  
>  	ret = uio_get_minor(idev);
> 
> 
> Cheers,
> Matthias

Sebastian

Re: uio drivers with IRQF_NO_THREAD on preempt-rt kernel

[Matthias Fuchs]

Hi Sebastian,

On 29.05.2018 18:51, Sebastian Andrzej Siewior wrote:
> On 2018-05-28 22:26:55 [+0200], Matthias Fuchs wrote:
>> Hi,
>>
>> I updated my modified uio.c code using simple wake queues. See below.
>> Blocking read on the uio device is fine. But select() with timeout
>> behaves a little strange. I am still digging to find out what happens,
>> but it seems that even I should never run into a timeout in my test application,
>> the event_count of two consecutive select()/read() pairs is not advanced by one.
>>
>> So is my implementation correct? Does using the normal waitqueue in this
>> manner satisfy uio_poll(). So in my case irq_flags has IRQF_NO_THREAD always set. This means
>> idev->wait never gets a wake_up_interruptible().
>>
>> diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
>> index bcc1fc0..779dcaf 100644
>> --- a/drivers/uio/uio.c
>> +++ b/drivers/uio/uio.c
>> @@ -25,6 +25,7 @@
>>   #include <linux/kobject.h>
>>   #include <linux/cdev.h>
>>   #include <linux/uio_driver.h>
>> +#include <linux/swait.h>
>>   
>>   #define UIO_MAX_DEVICES		(1U << MINORBITS)
>>   
>> @@ -394,8 +395,12 @@ void uio_event_notify(struct uio_info *info)
>>   	struct uio_device *idev = info->uio_dev;
>>   
>>   	atomic_inc(&idev->event);
>> -	wake_up_interruptible(&idev->wait);
>> -	kill_fasync(&idev->async_queue, SIGIO, POLL_IN);
>> +	if (idev->info->irq_flags & IRQF_NO_THREAD) {
>> +		swake_up_locked(&idev->swait);
> 
> you want swake_up().
> 
>> +	} else {
>> +		wake_up_interruptible(&idev->wait);
>> +		kill_fasync(&idev->async_queue, SIGIO, POLL_IN);
> 
> also you need do this if you have someone is in poll(). You could the
> upper part in the primary handler this in the threaded handler.

uio.c only has a single handler that calls uio_event_notify(). That 
handler is typically threaded and in my case not (uio driver passes 
IRQF_NO_THREAD). So what threaded handler do you mean? Do you mean from 
uio_read()? Does this work?

My itention is to use select() (aka poll) on the uio driver from my RT 
application and have no irq thread(). I do not want to implement a 
separate timeout mechanism on read().

I've seen this requirement in other places before.

Matthias

> 
>> +	}
>>   }
>>   EXPORT_SYMBOL_GPL(uio_event_notify);
>>   
>> @@ -508,6 +513,7 @@ static ssize_t uio_read(struct file *filep, char __user *buf,
>>   	struct uio_listener *listener = filep->private_data;
>>   	struct uio_device *idev = listener->dev;
>>   	DECLARE_WAITQUEUE(wait, current);
>> +	DECLARE_SWAITQUEUE(swait);
>>   	ssize_t retval;
>>   	s32 event_count;
>>   
>> @@ -520,11 +526,10 @@ static ssize_t uio_read(struct file *filep, char __user *buf,
>>   	add_wait_queue(&idev->wait, &wait);
>>   
>>   	do {
>> -		set_current_state(TASK_INTERRUPTIBLE);
>> +		prepare_to_swait(&idev->swait, &swait, TASK_INTERRUPTIBLE);
>>   
>>   		event_count = atomic_read(&idev->event);
>>   		if (event_count != listener->event_count) {
>> -			__set_current_state(TASK_RUNNING);
>>   			if (copy_to_user(buf, &event_count, count))
>>   				retval = -EFAULT;
>>   			else {
>> @@ -546,7 +551,7 @@ static ssize_t uio_read(struct file *filep, char __user *buf,
>>   		schedule();
>>   	} while (1);
>>   
>> -	__set_current_state(TASK_RUNNING);
>> +	finish_swait(&idev->swait, &swait);
>>   	remove_wait_queue(&idev->wait, &wait);
> 
> and ->wait isn't used in ->read() anymore, right? Just in ->poll(). If
> so it could go.
> 
>>   	return retval;
>> @@ -814,6 +819,7 @@ int __uio_register_device(struct module *owner,
>>   	idev->owner = owner;
>>   	idev->info = info;
>>   	init_waitqueue_head(&idev->wait);
>> +	init_swait_queue_head(&idev->swait);
>>   	atomic_set(&idev->event, 0);
>>   
>>   	ret = uio_get_minor(idev);
>>
>>
>> Cheers,
>> Matthias
> 
> Sebastian
> 
Matthias