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

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

On 2021/1/5 2:02, Stefan Hajnoczi wrote:
> 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.

So do you mean to mark the first one of each grouped requests ?

For example, if a driver kicks twice and the virtqueue has following 
requests.

R11 R12 R21 R22 R23

(First kick: R11 R12, Second kick: R21 R22 R23)

Do you want the driver to set the flags of R11 and R21 to be 
VIRTIO_I2C_FLAGS_FAIL_NEXT ?

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