lkml.org 
[lkml]   [2020]   [Jan]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] fs/epoll: rework safewake for CONFIG_DEBUG_LOCK_ALLOC
From
Date
Hi,

Thanks for looking at this.

On 1/6/20 2:38 PM, Davidlohr Bueso wrote:
> Some comments on:
>
> f6520c52084 (epoll: simplify ep_poll_safewake() for CONFIG_DEBUG_LOCK_ALLOC)
>
> For one this does not play nice with preempt_rt as disabling irq and
> then taking a spinlock_t is a no no; the critical region becomes
> preemptible. This is particularly important as -rt is being mainlined.
>

hmmm, but before the spinlock is taken there is a preempt_disable() call.

> Secondly, it is really ugly compared to what we had before - albeit not
> having to deal with all the ep_call_nested() checks, but I doubt this
> overhead matters at all with CONFIG_DEBUG_LOCK_ALLOC.
>

Yes, the main point of the patch is to continue to remove dependencies
on ep_call_nested(), and then eventually to remove it completely.

> While the current logic avoids nesting by disabling irq during the whole
> path, this seems like an overkill under debug. This patch proposes using
> this_cpu_inc_return() then taking the irq-safe lock - albeit a possible
> irq coming in the middle between these operations and messing up the
> subclass. If this is unacceptable, we could always revert the patch,
> as this was never a problem in the first place.

I personally don't want to introduce false positives. But I'm not quite
sore on that point - the subclass will still I think always increase on
nested calls it just may skip some numbers. I'm not sure offhand if that
messes up lockdep. perhaps not?


Thanks,

-Jason


>
> Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
> ---
>
> This is pretty much untested - I wanted to see what is the best way to
> address the concerns first.
>
> XXX: I don't think we need get/put_cpu() around the call, this was intended
> only for ep_call_nested() tasks_call_list checking afaict.
>
> fs/eventpoll.c | 19 ++++++++++---------
> 1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/fs/eventpoll.c b/fs/eventpoll.c
> index 67a395039268..97d036deff3e 100644
> --- a/fs/eventpoll.c
> +++ b/fs/eventpoll.c
> @@ -558,16 +558,17 @@ static void ep_poll_safewake(wait_queue_head_t *wq)
> unsigned long flags;
> int subclass;
>
> - local_irq_save(flags);
> - preempt_disable();
> - subclass = __this_cpu_read(wakeup_nest);
> - spin_lock_nested(&wq->lock, subclass + 1);
> - __this_cpu_inc(wakeup_nest);
> + subclass = this_cpu_inc_return(wakeup_nest);
> + /*
> + * Careful: while unlikely, an irq can occur here and mess up
> + * the subclass. The same goes for right before the dec
> + * operation, but that does not matter.
> + */
> + spin_lock_irqsave_nested(&wq->lock, flags, subclass + 1);
> wake_up_locked_poll(wq, POLLIN);
> - __this_cpu_dec(wakeup_nest);
> - spin_unlock(&wq->lock);
> - local_irq_restore(flags);
> - preempt_enable();
> + spin_unlock_irqrestore(&wq->lock, flags);
> +
> + this_cpu_dec(wakeup_nest);
> }
>
> #else
>

\
 
 \ /
  Last update: 2020-01-06 21:30    [W:0.120 / U:1.160 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site