Re: [RFC V2 PATCH 05/11] cxl/mem: Trace General Media Event Record

[Ira Weiny]

On Tue, Oct 18, 2022 at 10:46:36AM +0100, Jonathan Cameron wrote:
> On Mon, 17 Oct 2022 13:21:43 -0400
> Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
> 
> > On Mon, 17 Oct 2022 17:37:17 +0100
> > Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> 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.

Given this information I think I'm going to punt on this and take your reviewed
by on the code as it is.

We can certainly try to change it later but for now I think it serves our
purpose.  Better to focus on getting the code working with irq's.

Ira