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:   Thu, 20 Oct 2022 22:13:54 -0700
From:   Ira Weiny <ira.weiny@intel.com>
To:     Jonathan Cameron <Jonathan.Cameron@huawei.com>
CC:     Steven Rostedt <rostedt@goodmis.org>,
        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: <Y1Iqkmq5qho10bmm@iweiny-desk3>
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>
 <20221018104636.000016fc@huawei.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20221018104636.000016fc@huawei.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?YlQr4luDfkaYpLM4OfvqHUXzycVOSKRVuRXwAWTOfCqW+jG3G8kSEYnzYkzq?=
 =?us-ascii?Q?oYF9M6ruc7WvUvHCv2Inr0mtWhkIC1MSHUCcFHSgeMKw3ITxW/K0kYjMNSqY?=
 =?us-ascii?Q?1HEXdoAOoghxoZtYH0h6HtEgWUp0Uea5Uh/D5kmwOXGrS39+siImn+FF7X3x?=
 =?us-ascii?Q?bPRxd4GbZSYN8d8NjxjZhqm0LuBqe1exXndLppmtTANu8VN8EXljf8A+lSOQ?=
 =?us-ascii?Q?h5qXpjpLC7y3rp8xtJSi7MjVMDsXt78BK55jbFidjeOhuYka+Z2ZAfyMyfzN?=
 =?us-ascii?Q?S5Gc59lHAZOlviwfR1kzkhVB9u7fHNwAiMi/dacFQBrlpese+3hmWGanzeJR?=
 =?us-ascii?Q?wcTFasXCGtHVzRlNZY2tl1k8GCo8M5Rjh0fPAU/cTRcgv4cmTbAOoP2cYidk?=
 =?us-ascii?Q?BkJ7GRD2RAzHUnN3c5Wyq+9BLH4uDCLQndEF6jj18FJgbCsMIQQvNO/UstQq?=
 =?us-ascii?Q?ffo7hNo4RnWN7lhAwNsSYXkp4MnIl2ZkWqp2Ew/moqUYp/YxLE8mNoIXQsVY?=
 =?us-ascii?Q?kn5KVzPEPCbEMLhGq9MaQaQybRKb7CtLQ9+MsJNWdUtmhbtY2byarCGzl29d?=
 =?us-ascii?Q?bZCT9foqyNaWNlJimgCpCYrA0ZmW/AFBtBVyod7h84/4bRQrJfAD76616Cqk?=
 =?us-ascii?Q?9ItaJby+j8bUuWXWNy88iXY7gimK/yzWCs/JG8MWXgXq5uVTXUk1VeVEYNnM?=
 =?us-ascii?Q?f9YXiwPDCInh1U3MtuRhjAXopX7MlLImWyhkOpl25DoTAKY/RiEFYi3A/PhH?=
 =?us-ascii?Q?AVU+7eYSD0MhU/r7g9aAiKsAlciFPmVZE0QsEiotpVreUl7OquTPhxbYVAfe?=
 =?us-ascii?Q?BwJprEw/5CGQ28pf5HKxTeuNpHyxlIQQUIP64Zqsrrb9a9zSCz/7yuSnN/7j?=
 =?us-ascii?Q?3TdxvUn3cPvMfsiDEaiA030en2MZQs6hn9duFI6iwSYm+t449Wl7NsOQSGmy?=
 =?us-ascii?Q?cItC5xGrb34jVuMAmIn5UkG8m80pvabz7lUZq1oCKTMj5lZNq1n/sCd+Ih8D?=
 =?us-ascii?Q?q5pXv3KIeJA7OI05eMgiSKkloTh6pHC8AXGuH5+lUm1lO954T1KVR8Iw+n2P?=
 =?us-ascii?Q?v0MPF3pOu7x9gh6paxIhiDFy/WHJmNhLoDLVWVWQWKW2pDwTl0vVTsTrjJ+H?=
 =?us-ascii?Q?g+ic4rA2ZJZOAX0nn3UvKdXMSfamkf1clsvsHqzRcOpa8pWQ2KRBrf8RXI0E?=
 =?us-ascii?Q?zqWxcGDc6LzxnK7KbI22BGIxOYm1KQ6hzcpIpqCTNrMVMEoje5cpwfkpI7L4?=
 =?us-ascii?Q?25Vgx8ILWDLp3mxPYoi6WVEG/59l04AFp8GdW+Q671EvAGFV+WHJI+u167wg?=
 =?us-ascii?Q?iMpVBowfQj+vIHsIVNoOQp6AACrMB52pMRF2Hox6JGPvhdYFlCkm+3FeL2do?=
 =?us-ascii?Q?WAwmQRTlZ0Tywd9rKgIGC2oBWbqqC6rvmo409g8tr1KhH4bcAmm8nnznEB9z?=
 =?us-ascii?Q?WpXDVQRgl37aMHFk4mEEnheu8Kv0xg8dofA/NMmduyReaRIxubiDHFJhymis?=
 =?us-ascii?Q?yRk4pB906ioqsrod7ecIXIPH0ASHAEpIvTTVQT+G6yfjUqMh2uDOn5gtM/qn?=
 =?us-ascii?Q?SmH7wFOs+/eOoiu+gc8sJeSTutaBAEFdy9NfCnfA?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 6e8aac61-be58-4801-5ac8-08dab3230eab
X-MS-Exchange-CrossTenant-AuthSource: SA1PR11MB6733.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 Oct 2022 05:13:57.9090
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: O+QGFoV7g6eIvcmuL78uIwfBoySj6Ahq+tLBGRBjqBjfHpEZ21NktMB1U46t2CaC5HlV1F9KLnodrtm4Jv97sg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR11MB4593
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-7.3 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_HI,
        SPF_HELO_NONE,SPF_NONE,URIBL_BLOCKED autolearn=ham autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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