lkml.org 
[lkml]   [2022]   [Oct]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [RFC PATCH 2/2] tracing/user_events: Fixup enable faults asyncly
From
On 2022-10-29 10:40, Mathieu Desnoyers wrote:
> On 2022-10-28 18:42, Beau Belgrave wrote:
>> On Fri, Oct 28, 2022 at 06:19:10PM -0400, Mathieu Desnoyers wrote:
>>> On 2022-10-27 18:40, Beau Belgrave wrote:
>>>> When events are enabled within the various tracing facilities, such as
>>>> ftrace/perf, the event_mutex is held. As events are enabled pages are
>>>> accessed. We do not want page faults to occur under this lock. Instead
>>>> queue the fault to a workqueue to be handled in a process context safe
>>>> way without the lock.
>>>>
>>>> The enable address is disabled while the async fault-in occurs. This
>>>> ensures that we don't attempt fault-in more than is necessary. Once the
>>>> page has been faulted in, the address write is attempted again. If the
>>>> page couldn't fault-in, then we wait until the next time the event is
>>>> enabled to prevent any potential infinite loops.
>>>
>>> I'm also unclear about how the system call initiating the enabled state
>>> change is delayed (or not) when a page fault is queued.
>>>
>>
>> It's not, if needed we could call schedule_delayed_work(). However, I
>> don't think we need it. When pin_user_pages_remote is invoked, it's with
>> FOLL_NOFAULT. This will tell us if we need to fault pages in, we then
>> call fixup_user_fault with unlocked value passed. This will cause the
>> fixup to retry and get the page in.
>>
>> It's called out in the comments for this exact purpose (lucked out
>> here):
>> mm/gup.c
>>   * This is meant to be called in the specific scenario where for
>> locking reasons
>>   * we try to access user memory in atomic context (within a
>> pagefault_disable()
>>   * section), this returns -EFAULT, and we want to resolve the user
>> fault before
>>   * trying again.
>>
>> The fault in happens in a workqueue, this is the same way KVM does it's
>> async page fault logic, so it's not a new pattern. As soon as the
>> fault-in is done, we have a page we should be able to use, so we
>> re-attempt the write immediately. If the write fails, another queue
>> happens and we could loop, but not like the unmap() case I replied with
>> earlier.
>>
>>> I would expect that when a page fault is needed, after enqueuing work
>>> to the
>>> worker thread, the system call initiating the state change would somehow
>>> wait for a completion (after releasing the user events mutex). That
>>> completion would be signaled by the worker thread either if the page
>>> fault
>>> fails, or if the state change is done.
>>>
>>
>> I didn't see a generic fault-in + notify anywhere, do you know of one I
>> could use? Otherwise, it seems the pattern used is check fault, fault-in
>> via workqueue, re-attempt.
>
> I don't think we're talking about the same thing. I'll try stating my
> concern in a different way.
>
> user_event_enabler_write() calls user_event_enabler_queue_fault() when
> it cannot perform the enabled state update immediately (because a page
> fault is needed).
>
> But then AFAIU it returns immediately to the caller. The caller could
> very well expect that the event has been enabled, as requested,
> immediately when the enabler write returns. The fact that enabling the
> event can be delayed for an arbitrary amount of time due to page faults
> means that we have no hard guarantee that the event is enabled as
> requested upon return to the caller.
>
> I'd like to add a completion there, to be waited for after
> user_event_enabler_queue_fault(), but before returning from
> user_event_enabler_create(). Waiting for the completion should be done
> without any mutexes held, so after releasing event_mutex.
>
> The completion would be placed on the stack of
> user_event_enabler_create(), and a reference to the completion would be
> placed in the queued fault request. The completion notification would be
> emitted by the worker thread either when enabling is done, or if a page
> fault fails.
>
> See include/linux/completion.h
>
> Thoughts ?

Actually, after further thinking, I wonder if we need a worker thread at
all to perform the page faults.

Could we simply handle the page faults from user_event_enabler_create()
by releasing the mutex and re-trying ? From what contexts is
user_event_enabler_create() called ? (any locks taken ? system call
context ? irqs and preemption enabled or disabled ?)

Thanks,

Mathieu

>
> Thanks,
>
> Mathieu
>
>
>>
>>> Thoughts ?
>>>
>>> Thanks,
>>>
>>> Mathieu
>>>
>>
>> Thanks,
>> -Beau
>

--
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com

\
 
 \ /
  Last update: 2022-10-30 12:46    [W:0.101 / U:0.776 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site