On 2020/12/31 20:42, Stefan Hajnoczi wrote:
This approach causes a bottleneck when there are multiple I2C devices.
It requires exclusive access to the virtqueue and virtual I2C bus,
slowing down applications that are accessing separate I2C devices.
This is inefficient, especially if the I2C devices are emulated or on
separate physical I2C busses where request concurrent processing is
possible.
This also creates starvation/denial-of-service problems in the driver
because an application can submit many requests and needs exclusive
access to the virtqueue until all requests have completed.
I suggest defining a VIRTIO_I2C_FLAGS_CONTINUATION bit in struct
virtio_i2c_out_hdr::flags. When a request fails and this flag is set
then the device fails all later requests until it reaches a request
where the flag is zero. This allows the driver to enqueue a sequence of
requests that fail as a group without waiting for an empty virtqueue.
The flags is designed to be a "driver->device" field for extensibility. It
is read-only� to the device.
So when a request fails, the device can't set this read-only field.
Sorry, "and this flag is set" means "and this flag was set by the
driver". My intention was that the driver marks a request so the device
knows that it needs to fail the next request too when this request
fails.
No. A device doesn't need to be told by a driver if it needs to fail the
next request.
Actually just the opposite, a driver may need to be notified. A driver
can't know which
one is the "next" because only a device know which request is the first
failed one.
Once a device fails to handle a request, it shall fail the existing
remaining requests
since these requests needs to be handled in order.
Thinking about it again, VIRTIO_I2C_FLAGS_FAIL_NEXT would be a clearer
name for this flag.
We just need to make sure that once the driver adds some requests to the
virtqueue,
it should complete them (either success or fail) before adding new requests.
I think this
is a behavior of physical I2C adapter drivers and we can follow.
Existing VIRTIO devices do not have special empty queue semantics and I
don't think virtio-i2c should introduce a special case here. The
VIRTIO_I2C_FLAGS_FAIL_NEXT flag is simple to implement, allows drivers
more freedom to enqueue requests, and eliminates the race condition with
polled device implementations (they could complete failed request #1
before request #2 is enqueued and then request #2 would not be
automatically failed).
The I2C core is using a synchronization semantics. It means once a I2C
bus driver send
some messages, it should wait for the device to complete them (either
success or fail)
before sending new messages. The virtio-i2c driver also have to follow
this to register it into
I2C subsystem. So the freedom is already limited by the I2C core, and as
a result, we don't have
such race conditions.
Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
