On Tue, 10 Nov 2020 at 15:25, Dmitry Vyukov <dvyukov@xxxxxxxxxx> wrote: > On Tue, Nov 10, 2020 at 2:53 PM Marco Elver <elver@xxxxxxxxxx> wrote: > > To toggle the allocation gates, we set up a delayed work that calls > > toggle_allocation_gate(). Here we use wait_event() to await an > > allocation and subsequently disable the static branch again. However, if > > the kernel has stopped doing allocations entirely, we'd wait > > indefinitely, and stall the worker task. This may also result in the > > appropriate warnings if CONFIG_DETECT_HUNG_TASK=y. > > > > Therefore, introduce a 1 second timeout and use wait_event_timeout(). If > > the timeout is reached, the static branch is disabled and a new delayed > > work is scheduled to try setting up an allocation at a later time. > > > > Note that, this scenario is very unlikely during normal workloads once > > the kernel has booted and user space tasks are running. It can, however, > > happen during early boot after KFENCE has been enabled, when e.g. > > running tests that do not result in any allocations. > > > > Link: https://lkml.kernel.org/r/CADYN=9J0DQhizAGB0-jz4HOBBh+05kMBXb4c0cXMS7Qi5NAJiw@xxxxxxxxxxxxxx > > Reported-by: Anders Roxell <anders.roxell@xxxxxxxxxx> > > Signed-off-by: Marco Elver <elver@xxxxxxxxxx> > > --- > > mm/kfence/core.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/mm/kfence/core.c b/mm/kfence/core.c > > index 9358f42a9a9e..933b197b8634 100644 > > --- a/mm/kfence/core.c > > +++ b/mm/kfence/core.c > > @@ -592,7 +592,11 @@ static void toggle_allocation_gate(struct work_struct *work) > > /* Enable static key, and await allocation to happen. */ > > atomic_set(&allocation_gate, 0); > > static_branch_enable(&kfence_allocation_key); > > - wait_event(allocation_wait, atomic_read(&allocation_gate) != 0); > > + /* > > + * Await an allocation. Timeout after 1 second, in case the kernel stops > > + * doing allocations, to avoid stalling this worker task for too long. > > + */ > > + wait_event_timeout(allocation_wait, atomic_read(&allocation_gate) != 0, HZ); > > I wonder what happens if we get an allocation right when the timeout fires. > Consider, another task already went to the slow path and is about to > wake this task. This task wakes on timeout and subsequently enables > static branch again. Now we can have 2 tasks on the slow path that > both will wake this task. How will it be handled? Can it lead to some > warnings or something? wake_up() does not require tasks to be in the wait queue, nor is there any requirement that it's exclusive (it takes the appropriate locks unlike wake_up_locked()). One of the wake_up() calls will wake the task, and the other is a noop. So this will work just fine. Thanks, -- Marco
