Re: [RFC PATCH 2/2] tracing/user_events: Fixup enable faults asyncly

[Beau Belgrave]

On Sun, Oct 30, 2022 at 07:45:33AM -0400, Mathieu Desnoyers wrote:
> 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 ?)
> 

The create case is in process context, via the reg IOCTL on the
user_events_data file. The only lock is the register lock, the
event_mutex is taken within the user_event_enabler_create() call to ensure
the enabler can safely be added to the shared user_event within the
group.

Shouldn't be a problem running it synchronously again or reporting back
a fault happened and letting the user call decide what to do.

> Thanks,
> 
> Mathieu
> 

[..]

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

Thanks,
-Beau