| Message ID | 20240910210450.3835123-1-peterx@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | migration/multifd: Fix build for qatzip | expand |
Peter Xu <peterx@redhat.com> writes: > The qatzip series was based on an older commit, it applied cleanly even > though it has conflicts. Neither CI nor myself found the build will break > as it's skipped by default when qatzip library was missing. It took longer than I expected. Do you think it would work if we wrapped all calls to external functions in a header and stubbed them out when there's no accelerator support?
On Tue, Sep 10, 2024 at 06:35:50PM -0300, Fabiano Rosas wrote: > Peter Xu <peterx@redhat.com> writes: > > > The qatzip series was based on an older commit, it applied cleanly even > > though it has conflicts. Neither CI nor myself found the build will break > > as it's skipped by default when qatzip library was missing. > > It took longer than I expected. What took longer? > > Do you think it would work if we wrapped all calls to external functions > in a header and stubbed them out when there's no accelerator support? I didn't catch the major benefit v.s. multifd_register_ops(). Any further elaborations?
On Tue, Sep 10, 2024 at 2:05 PM Peter Xu <peterx@redhat.com> wrote: > > The qatzip series was based on an older commit, it applied cleanly even > though it has conflicts. Neither CI nor myself found the build will break > as it's skipped by default when qatzip library was missing. > > Fix the build issues. No need to copy stable as it just landed 9.2. > > Cc: Yichen Wang <yichen.wang@bytedance.com> > Cc: Bryan Zhang <bryan.zhang@bytedance.com> > Cc: Hao Xiang <hao.xiang@linux.dev> > Cc: Yuan Liu <yuan1.liu@intel.com> > Fixes: 80484f9459 ("migration: Introduce 'qatzip' compression method") > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > > Qatzip developers: would you help me to double check whether this is the > right fix? It makes sense and looks good to me. Thanks a lot for correcting this. > --- > migration/multifd-qatzip.c | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/migration/multifd-qatzip.c b/migration/multifd-qatzip.c > index 3c787ed879..7b68397625 100644 > --- a/migration/multifd-qatzip.c > +++ b/migration/multifd-qatzip.c > @@ -160,7 +160,8 @@ static void qatzip_send_cleanup(MultiFDSendParams *p, Error **errp) > */ > static int qatzip_send_prepare(MultiFDSendParams *p, Error **errp) > { > - MultiFDPages_t *pages = p->pages; > + uint32_t page_size = multifd_ram_page_size(); > + MultiFDPages_t *pages = &p->data->u.ram; > QatzipData *q = p->compress_data; > int ret; > unsigned int in_len, out_len; > @@ -179,12 +180,12 @@ static int qatzip_send_prepare(MultiFDSendParams *p, Error **errp) > * implementation. > */ > for (int i = 0; i < pages->normal_num; i++) { > - memcpy(q->in_buf + (i * p->page_size), > + memcpy(q->in_buf + (i * page_size), > pages->block->host + pages->offset[i], > - p->page_size); > + page_size); > } > > - in_len = pages->normal_num * p->page_size; > + in_len = pages->normal_num * page_size; > if (in_len > q->in_len) { > error_setg(errp, "multifd %u: unexpectedly large input", p->id); > return -1; > @@ -197,7 +198,7 @@ static int qatzip_send_prepare(MultiFDSendParams *p, Error **errp) > p->id, ret); > return -1; > } > - if (in_len != pages->normal_num * p->page_size) { > + if (in_len != pages->normal_num * page_size) { > error_setg(errp, "multifd %u: QATzip failed to compress all input", > p->id); > return -1; > @@ -329,7 +330,8 @@ static int qatzip_recv(MultiFDRecvParams *p, Error **errp) > int ret; > unsigned int in_len, out_len; > uint32_t in_size = p->next_packet_size; > - uint32_t expected_size = p->normal_num * p->page_size; > + uint32_t page_size = multifd_ram_page_size(); > + uint32_t expected_size = p->normal_num * page_size; > uint32_t flags = p->flags & MULTIFD_FLAG_COMPRESSION_MASK; > > if (in_size > q->in_len) { > @@ -370,9 +372,7 @@ static int qatzip_recv(MultiFDRecvParams *p, Error **errp) > > /* Copy each page to its appropriate location. */ > for (int i = 0; i < p->normal_num; i++) { > - memcpy(p->host + p->normal[i], > - q->out_buf + p->page_size * i, > - p->page_size); > + memcpy(p->host + p->normal[i], q->out_buf + page_size * i, page_size); > } > return 0; > } > -- > 2.45.0 >
Peter Xu <peterx@redhat.com> writes: > On Tue, Sep 10, 2024 at 06:35:50PM -0300, Fabiano Rosas wrote: >> Peter Xu <peterx@redhat.com> writes: >> >> > The qatzip series was based on an older commit, it applied cleanly even >> > though it has conflicts. Neither CI nor myself found the build will break >> > as it's skipped by default when qatzip library was missing. >> >> It took longer than I expected. > > What took longer? For a change that breaks the build to be committed in one of these parts of the code that are disabled by default. You might remember I told you in one of our meetings that I was concerned about that. > >> >> Do you think it would work if we wrapped all calls to external functions >> in a header and stubbed them out when there's no accelerator support? > > I didn't catch the major benefit v.s. multifd_register_ops(). Any further > elaborations? I'm trying to find a way of having more code compiled by default and only a minimal amount of code put under the CONFIG_FOO options. So if some multifd code depends on a library call, say deflateInit, we make that a multifd_deflate_init and add a stub for when !ZLIB (just an example). I'm not sure it's feasible though, I'm just bouncing the idea off of you.
> -----Original Message----- > From: Peter Xu <peterx@redhat.com> > Sent: Wednesday, September 11, 2024 5:05 AM > To: qemu-devel@nongnu.org > Cc: peterx@redhat.com; Fabiano Rosas <farosas@suse.de>; Prasad Pandit > <ppandit@redhat.com>; Wang, Yichen <yichen.wang@bytedance.com>; Bryan > Zhang <bryan.zhang@bytedance.com>; Hao Xiang <hao.xiang@linux.dev>; Liu, > Yuan1 <yuan1.liu@intel.com> > Subject: [PATCH] migration/multifd: Fix build for qatzip > > The qatzip series was based on an older commit, it applied cleanly even > though it has conflicts. Neither CI nor myself found the build will break > as it's skipped by default when qatzip library was missing. > > Fix the build issues. No need to copy stable as it just landed 9.2. > > Cc: Yichen Wang <yichen.wang@bytedance.com> > Cc: Bryan Zhang <bryan.zhang@bytedance.com> > Cc: Hao Xiang <hao.xiang@linux.dev> > Cc: Yuan Liu <yuan1.liu@intel.com> > Fixes: 80484f9459 ("migration: Introduce 'qatzip' compression method") > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > > Qatzip developers: would you help me to double check whether this is the > right fix? Looks good to me, thanks > migration/multifd-qatzip.c | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/migration/multifd-qatzip.c b/migration/multifd-qatzip.c > index 3c787ed879..7b68397625 100644 > --- a/migration/multifd-qatzip.c > +++ b/migration/multifd-qatzip.c > @@ -160,7 +160,8 @@ static void qatzip_send_cleanup(MultiFDSendParams *p, > Error **errp) > */ > static int qatzip_send_prepare(MultiFDSendParams *p, Error **errp) > { > - MultiFDPages_t *pages = p->pages; > + uint32_t page_size = multifd_ram_page_size(); > + MultiFDPages_t *pages = &p->data->u.ram; > QatzipData *q = p->compress_data; > int ret; > unsigned int in_len, out_len; > @@ -179,12 +180,12 @@ static int qatzip_send_prepare(MultiFDSendParams *p, > Error **errp) > * implementation. > */ > for (int i = 0; i < pages->normal_num; i++) { > - memcpy(q->in_buf + (i * p->page_size), > + memcpy(q->in_buf + (i * page_size), > pages->block->host + pages->offset[i], > - p->page_size); > + page_size); > } > > - in_len = pages->normal_num * p->page_size; > + in_len = pages->normal_num * page_size; > if (in_len > q->in_len) { > error_setg(errp, "multifd %u: unexpectedly large input", p->id); > return -1; > @@ -197,7 +198,7 @@ static int qatzip_send_prepare(MultiFDSendParams *p, > Error **errp) > p->id, ret); > return -1; > } > - if (in_len != pages->normal_num * p->page_size) { > + if (in_len != pages->normal_num * page_size) { > error_setg(errp, "multifd %u: QATzip failed to compress all > input", > p->id); > return -1; > @@ -329,7 +330,8 @@ static int qatzip_recv(MultiFDRecvParams *p, Error > **errp) > int ret; > unsigned int in_len, out_len; > uint32_t in_size = p->next_packet_size; > - uint32_t expected_size = p->normal_num * p->page_size; > + uint32_t page_size = multifd_ram_page_size(); > + uint32_t expected_size = p->normal_num * page_size; > uint32_t flags = p->flags & MULTIFD_FLAG_COMPRESSION_MASK; > > if (in_size > q->in_len) { > @@ -370,9 +372,7 @@ static int qatzip_recv(MultiFDRecvParams *p, Error > **errp) > > /* Copy each page to its appropriate location. */ > for (int i = 0; i < p->normal_num; i++) { > - memcpy(p->host + p->normal[i], > - q->out_buf + p->page_size * i, > - p->page_size); > + memcpy(p->host + p->normal[i], q->out_buf + page_size * i, > page_size); > } > return 0; > } > -- > 2.45.0
On Tue, Sep 10, 2024 at 07:32:19PM -0300, Fabiano Rosas wrote: > I'm trying to find a way of having more code compiled by default and > only a minimal amount of code put under the CONFIG_FOO options. So if > some multifd code depends on a library call, say deflateInit, we make > that a multifd_deflate_init and add a stub for when !ZLIB (just an > example). I'm not sure it's feasible though, I'm just bouncing the idea > off of you. Not sure how much it helps. It adds more work, add slightly more code to maintain (then we will then need to maintain the shim layer, and that's per-compressor), while I am not sure it'll be good enough either.. For example, even if it compiles it can still run into constant failure when with the real library / hardware underneath. This not so bad to me yet: do you still remember or aware of the "joke" on how people remove a feature in Linux? One can introduce a bug that can directly crash when some feature enabled, then after two years the developer can say "see, this feature is not used by anyone, let's remove it". I think it's a joke (which might come from reality..) but it's kind of a way that how we should treat these compressors as a start, IMHO. AFAIU many of these compressors start with PoC-type projects where it's used to justify the hardware features. The next step is in production use but that requires software vendors to involve, IIUC. I think that's what we're waiting for, on company use it in more serious way that sign these features off. I don't think all such compressors will reach that point. Meanwhile I don't think we (as qemu migration maintainers) can maintain that code well, if we don't get sponsored by people with hardwares to test. I think it means it's not our job to maintain it at 100%, yet so far. We will still try our best, but that's always limited. As we discussed before, we always need to rely on vendors so far for most of them. If after a few releases we found it's broken so bad, it may mean it finished its job as PoC or whatever purpose it services. It means we could choose to move on, with no joking. That's why I think it's not so urgent, and maybe we don't need extra effort to make it harder for us to notice nobody is using it - we keep everything we know productions are actively using seriously (like multifd, postcopy, etc.). Either some compressors become part of the serious use case, or we move on. I recently do find more that the only way to make QEMU keep living well is to sometimes throw things away..
Peter Xu <peterx@redhat.com> writes: > On Tue, Sep 10, 2024 at 07:32:19PM -0300, Fabiano Rosas wrote: >> I'm trying to find a way of having more code compiled by default and >> only a minimal amount of code put under the CONFIG_FOO options. So if >> some multifd code depends on a library call, say deflateInit, we make >> that a multifd_deflate_init and add a stub for when !ZLIB (just an >> example). I'm not sure it's feasible though, I'm just bouncing the idea >> off of you. > > Not sure how much it helps. It adds more work, add slightly more code to > maintain (then we will then need to maintain the shim layer, and that's > per-compressor), while I am not sure it'll be good enough either.. For > example, even if it compiles it can still run into constant failure when > with the real library / hardware underneath. > > This not so bad to me yet: do you still remember or aware of the "joke" on > how people remove a feature in Linux? One can introduce a bug that can > directly crash when some feature enabled, then after two years the > developer can say "see, this feature is not used by anyone, let's remove > it". > > I think it's a joke (which might come from reality..) but it's kind of a > way that how we should treat these compressors as a start, IMHO. AFAIU > many of these compressors start with PoC-type projects where it's used to > justify the hardware features. The next step is in production use but that > requires software vendors to involve, IIUC. I think that's what we're > waiting for, on company use it in more serious way that sign these features > off. > > I don't think all such compressors will reach that point. Meanwhile I > don't think we (as qemu migration maintainers) can maintain that code well, > if we don't get sponsored by people with hardwares to test. > > I think it means it's not our job to maintain it at 100%, yet so far. We > will still try our best, but that's always limited. As we discussed > before, we always need to rely on vendors so far for most of them. > > If after a few releases we found it's broken so bad, it may mean it > finished its job as PoC or whatever purpose it services. It means we could > choose to move on, with no joking. > > That's why I think it's not so urgent, and maybe we don't need extra effort > to make it harder for us to notice nobody is using it - we keep everything > we know productions are actively using seriously (like multifd, postcopy, > etc.). Either some compressors become part of the serious use case, or we > move on. I recently do find more that the only way to make QEMU keep > living well is to sometimes throw things away.. Ok, that's all fair. I agree we can continue with that policy. Thanks for sharing your thoughts.
diff --git a/migration/multifd-qatzip.c b/migration/multifd-qatzip.c index 3c787ed879..7b68397625 100644 --- a/migration/multifd-qatzip.c +++ b/migration/multifd-qatzip.c @@ -160,7 +160,8 @@ static void qatzip_send_cleanup(MultiFDSendParams *p, Error **errp) */ static int qatzip_send_prepare(MultiFDSendParams *p, Error **errp) { - MultiFDPages_t *pages = p->pages; + uint32_t page_size = multifd_ram_page_size(); + MultiFDPages_t *pages = &p->data->u.ram; QatzipData *q = p->compress_data; int ret; unsigned int in_len, out_len; @@ -179,12 +180,12 @@ static int qatzip_send_prepare(MultiFDSendParams *p, Error **errp) * implementation. */ for (int i = 0; i < pages->normal_num; i++) { - memcpy(q->in_buf + (i * p->page_size), + memcpy(q->in_buf + (i * page_size), pages->block->host + pages->offset[i], - p->page_size); + page_size); } - in_len = pages->normal_num * p->page_size; + in_len = pages->normal_num * page_size; if (in_len > q->in_len) { error_setg(errp, "multifd %u: unexpectedly large input", p->id); return -1; @@ -197,7 +198,7 @@ static int qatzip_send_prepare(MultiFDSendParams *p, Error **errp) p->id, ret); return -1; } - if (in_len != pages->normal_num * p->page_size) { + if (in_len != pages->normal_num * page_size) { error_setg(errp, "multifd %u: QATzip failed to compress all input", p->id); return -1; @@ -329,7 +330,8 @@ static int qatzip_recv(MultiFDRecvParams *p, Error **errp) int ret; unsigned int in_len, out_len; uint32_t in_size = p->next_packet_size; - uint32_t expected_size = p->normal_num * p->page_size; + uint32_t page_size = multifd_ram_page_size(); + uint32_t expected_size = p->normal_num * page_size; uint32_t flags = p->flags & MULTIFD_FLAG_COMPRESSION_MASK; if (in_size > q->in_len) { @@ -370,9 +372,7 @@ static int qatzip_recv(MultiFDRecvParams *p, Error **errp) /* Copy each page to its appropriate location. */ for (int i = 0; i < p->normal_num; i++) { - memcpy(p->host + p->normal[i], - q->out_buf + p->page_size * i, - p->page_size); + memcpy(p->host + p->normal[i], q->out_buf + page_size * i, page_size); } return 0; }
The qatzip series was based on an older commit, it applied cleanly even though it has conflicts. Neither CI nor myself found the build will break as it's skipped by default when qatzip library was missing. Fix the build issues. No need to copy stable as it just landed 9.2. Cc: Yichen Wang <yichen.wang@bytedance.com> Cc: Bryan Zhang <bryan.zhang@bytedance.com> Cc: Hao Xiang <hao.xiang@linux.dev> Cc: Yuan Liu <yuan1.liu@intel.com> Fixes: 80484f9459 ("migration: Introduce 'qatzip' compression method") Signed-off-by: Peter Xu <peterx@redhat.com> --- Qatzip developers: would you help me to double check whether this is the right fix? --- migration/multifd-qatzip.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)