Problems implementing poll call

Problems implementing poll call

[Constantine Gavrilov]

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


-- 
----------------------------------------
Constantine Gavrilov
Linux Leader
Optibase Ltd
7 Shenkar St, Herzliya 46120, Israel
Phone: (972-9)-970-9140
Fax:   (972-9)-958-6099
----------------------------------------




[-- Attachment #2: let.txt --]
[-- Type: text/plain, Size: 2625 bytes --]

Hi,

I have a problem implementing poll method.

I have written a driver for MPEG encoder card. The user-space SDK needs to be able to wait for a certain event that is reported by the interrupt handler. I have done it using ioctl method, like this:

	u32 timeout=milliseconds * HZ / 1000;

	set_bit(0, &dev->fintwait);
	if(test_bit(0, &dev->fintwait)) {
		interruptible_sleep_on_timeout(&dev->interrupt_queue,timeout);

		if (signal_pending(current)) {
			printk(KERN_ERR "optenc: IntWait restarted by signal\n");
			return -ERESTARTSYS;
		}

		if(test_bit(0, &dev->fintwait)) {
			printk(KERN_ERR "optenc: intwait timeout\n");
			..//returns wait timeout
		}
		else {
			...//returns wait OK
		}
	}
	//returns wait OK
	
The interrupt handler wakes up the queue and updates dev->fintwait like this:

	clear_bit(0, &dev->fintwait);
	wake_up_interruptible(&dev->interrupt_queue);
	

It worked very well for me. I wanted to implement the same wait using the poll method. So, my poll function looks like this:

unsigned int optenc_poll(struct file *filp, poll_table *wait_table)
{
	unsigned int mask = 0;
	struct mydev *dev = filp->private_data;
	
	set_bit(0, &dev->fintwait);
	if(test_bit(0, &dev->fintwait)) {
		poll_wait(filp, &dev->interrupt_queue, wait_table);
		if(test_bit(0, &dev->fintwait))
			return mask;
		else {
			mask |= POLLIN |POLLRDNORM;
			return mask;
		}
	}
	else {
		mask |= POLLIN |POLLRDNORM;
		return mask;
	}
}

Seems straightforward and the same thing as above. But, I have the following problems:

a) I have a lot of calls with wait_table = NULL and poll_wait does not block. I always do select on one file descriptor only and I never use zero timeout, so I do not understand the reason for it.

b) Even when wait_table is not NULL, poll_wait returns before (!!) interrupt handler wakes up the queue. I have checked it with printk. It always like this:

	set_bit
	poll_wait
	poll_wait returns and test_bit is true
	wake_up and clear_bit

It is like poll_wait does not seem to block and I have spurious calles with wait_table == NULL. Any ideas?


Just to verify, the user-space wait function looks like this :

BOOL Wait(int timeout)
{

	fd_set set;
	struct timeval tv;
	int retval;

	FD_ZERO(&set);
	FD_SET(fd, &set);
	tv.tv_sec = timeout/1000;
	tv.tv_usec = (timeout%1000)*1000;

	int rc=select(fd+1, &set, NULL, NULL, &tv);
	if(rc == -1) {
		PERROR("select");
		return FALSE;
	}
	if(rc == 1)
		return TRUE;
	else
		return FALSE;
}

I use 2.4.18-pre7ac1 and I have also checked stock RedHat's 2.4.9-34.

Re: Problems implementing poll call

[Dan Maas]

* Constantine Gavrilov (const-g@optibase.com) wrote:
> As far as races are concerned, it should be OK. This is because my
> condition uses testbit() and interrupt handler uses setbit(). These
> operations are atomic even on SMP configurations. Right?

Yes, test/setbit() are atomic, but that's not the point - the "do I
need to sleep?" check must be atomic with respect to
sleep_on(). Consider the following case:

if(testbit(...)) {
	// bit is TRUE now
	
	// now the interrupt handler runs:
	// setbit(FALSE)
	// wake_up_interruptible(...)
	
	interruptible_sleep_on(...);
}

Since wake_up() is called before sleep_on(), it will not wake you
up. If this case occurs, you will remain asleep forever.

You will probably find a few places in the kernel that suffer this
condition; the race window is very small so it's not likely to happen
in practice. But if I were developing a driver I'd definitely pay
attention to it...

A good description of the problem and solution is on page 287 of
Rubini's "Linux Device Drivers", 2nd ed. (I highly recommend this book
btw).

I just learned that there are macros in sched.h that encapsulate the
race-free solution (wait_event() and wait_event_interuptible()):
instead of the if() statement, you can write wait_event(..., !testbit());

> What do you think is of lower latency -- a poll() hook or an ioctl()
> hook?

Probably ioctl(). But ioctl() has the disadvantage that you can only
wait on one file descriptor. It's easy to provide both...

> With poll(), I do not know how to catch an error, because the poll
> function is called many times and I do not know which return is
> last. But it is not critical for me.

poll() should not return an error unless there is something wrong with
poll() itself. poll() merely tells the user that "some activity has
occurred on this file descriptor" - where "some activity" may include
an error. Device-specific errors should be returned when the user
subsequently calls read() write() or ioctl() on your device.

Regards,
Dan

Re: Problems implementing poll call

[Constantine Gavrilov]

Thanks a lot. I get it now.

As far as races are concerned,  it should be OK. This is because my 
condition uses testbit() and interrupt handler uses setbit(). These 
operations are atomic even on SMP configurations. Right?

What do you think is of lower latency -- a poll() hook or an ioctl() hook?

One thing I could have with ioctl implementation was that I knew in 
kernel space that a timeout has occurred. With poll(), I do not know how 
to catch an error, because the poll function is called many times and I 
do not know which return is last. But it is not critical for me.

Dan Maas wrote:

>Hi Constantine, this is in reponse to your post on the Linux kernel
>list of 17 October...
>
>I think you should take a look at the poll() implementation of other
>drivers. You should call poll_wait() before testing the bit flag,
>regardless of whether you actually need to block or not.
>
>Note that poll_wait() does not block; it merely adds your device's
>wait queue to a list of items the user is waiting on. The actual
>blocking occurs in other kernel code after your poll() method
>returns... select() and poll() can make several passes over the file
>descriptors; wait_table may NULL if the kernel knows for sure it is
>not going to block and is just checking the status of your device.
>
>I think you also need to be a little more careful about race
>conditions in your sleeping code. If you write this:
>
>if(need_to_sleep) {
>	interruptible_sleep_on(...);
>}
>
>You create a potential race condition where need_to_sleep is changed
>by an interrupt between the if() check and the sleep_on(). To
>eliminate these races you need to set the task state to
>TASK_INTERRUPTIBLE, then check the 'need_to_sleep' condition, then
>schedule() to actually go to sleep. This way if the interrupt handler
>changes need_to_sleep after your if() statement, you will wake up
>immediately instead of staying asleep forever. For an example of this
>technique see the comments around line 1680 in my driver
>drivers/ieee1394/dv1394.c
>
>Regards,
>Dan
>


-- 
----------------------------------------
Constantine Gavrilov
Linux Leader
Optibase Ltd
7 Shenkar St, Herzliya 46120, Israel
Phone: (972-9)-970-9140
Fax:   (972-9)-958-6099
----------------------------------------