Re: [virtio-comment] Re: [PATCH v3 6/8] admin: Add theory of operation for write recording commands

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

On Fri, Nov 17, 2023 at 05:59:35PM +0800, Zhu, Lingshan wrote:
> 
> 
> On 11/16/2023 7:59 PM, Michael S. Tsirkin wrote:
> > On Thu, Nov 16, 2023 at 06:28:07PM +0800, Zhu, Lingshan wrote:
> > > 
> > > On 11/16/2023 1:51 PM, Michael S. Tsirkin wrote:
> > > > On Thu, Nov 16, 2023 at 05:29:54AM +0000, Parav Pandit wrote:
> > > > > We should expose a limit of the device in the proposed WRITE_RECORD_CAP_QUERY command, that how much range it can track.
> > > > > So that future provisioning framework can use it.
> > > > > 
> > > > > I will cover this in v5 early next week.
> > > > I do worry about how this can even work though. If you want a generic
> > > > device you do not get to dictate how much memory VM has.
> > > > 
> > > > Aren't we talking bit per page? With 1TByte of memory to track ->
> > > > 256Gbit -> 32Gbit -> 8Gbyte per VF?
> > > > 
> > > > And you happily say "we'll address this in the future" while at the same
> > > > time fighting tooth and nail against adding single bit status registers
> > > > because scalability?
> > > > 
> > > > 
> > > > I have a feeling doing this completely theoretical like this is problematic.
> > > > Maybe you have it all laid out neatly in your head but I suspect
> > > > not all of TC can picture it clearly enough based just on spec text.
> > > > 
> > > > We do sometimes ask for POC implementation in linux / qemu to
> > > > demonstrate how things work before merging code. We skipped this
> > > > for admin things so far but I think it's a good idea to start doing
> > > > it here.
> > > > 
> > > > What makes me pause a bit before saying please do a PoC is
> > > > all the opposition that seems to exist to even using admin
> > > > commands in the 1st place. I think once we finally stop
> > > > arguing about whether to use admin commands at all then
> > > > a PoC will be needed before merging.
> > > We have POR productions that implemented the approach in my series. They are
> > > multiple generations
> > > of productions in market and running in customers data centers for years.
> > > 
> > > Back to 2019 when we start working on vDPA, we have sent some samples of
> > > production(e.g., Cascade Glacier)
> > > and the datasheet, you can find live migration facilities there, includes
> > > suspend, vq state and other
> > > features.
> > > 
> > > And there is an reference in DPDK live migration, I have provided this page
> > > before:
> > > https://doc.dpdk.org/guides-21.11/vdpadevs/ifc.html, it has been working for
> > > long long time.
> > > 
> > > So if we let the facts speak, if we want to see if the proposal is proven to
> > > work, I would
> > > say: They are POR for years, customers already deployed them for years.
> > And I guess what you are trying to say is that this patchset
> > we are reviewing here should be help to the same standard and
> > there should be a PoC? Sounds reasonable.
> Yes and the in-marketing productions are POR, the series just improves the
> design,
> for example, our series also use registers to track vq state, but
> improvements
> than CG or BSC. So I think they are proven to work.

Well yes and no. It works for vdpa because it's a very specific device
with very specific behaviour. If it needs to work for virtio generally,
then 16 bits of state won't be enough so registers won't work.


> > 
> > > For dirty page tracking, I see you want both platform IOMMU tracking and
> > > shadow vqs, I am
> > > totally fine with this idea. And I think maybe we should merge the basic
> > > features first, and
> > > dirty page tracking should be the second step.
> > > 
> > > Thanks
> > Parav wants to add an option of on-device tracking. Which also seems
> > fine. I think it should be optional though because shadow and IOMMU
> > options exist.
> I agree, the vendor can choose to implement their own facility as a backup.
> > 

No that is a bad idea if vendor is doing full virtio, things need to be in spec.

-- 
MST