Re: [PATCH RFC v2 16/18] tools/testing/cxl: Make event logs dynamic

From: Ira Weiny <ira.weiny@intel.com>
To: Jonathan Cameron <Jonathan.Cameron@huawei.com>,
	Ira Weiny <ira.weiny@intel.com>
Cc: Dan Williams <dan.j.williams@intel.com>,
	Navneet Singh <navneet.singh@intel.com>,
	Fan Ni <fan.ni@samsung.com>, Davidlohr Bueso <dave@stgolabs.net>,
	Dave Jiang <dave.jiang@intel.com>,
	Alison Schofield <alison.schofield@intel.com>,
	Vishal Verma <vishal.l.verma@intel.com>,
	<linux-cxl@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH RFC v2 16/18] tools/testing/cxl: Make event logs dynamic
Date: Wed, 6 Sep 2023 14:15:28 -0700	[thread overview]
Message-ID: <64f8ebf0d849a_1e8e782946d@iweiny-mobl.notmuch> (raw)
In-Reply-To: <20230830131147.000034bc@Huawei.com>

Jonathan Cameron wrote:
> On Mon, 28 Aug 2023 22:21:07 -0700
> Ira Weiny <ira.weiny@intel.com> wrote:
> 
> > The test event logs were created as static arrays as an easy way to mock
> > events.  Dynamic Capacity Device (DCD) test support requires events be
> > created dynamically when extents are created/destroyed.
> > 
> > Modify the event log storage to be dynamically allocated.  Thus they can
> > accommodate the dynamic events required by DCD.  Reuse the static event
> > data to create the dynamic events in the new logs without inventing
> > complex event injection through the test sysfs.  Simplify the processing
> > of the logs by using the event log array index as the handle.  Add a
> > lock to manage concurrency to come with DCD extent testing.
> > 
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> Diff did a horrible job on readability of this patch.

Yea apologies.  I'm not sure if b4 can use --patience or not but I tried
patience by hand and it did not do any better.

> 
> Ah well. Comments superficial only.
> 
> Jonathan
> 
> > ---
> >  tools/testing/cxl/test/mem.c | 276 ++++++++++++++++++++++++++-----------------
> >  1 file changed, 170 insertions(+), 106 deletions(-)
> > 
> > diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
> > index 51be202fabd0..6a036c8d215d 100644
> > --- a/tools/testing/cxl/test/mem.c
> > +++ b/tools/testing/cxl/test/mem.c
> > @@ -118,18 +118,27 @@ static struct {
> >  
> >  #define PASS_TRY_LIMIT 3
> >  
> > -#define CXL_TEST_EVENT_CNT_MAX 15
> > +#define CXL_TEST_EVENT_CNT_MAX 17
> >  
> >  /* Set a number of events to return at a time for simulation.  */
> >  #define CXL_TEST_EVENT_CNT 3
> >  
> > +/*
> > + * @next_handle: next handle (index) to be stored to
> > + * @cur_handle: current handle (index) to be returned to the user on get_event
> > + * @nr_events: total events in this log
> > + * @nr_overflow: number of events added past the log size
> > + * @lock: protect these state variables
> > + * @events: array of pending events to be returned.
> > + */
> >  struct mock_event_log {
> > -	u16 clear_idx;
> > -	u16 cur_idx;
> > +	u16 next_handle;
> > +	u16 cur_handle;
> >  	u16 nr_events;
> >  	u16 nr_overflow;
> > -	u16 overflow_reset;
> > -	struct cxl_event_record_raw *events[CXL_TEST_EVENT_CNT_MAX];
> > +	rwlock_t lock;
> > +	/* 1 extra slot to accommodate that handles can't be 0 */
> > +	struct cxl_event_record_raw *events[CXL_TEST_EVENT_CNT_MAX+1];
> 
> Spaces around +

Done.

> 
> >  };
> >  
> 
> ...
> 
> 
> >  
> > -static void cxl_mock_add_event_logs(struct mock_event_store *mes)
> > +/* Create a dynamically allocated event out of a statically defined event. */
> > +static void add_event_from_static(struct mock_event_store *mes,
> > +				  enum cxl_event_log_type log_type,
> > +				  struct cxl_event_record_raw *raw)
> > +{
> > +	struct device *dev = mes->mds->cxlds.dev;
> > +	struct cxl_event_record_raw *rec;
> > +
> > +	rec = devm_kzalloc(dev, sizeof(*rec), GFP_KERNEL);
> > +	if (!rec) {
> > +		dev_err(dev, "Failed to alloc event for log\n");
> > +		return;
> > +	}
> > +
> > +	memcpy(rec, raw, sizeof(*rec));
> 
> devm_kmemdup()?

Yea!  Thanks!

Ira