Message-ID: <YyN0qY5yaXwTwLDF@iweiny-desk3>
Date:   Thu, 15 Sep 2022 11:53:29 -0700
From:   Ira Weiny <ira.weiny@...el.com>
To:     Jonathan Cameron <Jonathan.Cameron@...wei.com>
CC:     Dan Williams <dan.j.williams@...el.com>,
        Alison Schofield <alison.schofield@...el.com>,
        Vishal Verma <vishal.l.verma@...el.com>,
        "Ben Widawsky" <bwidawsk@...nel.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Davidlohr Bueso <dave@...olabs.net>,
        <linux-kernel@...r.kernel.org>, <linux-cxl@...r.kernel.org>
Subject: Re: [RFC PATCH 7/9] cxl/test: Add generic mock events

On Thu, Aug 25, 2022 at 12:31:19PM +0100, Jonathan Cameron wrote:
> On Fri, 12 Aug 2022 22:32:41 -0700
> ira.weiny@...el.com wrote:
> 
> > From: Ira Weiny <ira.weiny@...el.com>
> > 
> > Facilitate testing basic Get/Clear Event functionality by creating
> > multiple logs and generic events with made up UUID's.
> > 
> > Data is completely made up with data patterns which should be easy to
> > spot in trace output.
> Hi Ira,
> 
> I'm tempted to hack the QEMU emulation for this in with appropriately
> complex interface to inject all the record types...

Every time I look at the QEMU code it makes my head spin.  :-(

I really thought about adding some support there.  And I think for irq's it may
work better?  But after your talk today I did a quick search to see what it
would take to do irqs in QEMU and got even more confused.  :-(

> Lots to do there though, so not sure where this fits in my priority list!

I bet it is higher on mine!  ;-)

> 
> > 
> > Test traces are easy to obtain with a small script such as this:
> > 
> > 	#!/bin/bash -x
> > 
> > 	devices=`find /sys/devices/platform -name cxl_mem*`
> > 
> > 	# Generate fake events if reset is passed in
> 
> reset is rather unintuitive naming.
> 
> fill_event_queue maybe or something more in that direction?

Fair enough...  Naming is hard and I'm one of the worst.

I've changed to

<sysfs>/.../event_fill_queue
<sysfs>/.../event_trigger

Thoughts?

[snip]

> >  
> > +/*
> > + * Mock Events
> > + */
> > +struct mock_event_log {
> > +	int cur_event;
> > +	int nr_events;
> > +	struct xarray events;
> 
> I'm not convinced an xarray is appropriate here (I'd have used
> a fixed size array) but meh, I don't care that much and mocking
> code doesn't have to be quick or elegant :)

I rather thought the xarray was more elegant than the fixed array.

> 
> > +};
> > +
> > +struct mock_event_store {
> > +	struct cxl_dev_state *cxlds;
> > +	struct mock_event_log *mock_logs[CXL_EVENT_TYPE_MAX];
> 
> Each entry isn't terribly big and there aren't that many of them.
> Make the code simpler by just embedding the instances here?

That is a good idea.  Not sure any more why I did it this way.

[snip]

> > +
> > +static void event_store_add_event(struct mock_event_store *es,
> > +				  enum cxl_event_log_type log_type,
> > +				  struct cxl_event_record_raw *event)
> > +{
> > +	struct mock_event_log *log;
> > +	struct device *dev = es->cxlds->dev;
> > +	int rc;
> > +
> > +	if (log_type >= CXL_EVENT_TYPE_MAX)
> > +		return;
> > +
> > +	log = es->mock_logs[log_type];
> > +	if (!log) {
> > +		log = devm_kzalloc(dev, sizeof(*log), GFP_KERNEL);
> 
> As above, I'd just embed the logs directly in the containing structure
> rather than allocating on demand. init them all up front.

yep.  Done.

> 
> > +		if (!log) {
> > +			dev_err(dev, "Failed to create %s log\n",
> > +				cxl_event_log_type_str(log_type));
> > +			return;
> > +		}
> > +		xa_init(&log->events);
> > +		devm_add_action(dev, xa_events_destroy, log);
> > +		es->mock_logs[log_type] = log;
> > +	}
> > +
> > +	rc = xa_insert(&log->events, log->nr_events, event, GFP_KERNEL);
> Not sure using an xa for a list really makes that much sense, but
> doesn't matter hugely. 

It is much easier than trying to manage pointers and allows the events to be
inserted more than once.

> > +	if (rc) {
> > +		dev_err(dev, "Failed to store event %s log\n",
> > +			cxl_event_log_type_str(log_type));
> > +		return;
> > +	}
> > +	log->nr_events++;
> 
> Having an index into a static set of events is more complex.
> I'd either switch to a simple array of pointers, or actually add and
> remove events (or pointers to them anyway).

xarray was much easier to deal with than an array of pointers.  Using a list
was hard because I wanted to reuse the static definitions of events rather than
have a bunch of them defined.

[snip]

> > +
> > +/*
> > + * Get and clear event only handle 1 record at a time as this is what is
> > + * currently implemented in the main code.
> 
> Duplicating this comment seems unnecessary.

I wanted to make it clear this test code could only test what was currently
implemented...

>  
> > + */
> > +static int mock_clear_event(struct cxl_dev_state *cxlds,
> > +			    struct cxl_mbox_cmd *cmd)
> > +{
> > +	struct cxl_mbox_clear_event_payload *pl = cmd->payload_in;
> > +	struct mock_event_log *log;
> > +	u8 log_type = pl->event_log;
> > +
> > +	/* Don't handle more than 1 record at a time */
> > +	if (pl->nr_recs != 1)
> > +		return -EINVAL;

... and this check ...

> > +
> > +	if (log_type >= CXL_EVENT_TYPE_MAX)
> > +		return -EINVAL;
> > +
> > +	log = find_event_log(cxlds, log_type);
> > +	if (!log)
> > +		return 0; /* No mock data in this log */
> > +
> > +	/*
> > +	 * The current code clears events as they are read
> > +	 * Test that behavior; not clearning from the middle of the log
> > +	 */

... and this one; prevents it from blowing up.

[snip]

> > +
> > +static void devm_cxl_mock_event_logs(struct cxl_memdev *cxlmd)
> > +{
> > +	struct device *dev = &cxlmd->dev;
> > +	struct mock_event_store *es;
> > +
> > +	/*
> > +	 * The memory device gets the sysfs attributes such that the cxlmd
> > +	 * pointer can be used to get to a cxlds pointer.
> > +	 */
> > +	if (device_add_groups(dev, cxl_mock_event_groups))
> 
> Whilst it might not matter in a mocking driver, it's normal to jump through
> hoops to avoid doing this because it races with userspace notifications in
> all sorts of hideous ways.  It makes the sysfs maintainers very grumpy ;)

<sigh> I know this is a hack...  I really wanted to hang this off of cxlds but
it did not make sense.

> To do it here, you would need to pass the group to devm_cxl_add_memdev()
> and have that slip it in before the cdev_device_add() call I think.
> That wouldn't be particular invasive though. 

I guess that would work and yea I guess it is not too invasive.

I'll throw it together for the next version and see how it looks/works.

> 
> 
> > +		return;
> > +	if (devm_add_action_or_reset(dev, remove_mock_event_groups, dev))
> > +		return;
> > +
> > +	/*
> > +	 * All the mock event data hangs off the device itself.
> 
> Nitpick of the day: Single line comment syntax ;)

:-D

Done.

Thanks again for the review!
Ira

Powered by blists - more mailing lists