On 10/24/2013 06:09 AM, Paul E. McKenney wrote:
> On Tue, Oct 01, 2013 at 05:08:14PM +0000, Jason Baron wrote:
>> When calling EPOLL_CTL_ADD for an epoll file descriptor that is attached
>> directly to a wakeup source, we do not need to take the global 'epmutex',
>> unless the epoll file descriptor is nested. The purpose of taking
>> the 'epmutex' on add is to prevent complex topologies such as loops and
>> deep wakeup paths from forming in parallel through multiple EPOLL_CTL_ADD
>> operations. However, for the simple case of an epoll file descriptor
>> attached directly to a wakeup source (with no nesting), we do not need
>> to hold the 'epmutex'.
>>
>> This patch along with 'epoll: optimize EPOLL_CTL_DEL using rcu' improves
>> scalability on larger systems. Quoting Nathan Zimmer's mail on SPECjbb
>> performance:
>>
>> "
>> On the 16 socket run the performance went from 35k jOPS to 125k jOPS.
>> In addition the benchmark when from scaling well on 10 sockets to scaling well
>> on just over 40 sockets.
>>
>> ...
>>
>> Currently the benchmark stops scaling at around 40-44 sockets but it seems like
>> I found a second unrelated bottleneck.
>> "
>
> Some questions and comments below.
>
>> Tested-by: Nathan Zimmer <nzimmer@xxxxxxx>
>> Signed-off-by: Jason Baron <jbaron@xxxxxxxxxx>
>> ---
>> fs/eventpoll.c | 94 ++++++++++++++++++++++++++++++++++++++++++----------------
>> 1 file changed, 68 insertions(+), 26 deletions(-)
>>
>> diff --git a/fs/eventpoll.c b/fs/eventpoll.c
>> index dd9fae1..0f25162 100644
>> --- a/fs/eventpoll.c
>> +++ b/fs/eventpoll.c
>> @@ -595,8 +595,7 @@ static inline void ep_pm_stay_awake_rcu(struct epitem *epi)
>> static int ep_scan_ready_list(struct eventpoll *ep,
>> int (*sproc)(struct eventpoll *,
>> struct list_head *, void *),
>> - void *priv,
>> - int depth)
>> + void *priv, int depth, int ep_locked)
>> {
>> int error, pwake = 0;
>> unsigned long flags;
>> @@ -607,7 +606,9 @@ static int ep_scan_ready_list(struct eventpoll *ep,
>> * We need to lock this because we could be hit by
>> * eventpoll_release_file() and epoll_ctl().
>> */
>> - mutex_lock_nested(&ep->mtx, depth);
>> +
>> + if (!ep_locked)
>> + mutex_lock_nested(&ep->mtx, depth);
>>
>> /*
>> * Steal the ready list, and re-init the original one to the
>> @@ -671,7 +672,8 @@ static int ep_scan_ready_list(struct eventpoll *ep,
>> }
>> spin_unlock_irqrestore(&ep->lock, flags);
>>
>> - mutex_unlock(&ep->mtx);
>> + if (!ep_locked)
>> + mutex_unlock(&ep->mtx);
>>
>> /* We have to call this outside the lock */
>> if (pwake)
>
> OK, allowing calls to ep_scan_ready_list() to be made under the lock.
>
> Usually you would use a __ep_scan_ready_list() instead of adding the
> flag, but you do have the wakeup that needs to be outside of the lock.
>
> But doesn't that mean that you need something like?
>
> if (pwake && !ep_locked)
>
> And then wouldn't you also need some way to inform the caller to
> do the wakeup after releasing ep->mtx?
>
> Or is it now safe to do the wakeup under ep->mtx? If so, why?
>
All I was trying to do here was to preserve the locking ep->mtx as it was
prior to this patch series. Because the patches now take the 'destination'
ep->mtx on an epoll_ctl() operation. It might be already held leading to
double locking (i hit this case while testing this series). So the idea
here is simply to realize when we've already grabbed this ep->mtx.
>> @@ -829,15 +831,34 @@ static int ep_read_events_proc(struct eventpoll *ep, struct list_head *head,
>> return 0;
>> }
>>
>> +static void ep_ptable_queue_proc(struct file *file, wait_queue_head_t *whead,
>> + poll_table *pt);
>> +
>> +struct readyevents_arg {
>> + struct eventpoll *ep;
>> + int locked;
>> +};
>> +
>> static int ep_poll_readyevents_proc(void *priv, void *cookie, int call_nests)
>> {
>
> OK, I will bite... What is constraining the arguments of the
> ep_poll_readyevents_proc()? I don't see any use of it except in this
> file. Nor do I see any use of ep_call_nested() except in this file.
> Might it be a bit cleaner to add a flag to the argument list?
>
> I don't feel strongly about this, just asking the question.
>
I don't feel too strongly either, but other users of ep_call_nested
don't need this either...
Thanks,
-Jason
--
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
