RE: [PATCH V7 mlx5-next 15/15] vfio: Extend the device migration protocol with PRE_COPY

From: "Tian, Kevin" <kevin.tian@intel.com>
To: Jason Gunthorpe <jgg@nvidia.com>
Cc: Yishai Hadas <yishaih@nvidia.com>,
	"alex.williamson@redhat.com" <alex.williamson@redhat.com>,
	"bhelgaas@google.com" <bhelgaas@google.com>,
	"saeedm@nvidia.com" <saeedm@nvidia.com>,
	"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"kuba@kernel.org" <kuba@kernel.org>,
	"leonro@nvidia.com" <leonro@nvidia.com>,
	"kwankhede@nvidia.com" <kwankhede@nvidia.com>,
	"mgurtovoy@nvidia.com" <mgurtovoy@nvidia.com>,
	"maorg@nvidia.com" <maorg@nvidia.com>,
	"Raj, Ashok" <ashok.raj@intel.com>,
	"shameerali.kolothum.thodi@huawei.com" 
	<shameerali.kolothum.thodi@huawei.com>
Subject: RE: [PATCH V7 mlx5-next 15/15] vfio: Extend the device migration protocol with PRE_COPY
Date: Wed, 23 Feb 2022 01:46:06 +0000	[thread overview]
Message-ID: <BN9PR11MB5276B8C8AA760FC52B33CD328C3C9@BN9PR11MB5276.namprd11.prod.outlook.com> (raw)
In-Reply-To: <20220223004432.GH10061@nvidia.com>

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Wednesday, February 23, 2022 8:45 AM
> 
> On Wed, Feb 23, 2022 at 12:40:58AM +0000, Tian, Kevin wrote:
> > > From: Jason Gunthorpe <jgg@nvidia.com>
> > > Sent: Tuesday, February 22, 2022 11:51 PM
> > >
> > > On Tue, Feb 22, 2022 at 01:43:13AM +0000, Tian, Kevin wrote:
> > >
> > > > > > > + * Drivers should attempt to return estimates so that initial_bytes
> +
> > > > > > > + * dirty_bytes matches the amount of data an immediate
> transition
> > > to
> > > > > > > STOP_COPY
> > > > > > > + * will require to be streamed.
> > > > > >
> > > > > > I didn't understand this requirement. In an immediate transition to
> > > > > > STOP_COPY I expect the amount of data covers the entire device
> > > > > > state, i.e. initial_bytes. dirty_bytes are dynamic and iteratively
> returned
> > > > > > then why we need set some expectation on the sum of
> > > > > > initial+round1_dity+round2_dirty+...
> > > > >
> > > > > "will require to be streamed" means additional data from this point
> > > > > forward, not including anything already sent.
> > > > >
> > > > > It turns into the estimate of how long STOP_COPY will take.
> > > >
> > > > I still didn't get the 'match' part. Why should the amount of data which
> > > > has already been sent match the additional data to be sent in
> STOP_COPY?
> > >
> > > None of it is 'already been sent' the return values are always 'still
> > > to be sent'
> > >
> >
> > Reread the description:
> >
> > + * Drivers should attempt to return estimates so that initial_bytes +
> > + * dirty_bytes matches the amount of data an immediate transition to
> STOP_COPY
> > + * will require to be streamed.
> >
> > I guess you intended to mean that when EITHER initial_bytes OR
> > dirty_bytes is read the returned value should match the amount
> > of data as described above. It is "+" which confused me to think
> > it as a sum of both numbers...
> 
> It is the sum
> 
> initial_bytes declines as the data is transferred. Once everything is
> read out the sum will be 0.
> 

That is the point which I overlooked (with the impression that initial_bytes
is static). As explained in the code comment 'initial' here means the
initial phase of precopy instead of a static number for the entire 
device state. During the initial precopy phase dirty_bytes should not
count any state which hasn't been transmitted then the sum of both
numbers can reflect the accurate size of remaining bytes to be
transmitted. Once initial phase is completed initial_bytes is always 
ZERO then dirty_bytes alone represents the remaining bytes. 😊

Thanks
Kevin