On Tue, Jan 21, 2025 at 02:36:11PM -0400, Jason Gunthorpe wrote:

On Mon, Jan 20, 2025 at 12:52:09PM -0800, Nicolin Chen wrote:

quoted

The counter of the number of events in the vEVENTQ could decrease
when userspace reads the queue. But you were saying "the number of
events that were sent into the queue", which is like a PROD index
that would keep growing but reset to 0 after UINT_MAX?

yes

Ack. Then I think we should name it "index", beside a "counter"
indicating the number of events in the queue. Or perhaps a pair
of consumer and producer indexes that wrap at end of a limit.

quoted

quoted

IOMMU_VEVENTQ_STATE_OVERFLOW with a 0 length event is seen if events
have been lost and no subsequent events are present. It exists to
ensure timely delivery of the overflow event to userspace. counter
will be the sequence number of the next successful event.

So userspace should first read the header to decide whether or not
to read a vEVENT. If header is overflows, it should skip the vEVENT
struct and read the next header?

Yes, but there won't be a next header. overflow would always be the
last thing in a read() response. If there is another event then
overflow is indicated by non-monotonic count.

I am not 100% sure why "overflow would always be the last thing
in a read() response". I thought that kernel should immediately
report an overflow to user space when the vEVENTQ is overflowed.

Yet, thinking about this once again: user space actually has its
own queue. There's probably no point in letting it know about an
overflow immediately when the kernel vEVENTQ overflows until its
own user queue overflows after it reads the entire vEVENTQ so it
can trigger a vHW event/irq to the VM?

quoted

quoted

If events are lost in the middle of the queue then flags will remain 0
but counter will become non-montonic. A counter delta > 1 indicates
that many events have been lost.

I don't quite get the "no subsequent events" v.s. "in the middle of
the queue"..

I mean to supress specific overflow events to userspace if the counter already
fully indicates overflow.

The purpose of the overflow event is specifically, and only, to
indicate immediately that an overflow occured at the end of the queue,
and no additional events have been pushed since the overflow.

Without this we could loose an event and userspace may not realize
it for a long time.

I see. Because there is no further new event, there would be no
new index to indicate a gap. Thus, we need an overflow node.

quoted

The producer is the driver calling iommufd_viommu_report_event that
only produces a single vEVENT at a time. When the number of vEVENTs
in the vEVENTQ hits the @veventq_depth, it won't insert new vEVENTs
but add an overflow (or exception) node to the head of deliver list
and increase the producer index so the next vEVENT that can find an
empty space in the queue will have an index with a gap (delta >= 1)?

Yes, but each new overflow should move the single preallocated
overflow node back to the end of the list, and the read side should
skip the overflow node if it is not the last entry in the list

If the number of events in the queue is below @veventq_depth as
userspace consumed the events from the queue, I think a new
iommufd_viommu_report_event call should delete the overflow node
from the end of the list, right? Since it can just insert a new
event to the list by marking it a non-monotonic index..

Thanks!
Nicolin