Message-ID: <20221121104714.00003bab@Huawei.com>
Date:   Mon, 21 Nov 2022 10:47:14 +0000
From:   Jonathan Cameron <Jonathan.Cameron@...wei.com>
To:     Ira Weiny <ira.weiny@...el.com>
CC:     Dan Williams <dan.j.williams@...el.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Alison Schofield <alison.schofield@...el.com>,
        "Vishal Verma" <vishal.l.verma@...el.com>,
        Ben Widawsky <bwidawsk@...nel.org>,
        Davidlohr Bueso <dave@...olabs.net>,
        <linux-kernel@...r.kernel.org>, <linux-cxl@...r.kernel.org>
Subject: Re: [PATCH 02/11] cxl/mem: Implement Get Event Records command

On Fri, 18 Nov 2022 15:26:17 -0800
Ira Weiny <ira.weiny@...el.com> wrote:

> On Thu, Nov 17, 2022 at 10:43:37AM +0000, Jonathan Cameron wrote:
> > On Wed, 16 Nov 2022 16:47:20 -0800
> > Ira Weiny <ira.weiny@...el.com> wrote:
> > 
> >   
> 
> [snip]
> 
> > >   
> > > >     
> > > > > +			int i;
> > > > > +
> > > > > +			for (i = 0; i < nr_rec; i++)
> > > > > +				trace_cxl_generic_event(dev_name(cxlds->dev),
> > > > > +							type,
> > > > > +							&payload.record[i]);
> > > > > +		}
> > > > > +
> > > > > +		if (trace_cxl_overflow_enabled() &&
> > > > > +		    (payload.flags & CXL_GET_EVENT_FLAG_OVERFLOW))
> > > > > +			trace_cxl_overflow(dev_name(cxlds->dev), type, &payload);
> > > > > +
> > > > > +	} while (pl_nr > CXL_GET_EVENT_NR_RECORDS ||    
> > > > 
> > > > Isn't pl_nr > CXL_GET_EVENT_NR_RECORDS a hardware bug? It's the number in returned
> > > > payload not the total number.    
> > > 
> > > I don't think so.  The only value passed to the device is the _input_ payload
> > > size.  The output payload size is not passed to the device and is not included
> > > in the Get Event Records Input Payload.  (Table 8-49)
> > > 
> > > So my previous code was wrong.  Here is an example I think which is within the
> > > spec but would result in the more records flag not being set.
> > > 
> > > 	Device log depth == 10
> > > 	nr log entries == 7
> > > 	nr log entries in 1MB ~= (1M - hdr size) / 128 ~= 8000
> > > 
> > > Device sets Output Payload.Event Record Count == 7 (which is < 8000).  Common
> > > mailbox code truncates that to 3.  More Event Records == 0 because it sent all
> > > 7 that it had.
> > > 
> > > This code will clear 3 and read again 2 more times.
> > > 
> > > Am I reading that wrong?  
> > 
> > I think this is still wrong, but for a different reason. :)  
> 
> I hope not...  :-/
> 
> > If we don't clear the records and more records is set, that means it didn't
> > fit in the mailbox payload (potentially 1MB)  then the next read
> > will return the next set of records from there.  
> 
> That is not how I read the Get Event Records command:
> 
> From 8.2.9.2.2 Get Event Records
> 
> ... "Devices shall return event records to the host in the temporal order the
> device detected the events in. The event occurring the earliest in time, in the
> specific event log, shall be returned first."
> 
> If item 3 below is earlier than 4 then it must be returned if we have not
> cleared it.  At least that is how I read the above.  :-/

In general that doesn't work.  Imagine we cleared no records.
In that case we'd return 4 despite there being earlier records.
There is no language to cover this particular case of clearing
part of what was returned.  The device did return the records
in temporal order, we just didn't notice some of them.

The wonders of slightly loose spec wording.  Far as I can tell
we are stuck with having to come with all things that could be
read as being valid implementations.

> 
> > 
> > Taking this patch only, let's say the mailbox takes 4 records.
> > Read 1: Records 0, 1, 2, 3 More set.
> >    We handle 0, 1, 2
> > Read 2: Records 4, 5, 6 More not set.
> >    We handle 4, 5, 6
> > 
> > Record 3 is never handled.
> > 
> > If we add in clearing as happens later in the series,  
> 
> I suppose I should squash the patches as this may not work without the
> clearing.  :-/
> 
> > the current
> > assumption is that if we clear some records a subsequent read will
> > start again.  I'm not sure that is true. If it is spec reference needed.
> > 
> > So assumption is
> > Read 1: Records 0, 1, 2, 3 More set
> >   Clear 0, 1, 2
> > Read 2: Records 3, 4, 5, 6
> >   Clear 3, 4, 5 More not set, but catch it with the condition above.
> > Read 3: 6 only
> >   Clear 6
> > 
> > However, I think a valid implementation could do the following
> > (imagine a ring buffer with a pointer to the 'next' record to read out and
> >  each record has a 'valid' flag to deal with corner cases around
> >  sequences such as read log once, start reading again and some
> >  clears occur using handles obtained from first read - not that
> >  case isn't ruled out by the spec as far as I can see).  
> 
> I believe this is a violation because the next pointer can't be advanced until
> the record is cleared.  Otherwise the device is not returning items in temporal
> order based on what is in the log.

Ah. This is where we disagree.  The temporal order is (potentially?) unconnected
from the clearing.  The device did return them in temporal order, we just didn't
take any novice of record 3 being returned.
A valid reading of that temporal order comment is actually the other way around
that the device must not reset it's idea of temporal order until all records
have been read (reading 3 twice is not in temporal order - imagine we had
read 5 each time and it becomes more obvious as the read order becomes
0,1,2,3,4,3,4,5,6,7 etc which is clearly not in temporal order by any normal
reading of the term. The more I read this, the more I think the current implementation
is not compliant with the specification at all.

I'm not seeing a spec mention of 'reseting' the ordering on clearing records
(which might have been a good thing in the first place but too late now).

> 
> > 
> > Read 1: Records 0, 1, 2, 3 More set.  'next' pointer points to record 4.
> >   Clear 0, 1, 2
> > Read 2: Records 4, 5, 6 More not set. 'next' pointer points to record 7.
> >   Clear 4, 5, 6
> > 
> > Skipping record 3.
> > 
> > So I think we have to absorb the full mailbox payload each time to guarantee
> > we don't skip events or process them out of order (which is what would happen
> > if we relied on a retry loop - we aren't allowed to clear them out of
> > order anyway 8.2.9.2.3 "Events shall be cleared in temporal order. The device
> > shall verify the event record handles specified in the input payload are in
> > temporal order. ... "). 
> > Obviously that temporal order thing is only relevant if we get my second
> > example occurring on real hardware.  I think the spec is vague enough
> > to allow that implementation.  Would have been easy to specify this originally
> > but it probably won't go in as errata so we need to cope with all the
> > flexibility that is present.  
> 
> :-(  Yea coulda, woulda, shoulda...  ;-)
> 
> > 
> > What fun and oh for a parameter to control how many records are returned!  
> 
> Yea.  But I really don't think there is a problem unless someone really take
> liberty with the spec.  I think it boils down to how one interprets _when_ a
> record is removed from the log.

This is nothing to do with removal. The wording we have is just about reading
and I think a strict reading of the spec would say your assumption of a reset of the
read pointer on clear is NOT a valid implementation.  There is separate wording
about clears being in temporal order, but that doesn't effect the Get Event
Records handling.

> 
> If the record is removed when it is returned (as in your 'next' pointer
> example) then why have a clear at all?

Because if your software crashes, you don't have a handshake to reestablish
state.  If that happens you read the whole log until MORE is not set and
then read it again to get a clean list.  It's messy situation that has
been discussed before for GET POISON LIST which has the same nasty handing
of MORE.  (look in appropriate forum for resolution to that one that we can't
yet discuss here!)

Also, allows for non destructive readback (debugging tools might take a look
having paused the normal handling).

> If my interpretation is correct then
> the next available entry is the one which has not been cleared.

If that is the case the language in "More Event Records" doesn't work
"The host should continue to retrieve records using this command, until
this indicator is no longer set by the device"

With your reading of the spec, if we clear nothing, we'd keep getting the
first set of records and only be able to read more by clearing them...


>  Therefore in
> your example 'next' is not incremented until clear has been called.  I think
> that implementation is also supported by the idea that records must be cleared
> in temporal order.  Otherwise I think devices would get confused.

Not hard for device to do this (how I now read the spec) properly.

Two pointers:
1) Next to clear: CLEAR
2) Next to read:  READ

Advance the the READ pointer on Get Event Records
For CLEAR, check that the requested clears are handled in order and that
they are before the READ pointer.

Maybe we should just take it to appropriate spec forum to seek a clarification?

Jonathan

> 
> FWIW the qemu implementation is based on my interpretation ATM.
> 
> Ira
> 
> > 
> > Jonathan
> > 
> >   
> > >   
> > > >     
> > > > > +		 payload.flags & CXL_GET_EVENT_FLAG_MORE_RECORDS);
> > > > > +}    
> > > >   
> >   
> > >   
> >   

Powered by blists - more mailing lists