Re: [virtio-comment] [PATCH v6] virtio-i2c: add the device specification

[Stefan Hajnoczi <stefanha@redhat.com>]

On Mon, Jan 04, 2021 at 10:31:48AM +0800, Jie Deng wrote:
> 
> 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.

I think we are miscommunicating. The driver is telling the device which
requests are grouped together - that is the purpose of the
VIRTIO_I2C_FLAGS_FAIL_NEXT flag. When a request fails the device is able
to fail the remaining requests in that group. Requests that are not part
of the group won't be failed.

> 
> 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.

VIRTIO devices are not specific to Linux so the design should avoid the
limitations of the current Linux driver behavior, if possible. It's easy
to include the VIRTIO_I2C_FLAGS_FAIL_NEXT bit that I described, both in
the spec and in the Linux driver, and it will allow flexibility in the
future.

Stefan

Attachment: signature.asc
Description: PGP signature