Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Tue, 19 Apr 2022 11:57:08 -0700
From:   Beau Belgrave <beaub@linux.microsoft.com>
To:     Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc:     rostedt <rostedt@goodmis.org>,
        Masami Hiramatsu <mhiramat@kernel.org>,
        linux-trace-devel <linux-trace-devel@vger.kernel.org>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        linux-arch <linux-arch@vger.kernel.org>
Subject: Re: [PATCH 6/7] tracing/user_events: Use bits vs bytes for enabled
 status page data
Message-ID: <20220419185708.GA1908@kbox>
References: <20220401234309.21252-1-beaub@linux.microsoft.com>
 <20220401234309.21252-7-beaub@linux.microsoft.com>
 <337584634.26921.1650378945485.JavaMail.zimbra@efficios.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <337584634.26921.1650378945485.JavaMail.zimbra@efficios.com>
Precedence: bulk

On Tue, Apr 19, 2022 at 10:35:45AM -0400, Mathieu Desnoyers wrote:
> ----- On Apr 1, 2022, at 7:43 PM, Beau Belgrave beaub@linux.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@efficios.com>
> > Signed-off-by: Beau Belgrave <beaub@linux.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.

However, you may be referring to something on the kernel side?

> I'm concerned about the performance penalty associated with partial
> register stalls when working with bytewise ALU operations rather than
> operations using the entire registers.
> 

On the kernel side these only occur when a registration happens (pretty
rare compared to enabled checks) or a delete (even rarer). But I have
the feeling you are more concerned about the user side, right?

> 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.

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

Thanks,
-Beau