Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 18 Oct 2022 10:46:36 +0100
From:   Jonathan Cameron <Jonathan.Cameron@huawei.com>
To:     Steven Rostedt <rostedt@goodmis.org>
CC:     Ira Weiny <ira.weiny@intel.com>,
        Dan Williams <dan.j.williams@intel.com>,
        Alison Schofield <alison.schofield@intel.com>,
        Vishal Verma <vishal.l.verma@intel.com>,
        Ben Widawsky <bwidawsk@kernel.org>,
        "Davidlohr Bueso" <dave@stgolabs.net>,
        <linux-kernel@vger.kernel.org>, <linux-cxl@vger.kernel.org>
Subject: Re: [RFC V2 PATCH 05/11] cxl/mem: Trace General Media Event Record
Message-ID: <20221018104636.000016fc@huawei.com>
In-Reply-To: <20221017132143.360d2d46@gandalf.local.home>
References: <20221010224131.1866246-1-ira.weiny@intel.com>
        <20221010224131.1866246-6-ira.weiny@intel.com>
        <20221011135702.00006f74@huawei.com>
        <Y0nx1UOeYfQu5WYO@iweiny-desk3>
        <20221017173717.00005921@huawei.com>
        <20221017132143.360d2d46@gandalf.local.home>
MIME-Version: 1.0
Content-Type: text/plain; charset="US-ASCII"
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Mon, 17 Oct 2022 13:21:43 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Mon, 17 Oct 2022 17:37:17 +0100
> Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
> 
> > Looking at other similar cases though and we have a lot of use
> > of trace_seq_printf() e.g. libata_trace_parse_status() though note
> > there is some magic macro stuff in include/trace/events/libata.h 
> > to tie that together.
> > https://elixir.bootlin.com/linux/latest/source/drivers/ata/libata-trace.c#L14
> > 
> > That seems to get you access to the actual buffer we are printing into
> > in similar cases.  
> 
> Looking at the code you linked to, I wonder why __print_flags() wasn't used?
> 
> For instance, you have:
> 
> const char *
> libata_trace_parse_status(struct trace_seq *p, unsigned char status)
> {
>         const char *ret = trace_seq_buffer_ptr(p);
> 
>         trace_seq_printf(p, "{ ");
>         if (status & ATA_BUSY)
>                 trace_seq_printf(p, "BUSY ");
>         if (status & ATA_DRDY)
>                 trace_seq_printf(p, "DRDY ");
>         if (status & ATA_DF)
>                 trace_seq_printf(p, "DF ");
>         if (status & ATA_DSC)
>                 trace_seq_printf(p, "DSC ");
>         if (status & ATA_DRQ)
>                 trace_seq_printf(p, "DRQ ");
>         if (status & ATA_CORR)
>                 trace_seq_printf(p, "CORR ");
>         if (status & ATA_SENSE)
>                 trace_seq_printf(p, "SENSE ");
>         if (status & ATA_ERR)
>                 trace_seq_printf(p, "ERR ");
>         trace_seq_putc(p, '}');
>         trace_seq_putc(p, 0);
> 
>         return ret;
> }
> 
> 
> Which is just a re-implementation of:
> 
> __print_flags(status, " ", 
> 	{ ATA_BUSY, "BUSY" },
> 	{ ATA_DRDY, "DRDY" },
> 	{ ATA_DF, "DF" },
> 	{ ATA_DSC, "DSC" },
> 	{ ATA_DRQ, "DRQ" },
> 	{ ATA_CORR, "CORR" },
> 	{ ATA_SENSE, "SENSE" },
> 	{ ATA_ERR, "ERR" })
> 
> 
> The major difference between the two, is that libtraceevent will be able to
> parse the above and convert the status bits into strings, whereas using
> libata_trace_parse_status() will just give you a parsing error.
> 
> That is, perf and trace-cmd will not be able to parse it unless you write a
> separate plugin for libtraceevent to do it but that means you'll have
> duplicate code.
> 
> I know you just want echo and cat, but that will still work, and this will
> make it work for the tooling as well.

Excellent point, though in the case we are interested in for CXL,
__print_flags() is not enough.

We have a mass of fields that only contain something useful to print if
the valid bits in a mask are set. I just pulled that example to
show how trace_seq_printf() could be used to achieve optional printing
as opposed to current situation where the reader of the print has
to interpret the mask to work out if fields contain anything useful.

To do something nice with them in perf (well probably ras-daemon in
this case) we'll have to parse the valid bits anyway so effectively
write such a plugin.  There we need to do a bunch of mangling to get
the events stored in a DB anyway, so this isn't a huge overhead.

Jonathan


> 
> -- Steve
>