Eric Wong <normalperson@xxxxxxxx> wrote:
> Hi Arve, looking at commit 4d7e30d98939a0340022ccd49325a3d70f7e0238
> (epoll: Add a flag, EPOLLWAKEUP, to prevent suspend ...)
>
> I think the reason for using ep->ws instead of epi->ws in the unlikely
> ovflist case applies to the likely rdllist case, too. Since epi->ws is
> only protected by ep->mtx, it can also be deactivated while inside
> ep_poll_callback.
>
> So something like the following patch might be necessary
> (shown here with extra context):
>
> --- a/fs/eventpoll.c
> +++ b/fs/eventpoll.c
> @@ -968,39 +968,45 @@ static int ep_poll_callback(wait_queue_t *wait, unsigned mode, int sync, void *k
> if (unlikely(ep->ovflist != EP_UNACTIVE_PTR)) {
> if (epi->next == EP_UNACTIVE_PTR) {
> epi->next = ep->ovflist;
> ep->ovflist = epi;
> if (epi->ws) {
> /*
> * Activate ep->ws since epi->ws may get
> * deactivated at any time.
> */
> __pm_stay_awake(ep->ws);
> }
>
> }
Thinking about this more, it looks like the original ep->ovflist case of
using ep->ws is unnecessary.
ep->ovflist != EP_UNACTIVE_PTR can only happen while ep->mtx is held (in
ep_scan_ready_list); which means ep_modify+friends cannot remove epi->ws.
ep_poll_callback holding ep->lock means ep_poll_callback prevents
ep_scan_ready_list from setting ep->ovflist = EP_UNACTIVE_PTR and
releasing ep->mtx.
> goto out_unlock;
> }
>
> /* If this file is already in the ready list we exit soon */
> if (!ep_is_linked(&epi->rdllink)) {
> list_add_tail(&epi->rdllink, &ep->rdllist);
> - __pm_stay_awake(epi->ws);
> + if (epi->ws) {
> + /*
> + * Activate ep->ws since epi->ws may get
> + * deactivated at any time.
> + */
> + __pm_stay_awake(ep->ws);
> + }
> }
I still think ep->ws needs to be used in the common ep->rdllist case.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
