Message-ID: <20221101165528.GB69@W11-BEAU-MD.localdomain>
Date:   Tue, 1 Nov 2022 09:55:28 -0700
From:   Beau Belgrave <beaub@...ux.microsoft.com>
To:     Masami Hiramatsu <mhiramat@...nel.org>
Cc:     rostedt@...dmis.org, mathieu.desnoyers@...icios.com,
        dcook@...ux.microsoft.com, alanau@...ux.microsoft.com,
        linux-trace-devel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 0/2] tracing/user_events: Remote write ABI

On Tue, Nov 01, 2022 at 10:52:20PM +0900, Masami Hiramatsu wrote:
> On Mon, 31 Oct 2022 10:27:06 -0700
> Beau Belgrave <beaub@...ux.microsoft.com> wrote:
> 
> > On Mon, Oct 31, 2022 at 11:15:56PM +0900, Masami Hiramatsu wrote:
> > > Hi Beau,
> > > 
> > > On Thu, 27 Oct 2022 15:40:09 -0700
> > > Beau Belgrave <beaub@...ux.microsoft.com> wrote:
> > > 
> > > > As part of the discussions for user_events aligned with user space
> > > > tracers, it was determined that user programs should register a 32-bit
> > > > value to set or clear a bit when an event becomes enabled. Currently a
> > > > shared page is being used that requires mmap().
> > > > 
> > > > In this new model during the event registration from user programs 2 new
> > > > values are specified. The first is the address to update when the event
> > > > is either enabled or disabled. The second is the bit to set/clear to
> > > > reflect the event being enabled. This allows for a local 32-bit value in
> > > > user programs to support both kernel and user tracers. As an example,
> > > > setting bit 31 for kernel tracers when the event becomes enabled allows
> > > > for user tracers to use the other bits for ref counts or other flags.
> > > > The kernel side updates the bit atomically, user programs need to also
> > > > update these values atomically.
> > > 
> > > I think you means the kernel tracer (ftrace/perf) and user tracers (e.g. 
> > > LTTng) use the same 32bit data so that traced user-application only checks
> > > that data for checking an event is enabled, right?
> > > 
> > 
> > Yes, exactly, user code can just check a single uint32 or uint64 to tell
> > if anything is enabled (kernel or user tracer).
> > 
> > > If so, who the user tracer threads updates the data bit? Is that thread
> > > safe to update both kernel tracer and user tracers at the same time?
> > > 
> > 
> > This is why atomics are used to set the bit on the kernel side. The user
> > side should do the same. This is like the futex code. Do you see a
> > problem with atomics being used between user and kernel space on a
> > shared 32/64-bit address?
> 
> Ah, OK. set_bit()/clear_bit() are atomic ops. So the user tracer must
> use per-arch atomic ops implementation too. Hmm, can you comment it there?
> 

I can add a comment here, I also plan to update our documentation. I
really want to get good feedback on this first, so I avoid updating the
documentation several times as we progress this conversation. Expect
documentation updates when I flip from RFC to normal patchset.

> > 
> > > And what is the actual advantage of this change? Are there any issue
> > > to use mmaped page? I would like to know more background of this
> > > change.
> > > 
> > 
> > Without this change user tracers like LTTng will have to check 2 values
> > instead of 1 to tell if the kernel tracer is enabled or not. Mathieu is
> > working on a user side tracing library in an effort to align writing
> > tracing code in user processes that works well for both kernel and user
> > tracers without much effort.
> > 
> > See here:
> > https://github.com/compudj/side
> 
> Thanks for pointing!
> 
> > 
> > Are you proposing we keep the bitmap approach and have side library just
> > hook another branch? Mathieu had issues with that approach during our
> > talks.
> 
> No, that makes things more complicated. We should choose one.
> 

Agree, it seems we are settling behind the user provided address
approach, as long as we can work through fork() and other scenarios.
During the bi-weekly tracefs meetings we've been going back and forth
on which approach to take.

I promised a RFC patch to see how far I could get on this to see what
edge cases exist that we need to work through. Currently fork() seems
the hardest to do with private mappings, but I believe I have a path
forward that I'll put in the next version of this patchset.

> > 
> > > Could you also provide any sample program which I can play it? :)
> > > 
> > 
> > When I make the next patch version, I will update the user_events sample
> > so you'll have something to try out.
> 
> That's helpful for me. We can have the code under tools/tracing/user_events/.
> 

I was planning to update the existing sample at samples/user_events/.
Any reason that location can't be used?

> Thank you,
> 
> > 
> > > > User provided addresses must be aligned on a 32-bit boundary, this
> > > > allows for single page checking and prevents odd behaviors such as a
> > > > 32-bit value straddling 2 pages instead of a single page.
> > > > 
> > > > When page faults are encountered they are done asyncly via a workqueue.
> > > > If the page faults back in, the write update is attempted again. If the
> > > > page cannot fault-in, then we log and wait until the next time the event
> > > > is enabled/disabled. This is to prevent possible infinite loops resulting
> > > > from bad user processes unmapping or changing protection values after
> > > > registering the address.
> > > > 
> > > > NOTE:
> > > > User programs that wish to have the enable bit shared across forks
> > > > either need to use a MAP_SHARED allocated address or register a new
> > > > address and file descriptor. If MAP_SHARED cannot be used or new
> > > > registrations cannot be done, then it's allowable to use MAP_PRIVATE
> > > > as long as the forked children never update the page themselves. Once
> > > > the page has been updated, the page from the parent will be copied over
> > > > to the child. This new copy-on-write page will not receive updates from
> > > > the kernel until another registration has been performed with this new
> > > > address.
> > > > 
> > > > Beau Belgrave (2):
> > > >   tracing/user_events: Use remote writes for event enablement
> > > >   tracing/user_events: Fixup enable faults asyncly
> > > > 
> > > >  include/linux/user_events.h      |  10 +-
> > > >  kernel/trace/trace_events_user.c | 396 ++++++++++++++++++++-----------
> > > >  2 files changed, 270 insertions(+), 136 deletions(-)
> > > > 
> > > > 
> > > > base-commit: 23758867219c8d84c8363316e6dd2f9fd7ae3049
> > > > -- 
> > > > 2.25.1
> > > > 
> > > 
> > > 
> > > -- 
> > > Masami Hiramatsu (Google) <mhiramat@...nel.org>
> > 
> > Thanks,
> > -Beau
> 
> 
> -- 
> Masami Hiramatsu (Google) <mhiramat@...nel.org>

Thanks,
-Beau

Powered by blists - more mailing lists