Re: [PATCH] fs/epoll: rework safewake for CONFIG_DEBUG_LOCK_ALLOC

[Davidlohr Bueso]

On Mon, 06 Jan 2020, Jason Baron wrote:
> > 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.

Yes, this is illegal in -rt as well.

> > 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.

I've also thought about this.

> > 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?

Yes I agree that this will only cause numbers to be skipped, but as mentioned
it's not very tested. I'll go see what comes out with more testing, of course
it means very little unless I can actually reproduce spurious irqs. Maybe I
can hack something up that bumps the subclass intentionally and see what happens
as well.

Thanks,
Davidlohr