Messages in this thread | | | Date | Fri, 2 Dec 2022 17:14:27 -0800 | From | Dan Williams <> | Subject | Re: [PATCH V2 03/11] cxl/mem: Implement Clear Event Records command |
| |
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.
| |