Messages in this thread | | | Date | Thu, 20 Oct 2022 22:13:54 -0700 | From | Ira Weiny <> | Subject | Re: [RFC V2 PATCH 05/11] cxl/mem: Trace General Media Event Record |
| |
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@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.
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
| |