Message-ID: <1647936661.29102.1650486080281.JavaMail.zimbra@efficios.com>
Date:   Wed, 20 Apr 2022 16:21:20 -0400 (EDT)
From:   Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To:     Beau Belgrave <beaub@...ux.microsoft.com>
Cc:     rostedt <rostedt@...dmis.org>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        linux-trace-devel <linux-trace-devel@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        linux-arch <linux-arch@...r.kernel.org>
Subject: Re: [PATCH 6/7] tracing/user_events: Use bits vs bytes for enabled
 status page data

----- On Apr 20, 2022, at 4:12 PM, Beau Belgrave beaub@...ux.microsoft.com wrote:

> On Wed, Apr 20, 2022 at 01:53:47PM -0400, Mathieu Desnoyers wrote:
>> 
>> 
>> ----- On Apr 19, 2022, at 7:48 PM, Beau Belgrave beaub@...ux.microsoft.com
>> wrote:
>> 
>> > On Tue, Apr 19, 2022 at 05:26:20PM -0400, Mathieu Desnoyers wrote:
>> >> ----- On Apr 19, 2022, at 2:57 PM, Beau Belgrave beaub@...ux.microsoft.com
>> >> wrote:
>> >> 
>> >> > On Tue, Apr 19, 2022 at 10:35:45AM -0400, Mathieu Desnoyers wrote:
>> >> >> ----- On Apr 1, 2022, at 7:43 PM, Beau Belgrave beaub@...ux.microsoft.com wrote:
>> >> >> 
>> >> >> > User processes may require many events and when they do the cache
>> >> >> > performance of a byte index status check is less ideal than a bit index.
>> >> >> > The previous event limit per-page was 4096, the new limit is 32,768.
>> >> >> > 
>> >> >> > This change adds a mask property to the user_reg struct. Programs check
>> >> >> > that the byte at status_index has a bit set by ANDing the status_mask.
>> >> >> > 
>> >> >> > Link:
>> >> >> > https://lore.kernel.org/all/2059213643.196683.1648499088753.JavaMail.zimbra@efficios.com/
>> >> >> > 
>> >> >> > Suggested-by: Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
>> >> >> > Signed-off-by: Beau Belgrave <beaub@...ux.microsoft.com>
>> >> >> 
>> >> >> Hi Beau,
>> >> >> 
>> >> >> Considering this will be used in a fast-path, why choose bytewise
>> >> >> loads for the byte at status_index and the status_mask ?
>> >> >> 
>> >> > 
>> >> > First, thanks for the review!
>> >> > 
>> >> > Which loads are you concerned about? The user programs can store the
>> >> > index and mask in another type after registration instead of an int.
>> >> 
>> >> I'm concerned about the loads from user-space, considering that
>> >> those are on the fast-path.
>> >> 
>> >> Indeed user programs will need to copy the status index and mask
>> >> returned in struct user_reg, so adapting the indexing and mask to
>> >> deal with an array of unsigned long rather than bytes can be done
>> >> at that point, but I wonder how many users will go through that
>> >> extra trouble unless there are helpers to convert the status index
>> >> from byte-wise to long-wise, and convert the status mask from a
>> >> byte-wise mask to a long-wise mask (and associated documentation).
>> >> 
>> > 
>> > Yeah, do you think it's wise to maybe add inline functions in
>> > user_events.h to do this conversion? I could then add them to our
>> > documentation.
>> > 
>> > Hopefully this would make more APIs/people do the better approach?
>> > 
>> >> 
>> >> > 
>> >> > However, you may be referring to something on the kernel side?
>> >> 
>> >> No.
>> >> 
>> > 
>> > [..]
>> > 
>> >> >> Ideally I would be tempted to use "unsigned long" type (32-bit on 32-bit
>> >> >> binaries and 64-bit on 64-bit binaries) for both the array access
>> >> >> and the status mask, but this brings extra complexity for 32-bit compat
>> >> >> handling.
>> >> >> 
>> >> > 
>> >> > User programs can store the index and mask returned into better value
>> >> > types for their architecture.
>> >> > 
>> >> > I agree it will cause compat handling issues if it's put into the user
>> >> > facing header as a long.
>> >> > 
>> >> > I was hoping APIs, like libtracefs, could abstract many callers from how
>> >> > best to use the returned values. For example, it could save the index
>> >> > and mask as unsigned long for the callers and use those for the
>> >> > enablement checks.
>> >> > 
>> >> > Do you think there is a way to enable these native types in the ABI
>> >> > without causing compat handling issues? I used ints to prevent compat
>> >> > issues between 32-bit user mode and 64-bit kernel mode.
>> >> 
>> >> I think you are right: this is not an ABI issue, but rather a usability
>> >> issue that can be solved by implementing and documenting user-space library
>> >> helpers to help user applications index the array and apply the mask to an
>> >> unsigned long type.
>> >> 
>> > 
>> > Great. Let me know if updating user_events.h to do the conversion is a
>> > good idea or not, or if you have other thoughts how to make more people
>> > do the best thing.
>> 
>> Usually uapi headers are reserved for exposing the kernel ABI to user-space.
>> I think the helpers we are discussing here do not belong to the uapi because
>> they
>> do not define the ABI, and should probably sit elsewhere in a proper library.
>> 
> 
> Makes sense.
> 
> That likely means I should remove the enablement helper check from
> user_events.h, right?

Yes, I would be tempted to remove it, and document the bitwise index ABI
instead.

> 
>> If the status_mask is meant to be modified in some ways by user-space before it
>> can
>> be used as a mask, I wonder why it is exposed as a byte-wise mask at all ?
>> 
>> Rather than exposing a byte-wise index and single-byte mask as ABI, the kernel
>> could
>> simply expose a bit-wise index, which can then be used by the application to
>> calculate
>> index and mask, which it should interpret in little endian if it wants to apply
>> the
>> mask on types larger than a single byte.
>> 
>> Thoughts ?
>> 
> 
> Yeah, you're right, we can just expose out the bit-wise index at the
> ABI.
> 
> I'll switch over to that model in the next version.

Allright !

Thanks,

Mathieu

> 
> Thanks,
> -Beau
> 
>> Thanks,
>> 
>> Mathieu
>> 
> 
> [..]

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

Powered by blists - more mailing lists