Re: [PATCH 1/3] kfence: await for allocation using wait_event

[Marco Elver]

On Wed, Apr 21, 2021 at 05:11PM +0800, Hillf Danton wrote:
> On Mon, 19 Apr 2021 11:49:04 Marco Elver wrote:
> >On Mon, 19 Apr 2021 at 11:44, Marco Elver <elver@xxxxxxxxxx> wrote:
> >> On Mon, 19 Apr 2021 at 11:41, Hillf Danton <hdanton@xxxxxxxx> wrote:
> >> > On Mon, 19 Apr 2021 10:50:25 Marco Elver wrote:
> >> > > +
> >> > > +     WRITE_ONCE(kfence_timer_waiting, true);
> >> > > +     smp_mb(); /* See comment in __kfence_alloc(). */
> >> >
> >> > This is not needed given task state change in wait_event().
> >>
> >> Yes it is. We want to avoid the unconditional irq_work in
> >> __kfence_alloc(). When the system is under load doing frequent
> >> allocations, at least in my tests this avoids the irq_work almost
> >> always. Without the irq_work you'd be correct of course.
> >
> >And in case this is about the smp_mb() here, yes it definitely is
> >required. We *must* order the write of kfence_timer_waiting *before*
> >the check of kfence_allocation_gate, which wait_event() does before
> >anything else (including changing the state).
> 
> One of the reasons why wait_event() checks the wait condition before anything
> else is no waker can help waiter before waiter gets themselves on the
> wait queue head list. Nor can waker without scheduling on the waiter
> side, even if the waiter is sitting on the list. So the mb cannot make sense
> without scheduling, let alone the mb in wait_event().

You are right of course. I just went and expanded wait_event():

	do {
		if (atomic_read(&kfence_allocation_gate))
			break;
		init_wait_entry(...);
		for (;;) {
			long __int = prepare_to_wait_event(...);
			if (atomic_read(&kfence_allocation_gate))
				break;
			...
			schedule();
		}
		finish_wait(...);
	} while (0);

I just kept looking at the first check. Before the wait entry setup and
finally the second re-check after the mb() in prepare_to_wait_event().
So removing the smp_mb() is indeed fine given the second re-check is
ordered after the write per state change mb().

And then I just saw we should just use waitqueue_active() anyway, which
documents this, too.

I'll send a v2.

Thank you!

-- Marco