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

Dan Williams
2022-12-03T01:14:44Z

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.