RE: [PATCH v5 5/7] migration/multifd: implement initialization of qpl compression
> -Original Message-
> From: Peter Xu
> Sent: Thursday, March 28, 2024 11:16 PM
> To: Liu, Yuan1
> Cc: Daniel P. Berrangé ; faro...@suse.de; qemu-
> de...@nongnu.org; hao.xi...@bytedance.com; bryan.zh...@bytedance.com; Zou,
> Nanhai
> Subject: Re: [PATCH v5 5/7] migration/multifd: implement initialization of
> qpl compression
>
> On Thu, Mar 28, 2024 at 02:32:37AM +, Liu, Yuan1 wrote:
> > > -Original Message-
> > > From: Peter Xu
> > > Sent: Thursday, March 28, 2024 3:26 AM
> > > To: Liu, Yuan1
> > > Cc: Daniel P. Berrangé ; faro...@suse.de; qemu-
> > > de...@nongnu.org; hao.xi...@bytedance.com; bryan.zh...@bytedance.com;
> Zou,
> > > Nanhai
> > > Subject: Re: [PATCH v5 5/7] migration/multifd: implement
> initialization of
> > > qpl compression
> > >
> > > On Fri, Mar 22, 2024 at 12:40:32PM -0400, Peter Xu wrote:
> > > > > > void multifd_recv_zero_page_process(MultiFDRecvParams *p)
> > > > > > {
> > > > > > for (int i = 0; i < p->zero_num; i++) {
> > > > > > void *page = p->host + p->zero[i];
> > > > > > if (!buffer_is_zero(page, p->page_size)) {
> > > > > > memset(page, 0, p->page_size);
> > > > > > }
> > > > > > }
> > > > > > }
> > > >
> > > > It may not matter much (where I also see your below comments), but
> just
> > > to
> > > > mention another solution to avoid this read is that we can maintain
> > > > RAMBlock->receivedmap for precopy (especially, multifd, afaiu
> multifd
> > > > doesn't yet update this bitmap.. even if normal precopy does), then
> here
> > > > instead of scanning every time, maybe we can do:
> > > >
> > > > /*
> > > >* If it's the 1st time receiving it, no need to clear it as it
> must
> > > be
> > > >* all zeros now.
> > > >*/
> > > > if (bitmap_test(rb->receivedmap, page_offset)) {
> > > > memset(page, 0, ...);
> > > > } else {
> > > > bitmap_set(rb->receivedmap, page_offset);
> > > > }
> > > >
> > > > And we also always set the bit when !zero too.
> > > >
> > > > My rational is that it's unlikely a zero page if it's sent once or
> more,
> > > > while OTOH for the 1st time we receive it, it must be a zero page,
> so no
> > > > need to scan for the 1st round.
> > >
> > > Thinking about this, I'm wondering whether we should have this
> regardless.
> > > IIUC now multifd will always require two page faults on destination
> for
> > > anonymous guest memories (I suppose shmem/hugetlb is fine as no zero
> page
> > > in those worlds). Even though it should be faster than DMA faults, it
> > > still is unwanted.
> > >
> > > I'll take a note myself as todo to do some measurements in the future
> > > first. However if anyone thinks that makes sense and want to have a
> look,
> > > please say so. It'll be more than welcomed.
> >
> > Yes, I think this is a better improvement to avoid two page faults. I
> can test
> > the performance impact of this change on SVM-capable devices and give
> some data
> > later. As we saw before, the IOTLB flush occurs via COW, with the
> change, the
> > impact of the COW should be gone.
> >
> > If you need more testing and analysis on this, please let me know
>
> Nothing more than that. Just a heads up that Xiang used to mention a test
> case where Richard used to suggest dropping the zero check:
>
> https://lore.kernel.org/r/CAAYibXib+TWnJpV22E=adncdBmwXJRqgRjJXK7X71J=bDfa
> x...@mail.gmail.com
>
> AFAIU this should be resolved if we have the bitmap maintained, but we can
> double check. IIUC that's exactly the case for an idle guest, in that
> case
> it should be even faster to skip the memcmp when bit clear.
>
> If you're going to post the patches, feel free to post that as a
> standalone
> small series first, then that can be considered merge even earlier.
>
> Thanks a lot for doing this.
Sure, I will prepare a separate patch for this, and we can have a better
discussion
on concrete implementation and test results.
Re: [PATCH v5 5/7] migration/multifd: implement initialization of qpl compression
On Thu, Mar 28, 2024 at 02:32:37AM +, Liu, Yuan1 wrote:
> > -Original Message-
> > From: Peter Xu
> > Sent: Thursday, March 28, 2024 3:26 AM
> > To: Liu, Yuan1
> > Cc: Daniel P. Berrangé ; faro...@suse.de; qemu-
> > de...@nongnu.org; hao.xi...@bytedance.com; bryan.zh...@bytedance.com; Zou,
> > Nanhai
> > Subject: Re: [PATCH v5 5/7] migration/multifd: implement initialization of
> > qpl compression
> >
> > On Fri, Mar 22, 2024 at 12:40:32PM -0400, Peter Xu wrote:
> > > > > void multifd_recv_zero_page_process(MultiFDRecvParams *p)
> > > > > {
> > > > > for (int i = 0; i < p->zero_num; i++) {
> > > > > void *page = p->host + p->zero[i];
> > > > > if (!buffer_is_zero(page, p->page_size)) {
> > > > > memset(page, 0, p->page_size);
> > > > > }
> > > > > }
> > > > > }
> > >
> > > It may not matter much (where I also see your below comments), but just
> > to
> > > mention another solution to avoid this read is that we can maintain
> > > RAMBlock->receivedmap for precopy (especially, multifd, afaiu multifd
> > > doesn't yet update this bitmap.. even if normal precopy does), then here
> > > instead of scanning every time, maybe we can do:
> > >
> > > /*
> > >* If it's the 1st time receiving it, no need to clear it as it must
> > be
> > >* all zeros now.
> > >*/
> > > if (bitmap_test(rb->receivedmap, page_offset)) {
> > > memset(page, 0, ...);
> > > } else {
> > > bitmap_set(rb->receivedmap, page_offset);
> > > }
> > >
> > > And we also always set the bit when !zero too.
> > >
> > > My rational is that it's unlikely a zero page if it's sent once or more,
> > > while OTOH for the 1st time we receive it, it must be a zero page, so no
> > > need to scan for the 1st round.
> >
> > Thinking about this, I'm wondering whether we should have this regardless.
> > IIUC now multifd will always require two page faults on destination for
> > anonymous guest memories (I suppose shmem/hugetlb is fine as no zero page
> > in those worlds). Even though it should be faster than DMA faults, it
> > still is unwanted.
> >
> > I'll take a note myself as todo to do some measurements in the future
> > first. However if anyone thinks that makes sense and want to have a look,
> > please say so. It'll be more than welcomed.
>
> Yes, I think this is a better improvement to avoid two page faults. I can test
> the performance impact of this change on SVM-capable devices and give some
> data
> later. As we saw before, the IOTLB flush occurs via COW, with the change, the
> impact of the COW should be gone.
>
> If you need more testing and analysis on this, please let me know
Nothing more than that. Just a heads up that Xiang used to mention a test
case where Richard used to suggest dropping the zero check:
https://lore.kernel.org/r/CAAYibXib+TWnJpV22E=adncdBmwXJRqgRjJXK7X71J=bdfa...@mail.gmail.com
AFAIU this should be resolved if we have the bitmap maintained, but we can
double check. IIUC that's exactly the case for an idle guest, in that case
it should be even faster to skip the memcmp when bit clear.
If you're going to post the patches, feel free to post that as a standalone
small series first, then that can be considered merge even earlier.
Thanks a lot for doing this.
--
Peter Xu
RE: [PATCH v5 5/7] migration/multifd: implement initialization of qpl compression
> -Original Message-
> From: Peter Xu
> Sent: Thursday, March 28, 2024 3:26 AM
> To: Liu, Yuan1
> Cc: Daniel P. Berrangé ; faro...@suse.de; qemu-
> de...@nongnu.org; hao.xi...@bytedance.com; bryan.zh...@bytedance.com; Zou,
> Nanhai
> Subject: Re: [PATCH v5 5/7] migration/multifd: implement initialization of
> qpl compression
>
> On Fri, Mar 22, 2024 at 12:40:32PM -0400, Peter Xu wrote:
> > > > void multifd_recv_zero_page_process(MultiFDRecvParams *p)
> > > > {
> > > > for (int i = 0; i < p->zero_num; i++) {
> > > > void *page = p->host + p->zero[i];
> > > > if (!buffer_is_zero(page, p->page_size)) {
> > > > memset(page, 0, p->page_size);
> > > > }
> > > > }
> > > > }
> >
> > It may not matter much (where I also see your below comments), but just
> to
> > mention another solution to avoid this read is that we can maintain
> > RAMBlock->receivedmap for precopy (especially, multifd, afaiu multifd
> > doesn't yet update this bitmap.. even if normal precopy does), then here
> > instead of scanning every time, maybe we can do:
> >
> > /*
> >* If it's the 1st time receiving it, no need to clear it as it must
> be
> >* all zeros now.
> >*/
> > if (bitmap_test(rb->receivedmap, page_offset)) {
> > memset(page, 0, ...);
> > } else {
> > bitmap_set(rb->receivedmap, page_offset);
> > }
> >
> > And we also always set the bit when !zero too.
> >
> > My rational is that it's unlikely a zero page if it's sent once or more,
> > while OTOH for the 1st time we receive it, it must be a zero page, so no
> > need to scan for the 1st round.
>
> Thinking about this, I'm wondering whether we should have this regardless.
> IIUC now multifd will always require two page faults on destination for
> anonymous guest memories (I suppose shmem/hugetlb is fine as no zero page
> in those worlds). Even though it should be faster than DMA faults, it
> still is unwanted.
>
> I'll take a note myself as todo to do some measurements in the future
> first. However if anyone thinks that makes sense and want to have a look,
> please say so. It'll be more than welcomed.
Yes, I think this is a better improvement to avoid two page faults. I can test
the performance impact of this change on SVM-capable devices and give some data
later. As we saw before, the IOTLB flush occurs via COW, with the change, the
impact of the COW should be gone.
If you need more testing and analysis on this, please let me know
Re: [PATCH v5 5/7] migration/multifd: implement initialization of qpl compression
On Fri, Mar 22, 2024 at 12:40:32PM -0400, Peter Xu wrote:
> > > void multifd_recv_zero_page_process(MultiFDRecvParams *p)
> > > {
> > > for (int i = 0; i < p->zero_num; i++) {
> > > void *page = p->host + p->zero[i];
> > > if (!buffer_is_zero(page, p->page_size)) {
> > > memset(page, 0, p->page_size);
> > > }
> > > }
> > > }
>
> It may not matter much (where I also see your below comments), but just to
> mention another solution to avoid this read is that we can maintain
> RAMBlock->receivedmap for precopy (especially, multifd, afaiu multifd
> doesn't yet update this bitmap.. even if normal precopy does), then here
> instead of scanning every time, maybe we can do:
>
> /*
>* If it's the 1st time receiving it, no need to clear it as it must be
>* all zeros now.
>*/
> if (bitmap_test(rb->receivedmap, page_offset)) {
> memset(page, 0, ...);
> } else {
> bitmap_set(rb->receivedmap, page_offset);
> }
>
> And we also always set the bit when !zero too.
>
> My rational is that it's unlikely a zero page if it's sent once or more,
> while OTOH for the 1st time we receive it, it must be a zero page, so no
> need to scan for the 1st round.
Thinking about this, I'm wondering whether we should have this regardless.
IIUC now multifd will always require two page faults on destination for
anonymous guest memories (I suppose shmem/hugetlb is fine as no zero page
in those worlds). Even though it should be faster than DMA faults, it
still is unwanted.
I'll take a note myself as todo to do some measurements in the future
first. However if anyone thinks that makes sense and want to have a look,
please say so. It'll be more than welcomed.
Thanks,
--
Peter Xu
Re: [PATCH v5 5/7] migration/multifd: implement initialization of qpl compression
On Fri, Mar 22, 2024 at 02:47:02PM +, Liu, Yuan1 wrote: > > -Original Message- > > From: Liu, Yuan1 > > Sent: Friday, March 22, 2024 10:07 AM > > To: Peter Xu > > Cc: Daniel P. Berrangé ; faro...@suse.de; qemu- > > de...@nongnu.org; hao.xi...@bytedance.com; bryan.zh...@bytedance.com; Zou, > > Nanhai > > Subject: RE: [PATCH v5 5/7] migration/multifd: implement initialization of > > qpl compression > > > > > -Original Message- > > > From: Peter Xu > > > Sent: Thursday, March 21, 2024 11:28 PM > > > To: Liu, Yuan1 > > > Cc: Daniel P. Berrangé ; faro...@suse.de; qemu- > > > de...@nongnu.org; hao.xi...@bytedance.com; bryan.zh...@bytedance.com; > > Zou, > > > Nanhai > > > Subject: Re: [PATCH v5 5/7] migration/multifd: implement initialization > > of > > > qpl compression > > > > > > On Thu, Mar 21, 2024 at 01:37:36AM +, Liu, Yuan1 wrote: > > > > > -Original Message- > > > > > From: Peter Xu > > > > > Sent: Thursday, March 21, 2024 4:32 AM > > > > > To: Liu, Yuan1 > > > > > Cc: Daniel P. Berrangé ; faro...@suse.de; qemu- > > > > > de...@nongnu.org; hao.xi...@bytedance.com; > > bryan.zh...@bytedance.com; > > > Zou, > > > > > Nanhai > > > > > Subject: Re: [PATCH v5 5/7] migration/multifd: implement > > > initialization of > > > > > qpl compression > > > > > > > > > > On Wed, Mar 20, 2024 at 04:23:01PM +, Liu, Yuan1 wrote: > > > > > > let me explain here, during the decompression operation of IAA, > > the > > > > > > decompressed data can be directly output to the virtual address of > > > the > > > > > > guest memory by IAA hardware. It can avoid copying the > > decompressed > > > > > data > > > > > > to guest memory by CPU. > > > > > > > > > > I see. > > > > > > > > > > > Without -mem-prealloc, all the guest memory is not populated, and > > > IAA > > > > > > hardware needs to trigger I/O page fault first and then output the > > > > > > decompressed data to the guest memory region. Besides that, CPU > > > page > > > > > > faults will also trigger IOTLB flush operation when IAA devices > > use > > > SVM. > > > > > > > > > > Oh so the IAA hardware already can use CPU pgtables? Nice.. > > > > > > > > > > Why IOTLB flush is needed? AFAIU we're only installing new pages, > > the > > > > > request can either come from a CPU access or a DMA. In all cases > > > there > > > > > should have no tearing down of an old page. Isn't an iotlb flush > > only > > > > > needed if a tear down happens? > > > > > > > > As far as I know, IAA hardware uses SVM technology to use the CPU's > > page > > > table > > > > for address translation (IOMMU scalable mode directly accesses the CPU > > > page table). > > > > Therefore, when the CPU page table changes, the device's Invalidation > > > operation needs > > > > to be triggered to update the IOMMU and the device's cache. > > > > > > > > My current kernel version is mainline 6.2. The issue I see is as > > > follows: > > > > --Handle_mm_fault > > > > | > > > > -- wp_page_copy > > > > > > This is the CoW path. Not usual at all.. > > > > > > I assume this issue should only present on destination. Then the guest > > > pages should be the destination of such DMAs to happen, which means > > these > > > should be write faults, and as we see here it is, otherwise it won't > > > trigger a CoW. > > > > > > However it's not clear to me why a pre-installed zero page existed. It > > > means someone read the guest pages first. > > > > > > It might be interesting to know _why_ someone reads the guest pages, > > even > > > if we know they're all zeros. If we can avoid such reads then it'll be > > a > > > hole rather than a prefaulted read on zero page, then invalidations are > > > not > > > needed, and I expect that should fix the iotlb storm issue. > > > > The received pages will be read for zero pages check first. Although > > these pages are zero pages, and IAA hardwar
RE: [PATCH v5 5/7] migration/multifd: implement initialization of qpl compression
> -Original Message- > From: Liu, Yuan1 > Sent: Friday, March 22, 2024 10:07 AM > To: Peter Xu > Cc: Daniel P. Berrangé ; faro...@suse.de; qemu- > de...@nongnu.org; hao.xi...@bytedance.com; bryan.zh...@bytedance.com; Zou, > Nanhai > Subject: RE: [PATCH v5 5/7] migration/multifd: implement initialization of > qpl compression > > > -Original Message- > > From: Peter Xu > > Sent: Thursday, March 21, 2024 11:28 PM > > To: Liu, Yuan1 > > Cc: Daniel P. Berrangé ; faro...@suse.de; qemu- > > de...@nongnu.org; hao.xi...@bytedance.com; bryan.zh...@bytedance.com; > Zou, > > Nanhai > > Subject: Re: [PATCH v5 5/7] migration/multifd: implement initialization > of > > qpl compression > > > > On Thu, Mar 21, 2024 at 01:37:36AM +, Liu, Yuan1 wrote: > > > > -Original Message- > > > > From: Peter Xu > > > > Sent: Thursday, March 21, 2024 4:32 AM > > > > To: Liu, Yuan1 > > > > Cc: Daniel P. Berrangé ; faro...@suse.de; qemu- > > > > de...@nongnu.org; hao.xi...@bytedance.com; > bryan.zh...@bytedance.com; > > Zou, > > > > Nanhai > > > > Subject: Re: [PATCH v5 5/7] migration/multifd: implement > > initialization of > > > > qpl compression > > > > > > > > On Wed, Mar 20, 2024 at 04:23:01PM +, Liu, Yuan1 wrote: > > > > > let me explain here, during the decompression operation of IAA, > the > > > > > decompressed data can be directly output to the virtual address of > > the > > > > > guest memory by IAA hardware. It can avoid copying the > decompressed > > > > data > > > > > to guest memory by CPU. > > > > > > > > I see. > > > > > > > > > Without -mem-prealloc, all the guest memory is not populated, and > > IAA > > > > > hardware needs to trigger I/O page fault first and then output the > > > > > decompressed data to the guest memory region. Besides that, CPU > > page > > > > > faults will also trigger IOTLB flush operation when IAA devices > use > > SVM. > > > > > > > > Oh so the IAA hardware already can use CPU pgtables? Nice.. > > > > > > > > Why IOTLB flush is needed? AFAIU we're only installing new pages, > the > > > > request can either come from a CPU access or a DMA. In all cases > > there > > > > should have no tearing down of an old page. Isn't an iotlb flush > only > > > > needed if a tear down happens? > > > > > > As far as I know, IAA hardware uses SVM technology to use the CPU's > page > > table > > > for address translation (IOMMU scalable mode directly accesses the CPU > > page table). > > > Therefore, when the CPU page table changes, the device's Invalidation > > operation needs > > > to be triggered to update the IOMMU and the device's cache. > > > > > > My current kernel version is mainline 6.2. The issue I see is as > > follows: > > > --Handle_mm_fault > > > | > > > -- wp_page_copy > > > > This is the CoW path. Not usual at all.. > > > > I assume this issue should only present on destination. Then the guest > > pages should be the destination of such DMAs to happen, which means > these > > should be write faults, and as we see here it is, otherwise it won't > > trigger a CoW. > > > > However it's not clear to me why a pre-installed zero page existed. It > > means someone read the guest pages first. > > > > It might be interesting to know _why_ someone reads the guest pages, > even > > if we know they're all zeros. If we can avoid such reads then it'll be > a > > hole rather than a prefaulted read on zero page, then invalidations are > > not > > needed, and I expect that should fix the iotlb storm issue. > > The received pages will be read for zero pages check first. Although > these pages are zero pages, and IAA hardware will not access them, the > COW happens and causes following IOTLB flush operation. As far as I know, > IOMMU quickly detects whether the address range has been used by the > device, > and does not invalidate the address that is not used by the device, this > has > not yet been resolved in Linux kernel 6.2. I will check the latest status > for > this. I checked the Linux mainline 6.8 code, there are no big changes for this. In version 6.8, if the process needs to flush MMU TLB, then I/O TLB flush will be also triggered when the process has SVM devic
RE: [PATCH v5 5/7] migration/multifd: implement initialization of qpl compression
> -Original Message-
> From: Peter Xu
> Sent: Thursday, March 21, 2024 11:28 PM
> To: Liu, Yuan1
> Cc: Daniel P. Berrangé ; faro...@suse.de; qemu-
> de...@nongnu.org; hao.xi...@bytedance.com; bryan.zh...@bytedance.com; Zou,
> Nanhai
> Subject: Re: [PATCH v5 5/7] migration/multifd: implement initialization of
> qpl compression
>
> On Thu, Mar 21, 2024 at 01:37:36AM +, Liu, Yuan1 wrote:
> > > -Original Message-
> > > From: Peter Xu
> > > Sent: Thursday, March 21, 2024 4:32 AM
> > > To: Liu, Yuan1
> > > Cc: Daniel P. Berrangé ; faro...@suse.de; qemu-
> > > de...@nongnu.org; hao.xi...@bytedance.com; bryan.zh...@bytedance.com;
> Zou,
> > > Nanhai
> > > Subject: Re: [PATCH v5 5/7] migration/multifd: implement
> initialization of
> > > qpl compression
> > >
> > > On Wed, Mar 20, 2024 at 04:23:01PM +, Liu, Yuan1 wrote:
> > > > let me explain here, during the decompression operation of IAA, the
> > > > decompressed data can be directly output to the virtual address of
> the
> > > > guest memory by IAA hardware. It can avoid copying the decompressed
> > > data
> > > > to guest memory by CPU.
> > >
> > > I see.
> > >
> > > > Without -mem-prealloc, all the guest memory is not populated, and
> IAA
> > > > hardware needs to trigger I/O page fault first and then output the
> > > > decompressed data to the guest memory region. Besides that, CPU
> page
> > > > faults will also trigger IOTLB flush operation when IAA devices use
> SVM.
> > >
> > > Oh so the IAA hardware already can use CPU pgtables? Nice..
> > >
> > > Why IOTLB flush is needed? AFAIU we're only installing new pages, the
> > > request can either come from a CPU access or a DMA. In all cases
> there
> > > should have no tearing down of an old page. Isn't an iotlb flush only
> > > needed if a tear down happens?
> >
> > As far as I know, IAA hardware uses SVM technology to use the CPU's page
> table
> > for address translation (IOMMU scalable mode directly accesses the CPU
> page table).
> > Therefore, when the CPU page table changes, the device's Invalidation
> operation needs
> > to be triggered to update the IOMMU and the device's cache.
> >
> > My current kernel version is mainline 6.2. The issue I see is as
> follows:
> > --Handle_mm_fault
> > |
> > -- wp_page_copy
>
> This is the CoW path. Not usual at all..
>
> I assume this issue should only present on destination. Then the guest
> pages should be the destination of such DMAs to happen, which means these
> should be write faults, and as we see here it is, otherwise it won't
> trigger a CoW.
>
> However it's not clear to me why a pre-installed zero page existed. It
> means someone read the guest pages first.
>
> It might be interesting to know _why_ someone reads the guest pages, even
> if we know they're all zeros. If we can avoid such reads then it'll be a
> hole rather than a prefaulted read on zero page, then invalidations are
> not
> needed, and I expect that should fix the iotlb storm issue.
The received pages will be read for zero pages check first. Although
these pages are zero pages, and IAA hardware will not access them, the
COW happens and causes following IOTLB flush operation. As far as I know,
IOMMU quickly detects whether the address range has been used by the device,
and does not invalidate the address that is not used by the device, this has
not yet been resolved in Linux kernel 6.2. I will check the latest status for
this.
void multifd_recv_zero_page_process(MultiFDRecvParams *p)
{
for (int i = 0; i < p->zero_num; i++) {
void *page = p->host + p->zero[i];
if (!buffer_is_zero(page, p->page_size)) {
memset(page, 0, p->page_size);
}
}
}
> It'll still be good we can fix this first to not make qpl special from
> this
> regard, so that the hope is migration submodule shouldn't rely on any
> pre-config (-mem-prealloc) on guest memory behaviors to work properly.
Even if the IOTLB problem can be avoided, the I/O page fault problem (normal
pages are loaded by the IAA device and solving normal page faults through IOMMU,
the performance is not good)
It can let the decompressed data of the IAA device be output to a pre-populated
memory instead of directly outputting to the guest address, but then each
multifd
thread needs two memory copies, one copy from the network to the IAA input
memory(pre-populated), and another copy from the IAA output
memory(pre-populated)
to the guest address, which may become a performance bottleneck at the
destination
during the live migration process.
So I think it is still necessary to use the -mem-prealloc option
> > -- mmu_notifier_invalidate_range
> > |
> > -- intel_invalidate_rage
> > |
> > -- qi_flush_piotlb
> > -- qi_flush_dev_iotlb_pasid
>
> --
> Peter Xu
Re: [PATCH v5 5/7] migration/multifd: implement initialization of qpl compression
On Thu, Mar 21, 2024 at 01:37:36AM +, Liu, Yuan1 wrote: > > -Original Message- > > From: Peter Xu > > Sent: Thursday, March 21, 2024 4:32 AM > > To: Liu, Yuan1 > > Cc: Daniel P. Berrangé ; faro...@suse.de; qemu- > > de...@nongnu.org; hao.xi...@bytedance.com; bryan.zh...@bytedance.com; Zou, > > Nanhai > > Subject: Re: [PATCH v5 5/7] migration/multifd: implement initialization of > > qpl compression > > > > On Wed, Mar 20, 2024 at 04:23:01PM +, Liu, Yuan1 wrote: > > > let me explain here, during the decompression operation of IAA, the > > > decompressed data can be directly output to the virtual address of the > > > guest memory by IAA hardware. It can avoid copying the decompressed > > data > > > to guest memory by CPU. > > > > I see. > > > > > Without -mem-prealloc, all the guest memory is not populated, and IAA > > > hardware needs to trigger I/O page fault first and then output the > > > decompressed data to the guest memory region. Besides that, CPU page > > > faults will also trigger IOTLB flush operation when IAA devices use SVM. > > > > Oh so the IAA hardware already can use CPU pgtables? Nice.. > > > > Why IOTLB flush is needed? AFAIU we're only installing new pages, the > > request can either come from a CPU access or a DMA. In all cases there > > should have no tearing down of an old page. Isn't an iotlb flush only > > needed if a tear down happens? > > As far as I know, IAA hardware uses SVM technology to use the CPU's page > table > for address translation (IOMMU scalable mode directly accesses the CPU page > table). > Therefore, when the CPU page table changes, the device's Invalidation > operation needs > to be triggered to update the IOMMU and the device's cache. > > My current kernel version is mainline 6.2. The issue I see is as follows: > --Handle_mm_fault > | > -- wp_page_copy This is the CoW path. Not usual at all.. I assume this issue should only present on destination. Then the guest pages should be the destination of such DMAs to happen, which means these should be write faults, and as we see here it is, otherwise it won't trigger a CoW. However it's not clear to me why a pre-installed zero page existed. It means someone read the guest pages first. It might be interesting to know _why_ someone reads the guest pages, even if we know they're all zeros. If we can avoid such reads then it'll be a hole rather than a prefaulted read on zero page, then invalidations are not needed, and I expect that should fix the iotlb storm issue. It'll still be good we can fix this first to not make qpl special from this regard, so that the hope is migration submodule shouldn't rely on any pre-config (-mem-prealloc) on guest memory behaviors to work properly. > | > -- mmu_notifier_invalidate_range > | > -- intel_invalidate_rage > | > -- qi_flush_piotlb > -- qi_flush_dev_iotlb_pasid -- Peter Xu
RE: [PATCH v5 5/7] migration/multifd: implement initialization of qpl compression
> -Original Message- > From: Peter Xu > Sent: Thursday, March 21, 2024 4:32 AM > To: Liu, Yuan1 > Cc: Daniel P. Berrangé ; faro...@suse.de; qemu- > de...@nongnu.org; hao.xi...@bytedance.com; bryan.zh...@bytedance.com; Zou, > Nanhai > Subject: Re: [PATCH v5 5/7] migration/multifd: implement initialization of > qpl compression > > On Wed, Mar 20, 2024 at 04:23:01PM +, Liu, Yuan1 wrote: > > let me explain here, during the decompression operation of IAA, the > > decompressed data can be directly output to the virtual address of the > > guest memory by IAA hardware. It can avoid copying the decompressed > data > > to guest memory by CPU. > > I see. > > > Without -mem-prealloc, all the guest memory is not populated, and IAA > > hardware needs to trigger I/O page fault first and then output the > > decompressed data to the guest memory region. Besides that, CPU page > > faults will also trigger IOTLB flush operation when IAA devices use SVM. > > Oh so the IAA hardware already can use CPU pgtables? Nice.. > > Why IOTLB flush is needed? AFAIU we're only installing new pages, the > request can either come from a CPU access or a DMA. In all cases there > should have no tearing down of an old page. Isn't an iotlb flush only > needed if a tear down happens? As far as I know, IAA hardware uses SVM technology to use the CPU's page table for address translation (IOMMU scalable mode directly accesses the CPU page table). Therefore, when the CPU page table changes, the device's Invalidation operation needs to be triggered to update the IOMMU and the device's cache. My current kernel version is mainline 6.2. The issue I see is as follows: --Handle_mm_fault | -- wp_page_copy | -- mmu_notifier_invalidate_range | -- intel_invalidate_rage | -- qi_flush_piotlb -- qi_flush_dev_iotlb_pasid > > Due to the inability to quickly resolve a large number of IO page faults > > and IOTLB flushes, the decompression throughput of the IAA device will > > decrease significantly. > > -- > Peter Xu
Re: [PATCH v5 5/7] migration/multifd: implement initialization of qpl compression
On Wed, Mar 20, 2024 at 04:23:01PM +, Liu, Yuan1 wrote: > let me explain here, during the decompression operation of IAA, the > decompressed data can be directly output to the virtual address of the > guest memory by IAA hardware. It can avoid copying the decompressed data > to guest memory by CPU. I see. > Without -mem-prealloc, all the guest memory is not populated, and IAA > hardware needs to trigger I/O page fault first and then output the > decompressed data to the guest memory region. Besides that, CPU page > faults will also trigger IOTLB flush operation when IAA devices use SVM. Oh so the IAA hardware already can use CPU pgtables? Nice.. Why IOTLB flush is needed? AFAIU we're only installing new pages, the request can either come from a CPU access or a DMA. In all cases there should have no tearing down of an old page. Isn't an iotlb flush only needed if a tear down happens? > > Due to the inability to quickly resolve a large number of IO page faults > and IOTLB flushes, the decompression throughput of the IAA device will > decrease significantly. -- Peter Xu
RE: [PATCH v5 5/7] migration/multifd: implement initialization of qpl compression
> -Original Message-
> From: Peter Xu
> Sent: Wednesday, March 20, 2024 11:35 PM
> To: Liu, Yuan1
> Cc: Daniel P. Berrangé ; faro...@suse.de; qemu-
> de...@nongnu.org; hao.xi...@bytedance.com; bryan.zh...@bytedance.com; Zou,
> Nanhai
> Subject: Re: [PATCH v5 5/7] migration/multifd: implement initialization of
> qpl compression
>
> On Wed, Mar 20, 2024 at 03:02:59PM +, Liu, Yuan1 wrote:
> > > > +static int alloc_zbuf(QplData *qpl, uint8_t chan_id, Error **errp)
> > > > +{
> > > > +int flags = MAP_PRIVATE | MAP_POPULATE | MAP_ANONYMOUS;
> > > > +uint32_t size = qpl->job_num * qpl->data_size;
> > > > +uint8_t *buf;
> > > > +
> > > > +buf = (uint8_t *) mmap(NULL, size, PROT_READ | PROT_WRITE,
> flags, -
> > > 1, 0);
> > > > +if (buf == MAP_FAILED) {
> > > > +error_setg(errp, "multifd: %u: alloc_zbuf failed, job
> num %u,
> > > size %u",
> > > > + chan_id, qpl->job_num, qpl->data_size);
> > > > +return -1;
> > > > +}
> > >
> > > What's the reason for using mmap here, rather than a normal
> > > malloc ?
> >
> > I want to populate the memory accessed by the IAA device in the
> initialization
> > phase, and then avoid initiating I/O page faults through the IAA device
> during
> > migration, a large number of I/O page faults are not good for
> performance.
>
> mmap() doesn't populate pages, unless with MAP_POPULATE. And even with
> that it shouldn't be guaranteed, as the populate phase should ignore all
> errors.
>
>MAP_POPULATE (since Linux 2.5.46)
> Populate (prefault) page tables for a mapping. For a file
> map‐
> ping, this causes read-ahead on the file. This will help to
> re‐
> duce blocking on page faults later. The mmap() call
> doesn't
> fail if the mapping cannot be populated (for example, due
> to
> limitations on the number of mapped huge pages when
> using
> MAP_HUGETLB). Support for MAP_POPULATE in conjunction with
> pri‐
> vate mappings was added in Linux 2.6.23.
>
> OTOH, I think g_malloc0() should guarantee to prefault everything in as
> long as the call returned (even though they can be swapped out later, but
> that applies to all cases anyway).
Thanks, Peter. I will try the g_malloc0 method here
> > This problem also occurs at the destination, therefore, I recommend that
> > customers need to add -mem-prealloc for destination boot parameters.
>
> I'm not sure what issue you hit when testing it, but -mem-prealloc flag
> should only control the guest memory backends not the buffers that QEMU
> internally use, afaiu.
>
> Thanks,
>
> --
> Peter Xu
let me explain here, during the decompression operation of IAA, the
decompressed data
can be directly output to the virtual address of the guest memory by IAA
hardware.
It can avoid copying the decompressed data to guest memory by CPU.
Without -mem-prealloc, all the guest memory is not populated, and IAA hardware
needs to trigger
I/O page fault first and then output the decompressed data to the guest memory
region.
Besides that, CPU page faults will also trigger IOTLB flush operation when IAA
devices use SVM.
Due to the inability to quickly resolve a large number of IO page faults and
IOTLB flushes, the
decompression throughput of the IAA device will decrease significantly.
RE: [PATCH v5 5/7] migration/multifd: implement initialization of qpl compression
> -Original Message-
> From: Daniel P. Berrangé
> Sent: Wednesday, March 20, 2024 11:21 PM
> To: Liu, Yuan1
> Cc: pet...@redhat.com; faro...@suse.de; qemu-devel@nongnu.org;
> hao.xi...@bytedance.com; bryan.zh...@bytedance.com; Zou, Nanhai
>
> Subject: Re: [PATCH v5 5/7] migration/multifd: implement initialization of
> qpl compression
>
> On Wed, Mar 20, 2024 at 03:02:59PM +, Liu, Yuan1 wrote:
> > > -Original Message-
> > > From: Daniel P. Berrangé
> > > Sent: Wednesday, March 20, 2024 6:42 PM
> > > To: Liu, Yuan1
> > > Cc: pet...@redhat.com; faro...@suse.de; qemu-devel@nongnu.org;
> > > hao.xi...@bytedance.com; bryan.zh...@bytedance.com; Zou, Nanhai
> > >
> > > Subject: Re: [PATCH v5 5/7] migration/multifd: implement
> initialization of
> > > qpl compression
> > >
> > > On Wed, Mar 20, 2024 at 12:45:25AM +0800, Yuan Liu wrote:
> > > > the qpl initialization includes memory allocation for compressed
> > > > data and the qpl job initialization.
> > > >
> > > > the qpl initialization will check whether the In-Memory Analytics
> > > > Accelerator(IAA) hardware is available, if the platform does not
> > > > have IAA hardware or the IAA hardware is not available, the QPL
> > > > compression initialization will fail.
> > > >
> > > > Signed-off-by: Yuan Liu
> > > > Reviewed-by: Nanhai Zou
> > > > ---
> > > > migration/multifd-qpl.c | 243
> +++-
> > > > 1 file changed, 242 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/migration/multifd-qpl.c b/migration/multifd-qpl.c
> > > > index 056a68a060..6de65e9da7 100644
> > > > --- a/migration/multifd-qpl.c
> > > > +++ b/migration/multifd-qpl.c
> > > > @@ -9,12 +9,253 @@
> > > > * This work is licensed under the terms of the GNU GPL, version 2
> or
> > > later.
> > > > * See the COPYING file in the top-level directory.
> > > > */
> > > > +
> > > > #include "qemu/osdep.h"
> > > > #include "qemu/module.h"
> > > > +#include "qapi/error.h"
> > > > +#include "migration.h"
> > > > +#include "multifd.h"
> > > > +#include "qpl/qpl.h"
> > > > +
> > > > +typedef struct {
> > > > +qpl_job **job_array;
> > > > +/* the number of allocated jobs */
> > > > +uint32_t job_num;
> > > > +/* the size of data processed by a qpl job */
> > > > +uint32_t data_size;
> > > > +/* compressed data buffer */
> > > > +uint8_t *zbuf;
> > > > +/* the length of compressed data */
> > > > +uint32_t *zbuf_hdr;
> > > > +} QplData;
> > > > +
> > > > +static void free_zbuf(QplData *qpl)
> > > > +{
> > > > +if (qpl->zbuf != NULL) {
> > > > +munmap(qpl->zbuf, qpl->job_num * qpl->data_size);
> > > > +qpl->zbuf = NULL;
> > > > +}
> > > > +if (qpl->zbuf_hdr != NULL) {
> > > > +g_free(qpl->zbuf_hdr);
> > > > +qpl->zbuf_hdr = NULL;
> > > > +}
> > > > +}
> > > > +
> > > > +static int alloc_zbuf(QplData *qpl, uint8_t chan_id, Error **errp)
> > > > +{
> > > > +int flags = MAP_PRIVATE | MAP_POPULATE | MAP_ANONYMOUS;
> > > > +uint32_t size = qpl->job_num * qpl->data_size;
> > > > +uint8_t *buf;
> > > > +
> > > > +buf = (uint8_t *) mmap(NULL, size, PROT_READ | PROT_WRITE,
> flags, -
> > > 1, 0);
> > > > +if (buf == MAP_FAILED) {
> > > > +error_setg(errp, "multifd: %u: alloc_zbuf failed, job
> num %u,
> > > size %u",
> > > > + chan_id, qpl->job_num, qpl->data_size);
> > > > +return -1;
> > > > +}
> > >
> > > What's the reason for using mmap here, rather than a normal
> > > malloc ?
> >
> > I want to populate the memory accessed by the IAA device in the
> initialization
> > phase, and then avoid initiating I/O page faults through the IAA device
> during
> > migration, a large number of I/O page faults are not good for
> performance.
>
> Does this mmap actually make a measurable dif
Re: [PATCH v5 5/7] migration/multifd: implement initialization of qpl compression
On Wed, Mar 20, 2024 at 03:02:59PM +, Liu, Yuan1 wrote:
> > > +static int alloc_zbuf(QplData *qpl, uint8_t chan_id, Error **errp)
> > > +{
> > > +int flags = MAP_PRIVATE | MAP_POPULATE | MAP_ANONYMOUS;
> > > +uint32_t size = qpl->job_num * qpl->data_size;
> > > +uint8_t *buf;
> > > +
> > > +buf = (uint8_t *) mmap(NULL, size, PROT_READ | PROT_WRITE, flags, -
> > 1, 0);
> > > +if (buf == MAP_FAILED) {
> > > +error_setg(errp, "multifd: %u: alloc_zbuf failed, job num %u,
> > size %u",
> > > + chan_id, qpl->job_num, qpl->data_size);
> > > +return -1;
> > > +}
> >
> > What's the reason for using mmap here, rather than a normal
> > malloc ?
>
> I want to populate the memory accessed by the IAA device in the initialization
> phase, and then avoid initiating I/O page faults through the IAA device during
> migration, a large number of I/O page faults are not good for performance.
mmap() doesn't populate pages, unless with MAP_POPULATE. And even with
that it shouldn't be guaranteed, as the populate phase should ignore all
errors.
MAP_POPULATE (since Linux 2.5.46)
Populate (prefault) page tables for a mapping. For a file map‐
ping, this causes read-ahead on the file. This will help to re‐
duce blocking on page faults later. The mmap() call doesn't
fail if the mapping cannot be populated (for example, due to
limitations on the number of mapped huge pages when using
MAP_HUGETLB). Support for MAP_POPULATE in conjunction with pri‐
vate mappings was added in Linux 2.6.23.
OTOH, I think g_malloc0() should guarantee to prefault everything in as
long as the call returned (even though they can be swapped out later, but
that applies to all cases anyway).
>
> This problem also occurs at the destination, therefore, I recommend that
> customers need to add -mem-prealloc for destination boot parameters.
I'm not sure what issue you hit when testing it, but -mem-prealloc flag
should only control the guest memory backends not the buffers that QEMU
internally use, afaiu.
Thanks,
--
Peter Xu
Re: [PATCH v5 5/7] migration/multifd: implement initialization of qpl compression
On Wed, Mar 20, 2024 at 03:02:59PM +, Liu, Yuan1 wrote:
> > -Original Message-
> > From: Daniel P. Berrangé
> > Sent: Wednesday, March 20, 2024 6:42 PM
> > To: Liu, Yuan1
> > Cc: pet...@redhat.com; faro...@suse.de; qemu-devel@nongnu.org;
> > hao.xi...@bytedance.com; bryan.zh...@bytedance.com; Zou, Nanhai
> >
> > Subject: Re: [PATCH v5 5/7] migration/multifd: implement initialization of
> > qpl compression
> >
> > On Wed, Mar 20, 2024 at 12:45:25AM +0800, Yuan Liu wrote:
> > > the qpl initialization includes memory allocation for compressed
> > > data and the qpl job initialization.
> > >
> > > the qpl initialization will check whether the In-Memory Analytics
> > > Accelerator(IAA) hardware is available, if the platform does not
> > > have IAA hardware or the IAA hardware is not available, the QPL
> > > compression initialization will fail.
> > >
> > > Signed-off-by: Yuan Liu
> > > Reviewed-by: Nanhai Zou
> > > ---
> > > migration/multifd-qpl.c | 243 +++-
> > > 1 file changed, 242 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/migration/multifd-qpl.c b/migration/multifd-qpl.c
> > > index 056a68a060..6de65e9da7 100644
> > > --- a/migration/multifd-qpl.c
> > > +++ b/migration/multifd-qpl.c
> > > @@ -9,12 +9,253 @@
> > > * This work is licensed under the terms of the GNU GPL, version 2 or
> > later.
> > > * See the COPYING file in the top-level directory.
> > > */
> > > +
> > > #include "qemu/osdep.h"
> > > #include "qemu/module.h"
> > > +#include "qapi/error.h"
> > > +#include "migration.h"
> > > +#include "multifd.h"
> > > +#include "qpl/qpl.h"
> > > +
> > > +typedef struct {
> > > +qpl_job **job_array;
> > > +/* the number of allocated jobs */
> > > +uint32_t job_num;
> > > +/* the size of data processed by a qpl job */
> > > +uint32_t data_size;
> > > +/* compressed data buffer */
> > > +uint8_t *zbuf;
> > > +/* the length of compressed data */
> > > +uint32_t *zbuf_hdr;
> > > +} QplData;
> > > +
> > > +static void free_zbuf(QplData *qpl)
> > > +{
> > > +if (qpl->zbuf != NULL) {
> > > +munmap(qpl->zbuf, qpl->job_num * qpl->data_size);
> > > +qpl->zbuf = NULL;
> > > +}
> > > +if (qpl->zbuf_hdr != NULL) {
> > > +g_free(qpl->zbuf_hdr);
> > > +qpl->zbuf_hdr = NULL;
> > > +}
> > > +}
> > > +
> > > +static int alloc_zbuf(QplData *qpl, uint8_t chan_id, Error **errp)
> > > +{
> > > +int flags = MAP_PRIVATE | MAP_POPULATE | MAP_ANONYMOUS;
> > > +uint32_t size = qpl->job_num * qpl->data_size;
> > > +uint8_t *buf;
> > > +
> > > +buf = (uint8_t *) mmap(NULL, size, PROT_READ | PROT_WRITE, flags, -
> > 1, 0);
> > > +if (buf == MAP_FAILED) {
> > > +error_setg(errp, "multifd: %u: alloc_zbuf failed, job num %u,
> > size %u",
> > > + chan_id, qpl->job_num, qpl->data_size);
> > > +return -1;
> > > +}
> >
> > What's the reason for using mmap here, rather than a normal
> > malloc ?
>
> I want to populate the memory accessed by the IAA device in the initialization
> phase, and then avoid initiating I/O page faults through the IAA device during
> migration, a large number of I/O page faults are not good for performance.
Does this mmap actually make a measurable difference ?
If I've followed the code paths correctly, I think this
alloc_zbuf method only gets called during initial setup
of each migration thread.
So this use of MAP_POPULATE seems to only make a difference
between faulting in before starting sending data, and faulting
in on first bit of data that's sent. I'm surprised if that's
noticable as a difference.
> This problem also occurs at the destination, therefore, I recommend that
> customers need to add -mem-prealloc for destination boot parameters.
I can understand mem-prelloc making a difference as that guarantees
all of guest RAM is faulted in.
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
RE: [PATCH v5 5/7] migration/multifd: implement initialization of qpl compression
> -Original Message-
> From: Daniel P. Berrangé
> Sent: Wednesday, March 20, 2024 6:42 PM
> To: Liu, Yuan1
> Cc: pet...@redhat.com; faro...@suse.de; qemu-devel@nongnu.org;
> hao.xi...@bytedance.com; bryan.zh...@bytedance.com; Zou, Nanhai
>
> Subject: Re: [PATCH v5 5/7] migration/multifd: implement initialization of
> qpl compression
>
> On Wed, Mar 20, 2024 at 12:45:25AM +0800, Yuan Liu wrote:
> > the qpl initialization includes memory allocation for compressed
> > data and the qpl job initialization.
> >
> > the qpl initialization will check whether the In-Memory Analytics
> > Accelerator(IAA) hardware is available, if the platform does not
> > have IAA hardware or the IAA hardware is not available, the QPL
> > compression initialization will fail.
> >
> > Signed-off-by: Yuan Liu
> > Reviewed-by: Nanhai Zou
> > ---
> > migration/multifd-qpl.c | 243 +++-
> > 1 file changed, 242 insertions(+), 1 deletion(-)
> >
> > diff --git a/migration/multifd-qpl.c b/migration/multifd-qpl.c
> > index 056a68a060..6de65e9da7 100644
> > --- a/migration/multifd-qpl.c
> > +++ b/migration/multifd-qpl.c
> > @@ -9,12 +9,253 @@
> > * This work is licensed under the terms of the GNU GPL, version 2 or
> later.
> > * See the COPYING file in the top-level directory.
> > */
> > +
> > #include "qemu/osdep.h"
> > #include "qemu/module.h"
> > +#include "qapi/error.h"
> > +#include "migration.h"
> > +#include "multifd.h"
> > +#include "qpl/qpl.h"
> > +
> > +typedef struct {
> > +qpl_job **job_array;
> > +/* the number of allocated jobs */
> > +uint32_t job_num;
> > +/* the size of data processed by a qpl job */
> > +uint32_t data_size;
> > +/* compressed data buffer */
> > +uint8_t *zbuf;
> > +/* the length of compressed data */
> > +uint32_t *zbuf_hdr;
> > +} QplData;
> > +
> > +static void free_zbuf(QplData *qpl)
> > +{
> > +if (qpl->zbuf != NULL) {
> > +munmap(qpl->zbuf, qpl->job_num * qpl->data_size);
> > +qpl->zbuf = NULL;
> > +}
> > +if (qpl->zbuf_hdr != NULL) {
> > +g_free(qpl->zbuf_hdr);
> > +qpl->zbuf_hdr = NULL;
> > +}
> > +}
> > +
> > +static int alloc_zbuf(QplData *qpl, uint8_t chan_id, Error **errp)
> > +{
> > +int flags = MAP_PRIVATE | MAP_POPULATE | MAP_ANONYMOUS;
> > +uint32_t size = qpl->job_num * qpl->data_size;
> > +uint8_t *buf;
> > +
> > +buf = (uint8_t *) mmap(NULL, size, PROT_READ | PROT_WRITE, flags, -
> 1, 0);
> > +if (buf == MAP_FAILED) {
> > +error_setg(errp, "multifd: %u: alloc_zbuf failed, job num %u,
> size %u",
> > + chan_id, qpl->job_num, qpl->data_size);
> > +return -1;
> > +}
>
> What's the reason for using mmap here, rather than a normal
> malloc ?
I want to populate the memory accessed by the IAA device in the initialization
phase, and then avoid initiating I/O page faults through the IAA device during
migration, a large number of I/O page faults are not good for performance.
This problem also occurs at the destination, therefore, I recommend that
customers need to add -mem-prealloc for destination boot parameters.
> > +qpl->zbuf = buf;
> > +qpl->zbuf_hdr = g_new0(uint32_t, qpl->job_num);
> > +return 0;
> > +}
> > +
> > +static void free_jobs(QplData *qpl)
> > +{
> > +for (int i = 0; i < qpl->job_num; i++) {
> > +qpl_fini_job(qpl->job_array[i]);
> > +g_free(qpl->job_array[i]);
> > +qpl->job_array[i] = NULL;
> > +}
> > +g_free(qpl->job_array);
> > +qpl->job_array = NULL;
> > +}
> > +
> > +static int alloc_jobs(QplData *qpl, uint8_t chan_id, Error **errp)
> > +{
> > +qpl_status status;
> > +uint32_t job_size = 0;
> > +qpl_job *job = NULL;
> > +/* always use IAA hardware accelerator */
> > +qpl_path_t path = qpl_path_hardware;
> > +
> > +status = qpl_get_job_size(path, _size);
> > +if (status != QPL_STS_OK) {
> > +error_setg(errp, "multifd: %u: qpl_get_job_size failed with
> error %d",
> > + chan_id, status);
> > +return -1;
> > +}
> > +qpl->job_array = g_new0(qpl_job *, qpl->job_num);
> > +fo
Re: [PATCH v5 5/7] migration/multifd: implement initialization of qpl compression
On Wed, Mar 20, 2024 at 12:45:25AM +0800, Yuan Liu wrote:
> the qpl initialization includes memory allocation for compressed
> data and the qpl job initialization.
>
> the qpl initialization will check whether the In-Memory Analytics
> Accelerator(IAA) hardware is available, if the platform does not
> have IAA hardware or the IAA hardware is not available, the QPL
> compression initialization will fail.
>
> Signed-off-by: Yuan Liu
> Reviewed-by: Nanhai Zou
> ---
> migration/multifd-qpl.c | 243 +++-
> 1 file changed, 242 insertions(+), 1 deletion(-)
>
> diff --git a/migration/multifd-qpl.c b/migration/multifd-qpl.c
> index 056a68a060..6de65e9da7 100644
> --- a/migration/multifd-qpl.c
> +++ b/migration/multifd-qpl.c
> @@ -9,12 +9,253 @@
> * This work is licensed under the terms of the GNU GPL, version 2 or later.
> * See the COPYING file in the top-level directory.
> */
> +
> #include "qemu/osdep.h"
> #include "qemu/module.h"
> +#include "qapi/error.h"
> +#include "migration.h"
> +#include "multifd.h"
> +#include "qpl/qpl.h"
> +
> +typedef struct {
> +qpl_job **job_array;
> +/* the number of allocated jobs */
> +uint32_t job_num;
> +/* the size of data processed by a qpl job */
> +uint32_t data_size;
> +/* compressed data buffer */
> +uint8_t *zbuf;
> +/* the length of compressed data */
> +uint32_t *zbuf_hdr;
> +} QplData;
> +
> +static void free_zbuf(QplData *qpl)
> +{
> +if (qpl->zbuf != NULL) {
> +munmap(qpl->zbuf, qpl->job_num * qpl->data_size);
> +qpl->zbuf = NULL;
> +}
> +if (qpl->zbuf_hdr != NULL) {
> +g_free(qpl->zbuf_hdr);
> +qpl->zbuf_hdr = NULL;
> +}
> +}
> +
> +static int alloc_zbuf(QplData *qpl, uint8_t chan_id, Error **errp)
> +{
> +int flags = MAP_PRIVATE | MAP_POPULATE | MAP_ANONYMOUS;
> +uint32_t size = qpl->job_num * qpl->data_size;
> +uint8_t *buf;
> +
> +buf = (uint8_t *) mmap(NULL, size, PROT_READ | PROT_WRITE, flags, -1, 0);
> +if (buf == MAP_FAILED) {
> +error_setg(errp, "multifd: %u: alloc_zbuf failed, job num %u, size
> %u",
> + chan_id, qpl->job_num, qpl->data_size);
> +return -1;
> +}
What's the reason for using mmap here, rather than a normal
malloc ?
> +qpl->zbuf = buf;
> +qpl->zbuf_hdr = g_new0(uint32_t, qpl->job_num);
> +return 0;
> +}
> +
> +static void free_jobs(QplData *qpl)
> +{
> +for (int i = 0; i < qpl->job_num; i++) {
> +qpl_fini_job(qpl->job_array[i]);
> +g_free(qpl->job_array[i]);
> +qpl->job_array[i] = NULL;
> +}
> +g_free(qpl->job_array);
> +qpl->job_array = NULL;
> +}
> +
> +static int alloc_jobs(QplData *qpl, uint8_t chan_id, Error **errp)
> +{
> +qpl_status status;
> +uint32_t job_size = 0;
> +qpl_job *job = NULL;
> +/* always use IAA hardware accelerator */
> +qpl_path_t path = qpl_path_hardware;
> +
> +status = qpl_get_job_size(path, _size);
> +if (status != QPL_STS_OK) {
> +error_setg(errp, "multifd: %u: qpl_get_job_size failed with error
> %d",
> + chan_id, status);
> +return -1;
> +}
> +qpl->job_array = g_new0(qpl_job *, qpl->job_num);
> +for (int i = 0; i < qpl->job_num; i++) {
> +job = g_malloc0(job_size);
> +status = qpl_init_job(path, job);
> +if (status != QPL_STS_OK) {
> +error_setg(errp, "multifd: %u: qpl_init_job failed with error
> %d",
> + chan_id, status);
> +free_jobs(qpl);
> +return -1;
> +}
> +qpl->job_array[i] = job;
> +}
> +return 0;
> +}
> +
> +static int init_qpl(QplData *qpl, uint32_t job_num, uint32_t data_size,
> +uint8_t chan_id, Error **errp)
> +{
IMHO this method should be a normal constructor, it it should
be responsible for allocating 'qpl' struct too, and returning
it, not have the caller allocate it.
> +qpl->job_num = job_num;
> +qpl->data_size = data_size;
> +if (alloc_zbuf(qpl, chan_id, errp) != 0) {
> +return -1;
> +}
> +if (alloc_jobs(qpl, chan_id, errp) != 0) {
> +free_zbuf(qpl);
> +return -1;
> +}
> +return 0;
> +}
> +
> +static void deinit_qpl(QplData *qpl)
> +{
> +if (qpl != NULL) {
> +free_jobs(qpl);
> +free_zbuf(qpl);
> +qpl->job_num = 0;
> +qpl->data_size = 0;
> +}
> +}
This should also free 'qpl' instead of leaving it upto the
caller.
> +
> +/**
> + * qpl_send_setup: setup send side
> + *
> + * Setup each channel with QPL compression.
> + *
> + * Returns 0 for success or -1 for error
> + *
> + * @p: Params for the channel that we are using
> + * @errp: pointer to an error
> + */
> +static int qpl_send_setup(MultiFDSendParams *p, Error **errp)
> +{
> +QplData *qpl;
> +
> +qpl = g_new0(QplData, 1);
> +if (init_qpl(qpl, p->page_count, p->page_size, p->id, errp) != 0) {
> +
