[lkml]   [2022]   [Oct]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [RFC PATCH 2/2] tracing/user_events: Fixup enable faults asyncly
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 ?



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

Mathieu Desnoyers
EfficiOS Inc.

 \ /
  Last update: 2022-10-29 16:41    [W:0.065 / U:0.536 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site