new poll callback'd wake up hell ...
Quoted 1 line · Davide Libenzi
1) Move the
wake_up()call done inside the poll callback outside the lock
You can't. You need to hold the lock over the callback or your callback could end up accessing a freed epitem.
Quoted 5 lines · John Myers
Quoted 1 line · Davide Libenzi
1) Move the
wake_up()call done inside the poll callback outside the lockYou can't. You need to hold the lock over the callback or your callback could end up accessing a freed epitem.
No, look at the code :
http://www.xmailserver.org/linux-patches/sys_epoll-2.5.49-0.58.diff
The function ep_collect_ready_items() increases the usage count under lock. So the epintem is protected, and the file* cannot desappear because of the read lock on epsem.
- Davide
Quoted 8 lines · Davide Libenzi
Quoted 5 lines · John Myers
Quoted 1 line · Davide Libenzi
1) Move the
wake_up()call done inside the poll callback outside the lockYou can't. You need to hold the lock over the callback or your callback could end up accessing a freed epitem.
Quoted 7 lines
No, look at the code :
http://www.xmailserver.org/linux-patches/sys_epoll-2.5.49-0.58.diff
The function
ep_collect_ready_items()increases the usage count under lock. So the epintem is protected, and the file* cannot desappear because of the read lock on epsem.
Ops, I understood the f_op->poll() not the wake_up(). It can be solved in the same way. I'll do it now.
- Davide
Quoted 12 lines · Davide Libenzi
Quoted 10 lines · Davide Libenzi
Quoted 5 lines · John Myers
Quoted 1 line · Davide Libenzi
1) Move the
wake_up()call done inside the poll callback outside the lockYou can't. You need to hold the lock over the callback or your callback could end up accessing a freed epitem.
No, look at the code :
Quoted 8 lines
Quoted 5 lines
http://www.xmailserver.org/linux-patches/sys_epoll-2.5.49-0.58.diff
The function
ep_collect_ready_items()increases the usage count under lock. So the epintem is protected, and the file* cannot desappear because of the read lock on epsem.Ops, I understood the
f_op->poll()not thewake_up(). It can be solved in the same way. I'll do it now.
The only place where a protection is needed is inside ep_insert() and not because of the wake_up() outsize the lock. Because of this :
/* Add the current item to the list of active epoll hook for this file */
spin_lock(&tfile->f_ep_lock);
list_add_tail(&epi->fllink, &tfile->f_ep_links);
spin_unlock(&tfile->f_ep_lock);- Davide
Quoted 3 lines · Davide Libenzi
No, look at the code :
Ok, I completely misread what you wrote. I thought you were suggestiong a change to the locking behavior of wake_up() itself.
One wonders if a work-to-do would be appropriate.
Quoted 7 lines · John Myers
Quoted 3 lines · Davide Libenzi
No, look at the code :
Ok, I completely misread what you wrote. I thought you were suggestiong a change to the locking behavior of
wake_up()itself.
The problem with the wake_up() is its potential recursion when using the callback'd version of the wait queue. We can have two scenarios ( given '->' == CONTAINED-IN ) :
1)
epfd1 -> epfd2 -> ... ->epfdN with N huge
2)
epfd1 -> epfd2 -> ... -> epfd1
Now, when you call wake_up() without using any care you'll blow up the stack. This should be adressed with the 0.58 version of the patch. Also, I moved the :
/* Add the current item to the list of active epoll hook for this file */
spin_lock(&tfile->f_ep_lock);
list_add_tail(&epi->fllink, &tfile->f_ep_links);
spin_unlock(&tfile->f_ep_lock);before inserting the item in the hash, so I don't need to increase the usage count in ep_insert().
- Davide