Re: [PATCH V2 03/11] cxl/mem: Implement Clear Event Records command

[Ira Weiny <ira.weiny@intel.com>]

On Fri, Dec 02, 2022 at 05:14:27PM -0800, Dan Williams wrote:
> Ira Weiny wrote:
> > On Thu, Dec 01, 2022 at 06:29:20PM -0800, Dan Williams wrote:
> > > ira.weiny@ wrote:
> > > > From: Ira Weiny <ira.weiny@intel.com>
> > > > 
> > > > CXL rev 3.0 section 8.2.9.2.3 defines the Clear Event Records mailbox
> > > > command.  After an event record is read it needs to be cleared from the
> > > > event log.
> > > > 
> > > > Implement cxl_clear_event_record() to clear all record retrieved from
> > > > the device.
> > > > 
> > > > Each record is cleared explicitly.  A clear all bit is specified but
> > > > events could arrive between a get and any final clear all operation.
> > > > This means events would be missed.
> > > > Therefore each event is cleared specifically.
> > > 
> > > Note that the spec has a better reason for why Clear All has limited
> > > usage:
> > > 
> > > "Clear All Events is only allowed when the Event Log has overflowed;
> > > otherwise, the device shall return Invalid Input."
> > > 
> > > Will need to wait and see if we need that to keep pace with a device
> > > with a high event frequency.
> > 
> > Perhaps.  But yea I would wait and see.
> > 
> > [snip]
> > 
> > > > +static int cxl_clear_event_record(struct cxl_dev_state *cxlds,
> > > > +				  enum cxl_event_log_type log,
> > > > +				  struct cxl_get_event_payload *get_pl,
> > > > +				  u16 total)
> > > > +{
> > > > +	struct cxl_mbox_clear_event_payload payload = {
> > > > +		.event_log = log,
> > > > +	};
> > > > +	int cnt;
> > > > +
> > > > +	/*
> > > > +	 * Clear Event Records uses u8 for the handle cnt while Get Event
> > > > +	 * Record can return up to 0xffff records.
> > > > +	 */
> > > > +	for (cnt = 0; cnt < total; /* cnt incremented internally */) {
> > > > +		u8 nr_recs = min_t(u8, (total - cnt),
> > > > +				   CXL_CLEAR_EVENT_MAX_HANDLES);
> > > 
> > > This seems overly complicated. @total is a duplicate of
> > > @get_pl->record_count, and the 2 loops feel like it could be cut
> > > down to one.
> > 
> > Sure, total is redundant to pass to the function.
> > 
> > However, 2 loops is IMO not at all overly complicated.  Note that the 2 loops
> > do not do the same thing.  The inner loop is filling in the payload for the
> > Clear command.  There is really no way around doing this.
> > 
> > Now that I've had time to think about it:
> > 
> > 	Are you suggesting we issue a single mailbox command for every handle?
> > 
> > That would be a single loop.  But a lot more mailbox commands.
> 
> I was thinking something like this pseudo code
> 
> int tosend = le16_to_cpu(get_pl->record_count);
> int added = 0;
> 
>     for (i = 0; i < tosend; i++) {
>     	add_to_clear(added++);
>     	if (added == MAX)
>     		send_mailbox();
> 	added = 0;
>     }
> 
>     if (added)
>     	send_mailbox();
> 
> ...where it batches and sends every 256 and one more send afterwards for
> any stragglers.

Ok I'm not convinced it makes that much difference but I don't have the
fortitude to try and look at the assembly to argue...  ;-)

Done.

Ira