From: Sai Prakash Ranjan <quic_saipraka@quicinc.com>
To: Arnd Bergmann <arnd@arndb.de>
Cc: Will Deacon <will@kernel.org>,
Steven Rostedt <rostedt@goodmis.org>,
Catalin Marinas <catalin.marinas@arm.com>,
<quic_psodagud@quicinc.com>, "Marc Zyngier" <maz@kernel.org>,
gregkh <gregkh@linuxfoundation.org>,
Linux ARM <linux-arm-kernel@lists.infradead.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
linux-arm-msm <linux-arm-msm@vger.kernel.org>,
Ingo Molnar <mingo@redhat.com>
Subject: Re: [PATCHv4 2/2] arm64/io: Add a header for mmio access instrumentation
Date: Mon, 22 Nov 2021 19:05:27 +0530 [thread overview]
Message-ID: <1609f1f7-6f61-6e17-d907-c526f09bffe5@quicinc.com> (raw)
In-Reply-To: <b07e339c-530d-683c-c626-14b73b42e72a@quicinc.com>
On 11/19/2021 9:36 AM, Sai Prakash Ranjan wrote:
> Hi Arnd,
>
> On 11/18/2021 8:54 PM, Arnd Bergmann wrote:
>> On Mon, Nov 15, 2021 at 12:33 PM Sai Prakash Ranjan
>> <quic_saipraka@quicinc.com> wrote:
>>> /*
>>> * Generic IO read/write. These perform native-endian accesses.
>>> */
>>> -#define __raw_writeb __raw_writeb
>>> -static inline void __raw_writeb(u8 val, volatile void __iomem *addr)
>>> +static inline void arch_raw_writeb(u8 val, volatile void __iomem
>>> *addr)
>>> {
>>> asm volatile("strb %w0, [%1]" : : "rZ" (val), "r" (addr));
>>> }
>> Woundn't removing the #define here will break the logic in
>> include/asm-generic/io.h,
>> making it fall back to the pointer-dereference version for the actual
>> access?
>
> #defines for these are added in mmio-instrumented.h header which is
> included in
> arm64/asm/io.h, so it won't break the logic by falling back to
> pointer-dereference.
>
>>> +#if IS_ENABLED(CONFIG_TRACE_MMIO_ACCESS) &&
>>> !(defined(__DISABLE_TRACE_MMIO__))
>>> +DECLARE_TRACEPOINT(rwmmio_write);
>>> +DECLARE_TRACEPOINT(rwmmio_read);
>>> +
>>> +void log_write_mmio(const char *width, volatile void __iomem *addr);
>>> +void log_read_mmio(const char *width, const volatile void __iomem
>>> *addr);
>>> +
>>> +#define __raw_write(v, a, _l) ({ \
>>> + volatile void __iomem *_a = (a); \
>>> + if (tracepoint_enabled(rwmmio_write)) \
>>> + log_write_mmio(__stringify(write##_l), _a); \
>>> + arch_raw_write##_l((v), _a); \
>>> + })
>> This feels like it's getting too big to be inlined. Have you considered
>> integrating this with the lib/logic_iomem.c infrastructure instead?
>>
>> That already provides a way to override MMIO areas, and it lets you do
>> the logging from a single place rather than having it duplicated in
>> every
>> single caller. It also provides a way of filtering it based on the
>> ioremap()
>> call.
>>
>
> Thanks for the suggestion, will look at the logic_iomem.c and see if
> it fits our
> usecase.
>
>
So I looked at logic_iomem.c which seems to be useful for emulated IO
for virtio drivers
but our usecase just needs to log the mmio operations and no additional
stuff, similar to
the logging access of x86 msr registers via tracepoint
(arch/x86/include/asm/msr-trace.h).
Also raw read/write macros in logic_iomem.c have the callbacks which
seems to be pretty costly
than inlining or direct function call given it has to be called for
every register read and write
which are going to be thousands in our case. In their usecase, read and
write callbacks are just
pci cfgspace reads and writes which may not be that frequently called
and the latency might not
be visible but in our case, I think it would be visible if we have a
callback as such. I know this is a
debug feature and perf isn't expected much but that wouldn't mean we
should not have a debug
feature which performs better right.
On the second point, filtering by ioremap isn't much useful for our
usecase since ioremapped
region can have 100s of registers and we are interested in the exact
register read/write which
would cause any of the issues mentioned in the description of this patchset.
So I feel like the current way where we consolidate the instrumentation
in mmio-instrumented.h
seems like the better way than adding tracing to an emulated iomem library.
Thanks,
Sai
next prev parent reply other threads:[~2021-11-22 13:35 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-15 11:33 [PATCHv4 0/2] tracing/rwmmio/arm64: Add support to trace register reads/writes Sai Prakash Ranjan
2021-11-15 11:33 ` [PATCHv4 1/2] tracing: Add register read/write tracing support Sai Prakash Ranjan
2021-11-19 13:43 ` Marc Zyngier
2021-11-19 14:07 ` Sai Prakash Ranjan
2021-11-19 14:17 ` Marc Zyngier
2021-11-19 15:19 ` Sai Prakash Ranjan
2021-11-15 11:33 ` [PATCHv4 2/2] arm64/io: Add a header for mmio access instrumentation Sai Prakash Ranjan
2021-11-16 22:40 ` Steven Rostedt
2021-11-17 3:53 ` Sai Prakash Ranjan
2021-11-18 14:58 ` kernel test robot
2021-11-18 15:24 ` Arnd Bergmann
2021-11-19 4:06 ` Sai Prakash Ranjan
2021-11-22 13:35 ` Sai Prakash Ranjan [this message]
2021-11-22 13:59 ` Arnd Bergmann
2021-11-22 14:19 ` Sai Prakash Ranjan
2021-11-22 14:30 ` Arnd Bergmann
2021-11-22 14:59 ` Sai Prakash Ranjan
2021-11-22 15:35 ` Arnd Bergmann
2021-11-22 15:43 ` Sai Prakash Ranjan
2021-11-29 13:49 ` Sai Prakash Ranjan
2021-11-19 13:48 ` Marc Zyngier
2021-11-19 14:09 ` Sai Prakash Ranjan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1609f1f7-6f61-6e17-d907-c526f09bffe5@quicinc.com \
--to=quic_saipraka@quicinc.com \
--cc=arnd@arndb.de \
--cc=catalin.marinas@arm.com \
--cc=gregkh@linuxfoundation.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=maz@kernel.org \
--cc=mingo@redhat.com \
--cc=quic_psodagud@quicinc.com \
--cc=rostedt@goodmis.org \
--cc=will@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).