wait_event_interruptible() seems non-atomic

wait_event_interruptible() seems non-atomic

[Jan Engelhardt]

Hi list,


upon reviewing some of my code for a device driver, I've come across:


static ssize_t uif_read(struct file *filp, char __user *buf,
  size_t count, loff_t *ppos)
{
    // Nothing read, nothing done
    if(count == 0) { return 0; }

    // Must sleep as long as there is no data
    if(down_interruptible(&Buffer_lock)) { return -ERESTARTSYS; }
    while(BufRP == BufWP) {
        up(&Buffer_lock);
        if(filp->f_flags & O_NONBLOCK) { return -EAGAIN; }

        // hm, the condition is not atomic or locked...
        if(wait_event_interruptible(Pull_queue, (BufRP != BufWP))) {
            return -ERESTARTSYS;
        }
        if(down_interruptible(&Buffer_lock)) { return -ERESTARTSYS; }
    }

    // Data is available, so give it to the user
    ...
}

As you can see, I lock Buffer_lock and then check BufRP == BufWP. I do that
because in an SMP environment, either of BufRP or BufWP might have been
updated. (I.e. one CPU currently does "mov BufRP to eax; mov BufWP to ebx; cmp
eax, ebx" while another does "inc BufWP")

I would like to also lock Buffer_lock around BufRP != BufWP, but don't see a
way on how to accomplish this.

Does anybody know a way how this could be achieved?



Jan Engelhardt
-- 
Gesellschaft für Wissenschaftliche Datenverarbeitung
Am Fassberg, 37077 Göttingen, www.gwdg.de

Re: wait_event_interruptible() seems non-atomic

[Manfred Spraul]

Hi Jan,

>I would like to also lock Buffer_lock around BufRP != BufWP, but don't see a
>way on how to accomplish this.
>
>  
>
This is not a problem: You compare BufRP and BufWP twice: once within 
wait_event_interruptible (without locking) and again a second test in 
your uif_read function with locking.
You are right that the test within wait_event_interruptible is 
optimistic: a concurrent uif_read could read the new data before the 
initial uif_read has a chance to acquire the BufferLock. But it doesn't 
matter: AFAICS the test is optimistic, it can't happen that BufRP and WP 
are actually different and wait_event sleeps. And the external loop 
within uif_read() just loops if the race that you describe happened.

Btw, could you post a link to the complete driver when asking questions? 
For example the use of down_interruptible() looks wrong to me, I'd use 
plain down().

--
    Manfred

Re: wait_event_interruptible() seems non-atomic

[Jan Engelhardt]

>Hi Jan,
>
>>I would like to also lock Buffer_lock around BufRP != BufWP, but don't see a
>>way on how to accomplish this.

>This is not a problem: You compare BufRP and BufWP twice: once within
>wait_event_interruptible (without locking) and again a second test in
>your uif_read function with locking.
>You are right that the test within wait_event_interruptible is
>optimistic: a concurrent uif_read could read the new data before the
>initial uif_read has a chance to acquire the BufferLock. But it doesn't
>matter: AFAICS the test is optimistic, it can't happen that BufRP and WP
>are actually different and wait_event sleeps. And the external loop
>within uif_read() just loops if the race that you describe happened.

Yay, I see now. It takes a fair amount of looking behind the scene to get the
idea. (It's just copy-and-paste from an O'reilly book with a few
modifications.)
Yes, there is always only one way from any insn you look at, which does not
require a lock.

>Btw, could you post a link to the complete driver when asking questions?

http://ttyrpld.sf.net/ -- "kernel-2.6/rpldev.c" in the tarball.

>For example the use of down_interruptible() looks wrong to me, I'd use
>plain down().

I'd like to be able to hit Ctrl+C (in the userspace application) whenever
possible. If that's not a reason, blame the book
http://www.xml.com/ldd/chapter/book/ch03.html#t8 ("the read method" a further
down below)

BTW, the complete book is at http://www.xml.com/ldd/chapter/book/index.html


Thanks,
Jan Engelhardt
-- 
Gesellschaft für Wissenschaftliche Datenverarbeitung
Am Fassberg, 37077 Göttingen, www.gwdg.de

Re: wait_event_interruptible() seems non-atomic

[Manfred Spraul]

Jan Engelhardt wrote:

>>For example the use of down_interruptible() looks wrong to me, I'd use
>>plain down().
>>    
>>
>
>I'd like to be able to hit Ctrl+C (in the userspace application) whenever
>possible. If that's not a reason, blame the book
>http://www.xml.com/ldd/chapter/book/ch03.html#t8 ("the read method" a further
>down below)
>
>  
>
As far as I can see BufferLock is only held for tiny sections - the 
longest thing is a copy_to_user(), i.e. at worst a swap in. I my opinion 
the delay for handling Ctrl+C is therefore negligible and not worth the 
added code for handling down_interruptible().
You have already written the code, so I'd leave it as it is and I'll 
blame the book. They probably started from an older version of 
fs/pipe.c, which contained _interruptible calls. There are gone now, 
this allowed some cleanup.

--
    Manfred

Re: wait_event_interruptible() seems non-atomic

[Jan Engelhardt]

>>>For example the use of down_interruptible() looks wrong to me, I'd use
>>>plain down().
>>
>>I'd like to be able to hit Ctrl+C (in the userspace application) whenever
>>possible. If that's not a reason, blame the book
>>http://www.xml.com/ldd/chapter/book/ch03.html#t8 ("the read method" a further
>>down below)
>
>You have already written the code, so I'd leave it as it is and I'll
>blame the book. They probably started from an older version of
>fs/pipe.c, which contained _interruptible calls. There are gone now,
>this allowed some cleanup.

Well, it's just one line so I would not care, and I'm also open for
suggestions. Does down_interruptible() cost so much more in CPU cycles than
down()?


Jan Engelhardt
-- 
Gesellschaft für Wissenschaftliche Datenverarbeitung
Am Fassberg, 37077 Göttingen, www.gwdg.de

Re: wait_event_interruptible() seems non-atomic

[Manfred Spraul]

Jan Engelhardt wrote:

>>You have already written the code, so I'd leave it as it is and I'll
>>blame the book. They probably started from an older version of
>>fs/pipe.c, which contained _interruptible calls. There are gone now,
>>this allowed some cleanup.
>>    
>>
>
>Well, it's just one line so I would not care, and I'm also open for
>suggestions. Does down_interruptible() cost so much more in CPU cycles than
>down()?
>
>  
>
It's more about code complexity than performance. down_interruptible() 
means that you must handle failures - double check that you free all 
temporary allocations, decrease all reference counts (make the reference 
counts atomic_t), etc.

--
    Manfred

Re: wait_event_interruptible() seems non-atomic

[Jan Engelhardt]

>>Well, it's just one line so I would not care, and I'm also open for
>>suggestions. Does down_interruptible() cost so much more in CPU cycles than
>>down()?
>
>It's more about code complexity than performance. down_interruptible()
>means that you must handle failures - double check that you free all
>temporary allocations, decrease all reference counts (make the reference
>counts atomic_t), etc.

All considered. rpldev.c only has 4 occurrences of down_interruptible, all
which are near the start of the function body. There's nothing to deallocate at
the time down_interruptible() is due ;-)



Jan Engelhardt
-- 
Gesellschaft für Wissenschaftliche Datenverarbeitung
Am Fassberg, 37077 Göttingen, www.gwdg.de