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

[Jie Deng <jie.deng@intel.com>]

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