Arve Hjønnevåg <arve@xxxxxxxxxxx> wrote:
> On Thu, Mar 7, 2013 at 5:30 PM, Eric Wong <normalperson@xxxxxxxx> wrote:
> > 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.
> >
>
> The callback function in ep_scan_ready_list can call __pm_relax on it though.
>
> > 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.
>
> This code is reached when ep_scan_ready_list has set ep->ovflist to
> NULL before releasing ep->lock. Since the callback function can call
> __pm_relax on epi->ws without holding ep->lock we call __pm_stay_awake
> in ep->ws here (the callback does not call __pm_relax on that).
Thanks for the explanation. I got "deactivate" and "destroy"
mixed up. However, I'm still concerned about the "destroy" case:
> >
> >> 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.
>
> ep_scan_ready_list calls __pm_relax on ep->ws when it is done, so this
> will not work. ep->ws is not a "ep->rdllist not empty wakeup_source is
> is a "ep_scan_ready_list is running" wakeup_source.
What happens if ep_modify calls ep_destroy_wakeup_source
while __pm_stay_awake is running on the same epi->ws?
--
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
