Re: [PATCH] epoll: simplify ep_poll_safewake() for CONFIG_DEBUG_LOCK_ALLOC

[Roman Penyaev]

On 2019-09-23 17:43, Jason Baron wrote:
> On 9/4/19 4:22 PM, Jason Baron wrote:
> > Currently, ep_poll_safewake() in the CONFIG_DEBUG_LOCK_ALLOC case uses
> > ep_call_nested() in order to pass the correct subclass argument to
> > spin_lock_irqsave_nested(). However, ep_call_nested() adds unnecessary
> > checks for epoll depth and loops that are already verified when doing
> > EPOLL_CTL_ADD. This mirrors a conversion that was done for
> > !CONFIG_DEBUG_LOCK_ALLOC in: commit 37b5e5212a44 ("epoll: remove
> > ep_call_nested() from ep_eventpoll_poll()")
> >
> > Signed-off-by: Jason Baron <jbaron@xxxxxxxxxx>
> > Cc: Davidlohr Bueso <dave@xxxxxxxxxxxx>
> > Cc: Roman Penyaev <rpenyaev@xxxxxxx>
> > Cc: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> > Cc: Eric Wong <normalperson@xxxxxxxx>
> > Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> > ---
> >  fs/eventpoll.c | 36 +++++++++++++-----------------------
> >  1 file changed, 13 insertions(+), 23 deletions(-)
> >
> > diff --git a/fs/eventpoll.c b/fs/eventpoll.c
> > index d7f1f50..a9b2737 100644
> > --- a/fs/eventpoll.c
> > +++ b/fs/eventpoll.c
> > @@ -551,28 +551,23 @@ static int ep_call_nested(struct nested_calls 
> > *ncalls,
> >   */
> >  #ifdef CONFIG_DEBUG_LOCK_ALLOC
> >
> > -static struct nested_calls poll_safewake_ncalls;
> > -
> > -static int ep_poll_wakeup_proc(void *priv, void *cookie, int 
> > call_nests)
> > -{
> > -	unsigned long flags;
> > -	wait_queue_head_t *wqueue = (wait_queue_head_t *)cookie;
> > -
> > -	spin_lock_irqsave_nested(&wqueue->lock, flags, call_nests + 1);
> > -	wake_up_locked_poll(wqueue, EPOLLIN);
> > -	spin_unlock_irqrestore(&wqueue->lock, flags);
> > -
> > -	return 0;
> > -}
> > +static DEFINE_PER_CPU(int, wakeup_nest);
> >
> >  static void ep_poll_safewake(wait_queue_head_t *wq)
> >  {
> > -	int this_cpu = get_cpu();
> > -
> > -	ep_call_nested(&poll_safewake_ncalls,
> > -		       ep_poll_wakeup_proc, NULL, wq, (void *) (long) this_cpu);
> > +	unsigned long flags;
> > +	int subclass;
> >
> > -	put_cpu();
> > +	local_irq_save(flags);
> > +	preempt_disable();
> > +	subclass = __this_cpu_read(wakeup_nest);
> > +	spin_lock_nested(&wq->lock, subclass + 1);
> > +	__this_cpu_inc(wakeup_nest);
> > +	wake_up_locked_poll(wq, POLLIN);
> > +	__this_cpu_dec(wakeup_nest);
> > +	spin_unlock(&wq->lock);
> > +	local_irq_restore(flags);
> > +	preempt_enable();
> >  }

What if reduce number of lines with something as the following:

   int this_cpu = get_cpu();
   subclass = __this_cpu_inc_return(wakeup_nest);
   spin_lock_irqsave_nested(&wq->lock, flags, subclass);
   wake_up_locked_poll(wq, POLLIN);
   spin_unlock_irqrestore(&wq->lock, flags);
   __this_cpu_dec(wakeup_nest);
   put_cpu();

Other than that looks good to me.

Reviewed-by: Roman Penyaev <rpenyaev@xxxxxxx>

--
Roman