Re: [virtio-comment] [PATCH requirements 1/7] net-features: Add requirements document for release 1.4

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Jun 06, 2023 at 10:28:46PM +0000, Parav Pandit wrote:
> 
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Tuesday, June 6, 2023 6:15 PM
> 
> > > +# 3. Requirements
> > > +## 3.1 Device counters
> > > +1. The driver should be able to query the device and/or per vq counters for
> > > +   debugging purpose using a control vq command.
> > > +2. The driver should be able to query which counters are supported using a
> > > +   control vq command.
> > 
> > why does it matter for requirements whether it's a control vq?
> >
> It matters for requirements, so we produce design that addresses it.
> We don't want to add config space every growing bit map which may be different between different devices.

then say that you want to conserve config space, that is
the requirement. not cvq specifically.

> > 
> > what matters is whether they have to be synchronized with a given queue - I get
> > it they don't have to.
> They don't have to be.

Then say that.

> I don't see how it can be every synchronized.

you could program them through the vq itself.


> > 
> > > +3. If this device is migrated between two hosts, the driver should be able
> > > +   get the counter values in the destination host from where it was left
> > > +   off in the source host.
> > 
> > we don't cover migration currently don't see how this is a spec rquirement.
> > unless maybe it's justification for 4?
> True, but design need to keep this in mind if it has some touch points like of #4 so when migration arrives, it has the building block in place.

so again, let's make sure we capture the actual spec requirement.


> > so maybe it means there needs to be a way to set counters?
> > so there's no need to mention migration - just that it should be possible to
> > move counters between devices.
> > 
> > > +4. If a virtio device is group member device, a group owner should be able
> > > +   to query all the counter attributes using the admin queue command which
> > > +   a virtio device will expose via a control vq to the driver.
> > 
> > 
> > this seems weirdly specific.
> > what is the actual requirement?
> >
> I don't follow the question.
> When a device migrates from src to dst, group owner needs to know if both side underlying member device has same counter attributes or not.

whether it's through a command or not is not a requirement
and I still do not know what the requirement is.
what does "same counter attributes" mean? you never mentioned
attributes before.


> > 
> > > +
> > > +### 3.1.1 Per receive queue counters
> > > +1. le64 rx_oversize_pkt_errors: Packet dropped due to receive packet being
> > > +    oversize than the buffer size
> > 
> > with mergeable buffers how does this differ from 2?
> > 
> > > +2. le64 rx_no_buffer_pkt_errors: Packet dropped due to unavailability of the
> > > +    buffer in the receive queue
> > > +3. le64 rx_gro_pkts: Packets treated as guest GSO sequence by the
> > > +device
> > 
> > what does this mean exactly? packets before or after they are combined?
> >
> Before.
>  
> > pls stick to device/driver terminology not guest/host
> > 
> Yes, will change the GUEST surfaced from the current F_GUEST terminology of the net device.

So?  this predates 1.x spec we never bothered changing them. 

> > > +4. le64 rx_pkts: Total packets received by the device
> > 
> > including dropped ones or not?
> >
> Not including. Will add this clarification further in v1.
>  
> > > +
> > > +### 3.1.2 Per transmit queue counters 1. le64 tx_bad_desc_errors:
> > > +Descriptors dropped by the device due to errors in
> > > +   descriptors
> > 
> > since when do we drop packets on error in descriptor?
> > we just as likely stall ...
> > 
> It is left to the device to implement on what to do on bad desc.

then how can you count drops? why does this even matter?
and why on tx specifically?
I feel addressing descriptor errors is a completely separate project.


> > > +2. le64 tx_gso_pkts: Packets send as host GSO sequence
> > 
> > same questions as gro
> > 
> > > +3. le64 tx_pkts: Total packets send by the device
> > 
> > sent
> >
> Ack.
> > >