Messages in this thread | | | Date | Thu, 15 Sep 2022 11:53:29 -0700 | From | Ira Weiny <> | 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@intel.com wrote: > > > From: Ira Weiny <ira.weiny@intel.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
| |