> > As promised, I have attached a patch to solve the problem. Instead of
> > doing as I had originally proposed, I simply added a wake up signal
> > to a sleeping thread if partial updates are enabled. When the worker
> > wakes up, it checks if no more input
> > is available and signals to the main thread if it has output ready
> > before going back
> > to sleep. This prevents the deadlock on my liblzma tests and testing
> > xz with/without timeout.
>
> Thanks to both of you for debugging this. I see now that I had
> completely missed this corner case. The patch looks correct except that
> the mutex locking order is wrong which can cause a new deadlock. If
> both thr->mutex and coder->mutex are locked at the same time,
> coder->mutex must be locked first.
Good catch on the deadlock I created by the order of the mutex locking.
I had forgotten that the cond_wait locks the mutex again after the
sleep is done.
Would you like me to fix this and submit another patch?
> About memlimit updates, that may indeed need some work but I don't know
> yet how much is worth the trouble. stream_decoder_mt_memconfig() has a
> few FIXMEs too, maybe they don't need to be changed but it needs to be
> decided.
I attached two patches to this message. The first should fix a bug
with the timeouts.
When a timeout is set and the main thread times out, ret = LZMA_TIMED_OUT.
The check at the end of the function checks if ret == LZMA_OK, and if
not it will
call threads_stop. If it stops a thread before it can mark the outbuf
as finished, the
main thread will always wait on that thread since its buffer is not finished.
The second patch is for the memlimit_threading update. I added a new API
function that will fail for anything that is not the multithreaded
decoder. I tested it
locally and it successfully resumed multithreading after adjusting the
function was
called.
>I'm in a hurry now but I should have time for xz next week. :-)
Let me know your thoughts on these patches when you have a chance :)
Jia Tan
From be3695ec59b62eb1c0872005ef58d54211b99cf1 Mon Sep 17 00:00:00 2001
From: jiat75 <jiat0...@gmail.com>
Date: Thu, 17 Mar 2022 00:55:14 +0800
Subject: [PATCH] Fixes bug with timeout in multithreaded decoder
The LZMA_TIMED_OUT was causing threads_stop to stop worker threads that were in
progress, so they were not able to mark their output_buf to finished. This
caused the main thread to believe the worker thread was still working on the
block, even though it had already been stopped.
---
src/liblzma/common/stream_decoder_mt.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/liblzma/common/stream_decoder_mt.c b/src/liblzma/common/stream_decoder_mt.c
index 1665e22b..1828378c 100644
--- a/src/liblzma/common/stream_decoder_mt.c
+++ b/src/liblzma/common/stream_decoder_mt.c
@@ -817,7 +817,7 @@ read_output_and_wait(struct lzma_stream_coder *coder,
// If we are returning an error, then the application cannot get
// more output from us and thus keeping the threads running is
// useless and waste of CPU time.
- if (ret != LZMA_OK)
+ if (ret != LZMA_OK && ret != LZMA_TIMED_OUT)
threads_stop(coder);
return ret;
--
2.32.0
From 8daba6345e711bfd1443b58b8566705b7670ac67 Mon Sep 17 00:00:00 2001
From: jiat75 <jiat0...@gmail.com>
Date: Thu, 17 Mar 2022 18:07:50 +0800
Subject: [PATCH] Created lzma_memlimit_threading_set
Can now change memlimit_threading in liblzma after
initialization of the stream
---
src/liblzma/api/lzma/base.h | 17 +++++++++++++++++
src/liblzma/common/common.c | 23 +++++++++++++++++++++++
src/liblzma/common/common.h | 8 ++++++++
src/liblzma/common/stream_decoder_mt.c | 15 +++++++++++++++
src/liblzma/liblzma.map | 1 +
5 files changed, 64 insertions(+)
diff --git a/src/liblzma/api/lzma/base.h b/src/liblzma/api/lzma/base.h
index 749e4fe7..0de0f309 100644
--- a/src/liblzma/api/lzma/base.h
+++ b/src/liblzma/api/lzma/base.h
@@ -699,3 +699,20 @@ extern LZMA_API(uint64_t) lzma_memlimit_get(const lzma_stream *strm)
*/
extern LZMA_API(lzma_ret) lzma_memlimit_set(
lzma_stream *strm, uint64_t memlimit) lzma_nothrow;
+
+/**
+ * @brief
+ * This function is only supported by lzma_decoder_mt. This function allows
+ * changing the memlimit_threading value after initialization to adjust
+ * the memory limit before single threaded mode is used
+ *
+ * \param strm Pointer to lzma_stream that is at least
+ * initialized with LZMA_STREAM_INIT.
+ * \param memlimit New value for memlimit_threading
+ *
+ * \return -LZMA_OK: New memlimit_threading value successfully set
+ * -LZMA_PROG_ERROR: Invalid arguments, e.g. *strm doesn't
+ * support memlimit_threading or was not properly initialized
+ */
+extern LZMA_API(lzma_ret) lzma_memlimit_threading_set(
+ lzma_stream *strm, uint64_t new_memlimit) lzma_nothrow;
diff --git a/src/liblzma/common/common.c b/src/liblzma/common/common.c
index 346fc7af..1755e617 100644
--- a/src/liblzma/common/common.c
+++ b/src/liblzma/common/common.c
@@ -455,3 +455,26 @@ lzma_memlimit_set(lzma_stream *strm, uint64_t new_memlimit)
return strm->internal->next.memconfig(strm->internal->next.coder,
&memusage, &old_memlimit, new_memlimit);
}
+
+
+extern LZMA_API(lzma_ret)
+lzma_memlimit_threading_set(lzma_stream *strm, uint64_t new_memlimit)
+{
+ // Dummy variables to simplify memconfig functions
+ uint64_t old_memlimit;
+
+ if (strm == NULL || strm->internal == NULL
+ || strm->internal->next.set_memlimit_threading
+ == NULL)
+ return LZMA_PROG_ERROR;
+
+ // Zero is a special value that cannot be used as an actual limit.
+ // If 0 was specified, use 1 instead.
+ if (new_memlimit == 0)
+ new_memlimit = 1;
+
+ return strm->internal->next.set_memlimit_threading(
+ strm->internal->next.coder,
+ &old_memlimit,
+ new_memlimit);
+}
diff --git a/src/liblzma/common/common.h b/src/liblzma/common/common.h
index 67996228..2c886059 100644
--- a/src/liblzma/common/common.h
+++ b/src/liblzma/common/common.h
@@ -186,6 +186,13 @@ struct lzma_next_coder_s {
/// seen, LZMA_OK is allowed too.
lzma_ret (*set_out_limit)(void *coder, uint64_t *uncomp_size,
uint64_t out_limit);
+
+ /// Set how many bytes the memlimit_threading option should be
+ /// This is currently only supported in the multithreaded decoder
+ /// to control how much memory can be used until single threaded
+ /// mode is used instead
+ lzma_ret (*set_memlimit_threading)(void *coder, uint64_t *old_limit,
+ uint64_t new_limit);
};
@@ -202,6 +209,7 @@ struct lzma_next_coder_s {
.memconfig = NULL, \
.update = NULL, \
.set_out_limit = NULL, \
+ .set_memlimit_threading = NULL \
}
diff --git a/src/liblzma/common/stream_decoder_mt.c b/src/liblzma/common/stream_decoder_mt.c
index 1828378c..ba174bbe 100644
--- a/src/liblzma/common/stream_decoder_mt.c
+++ b/src/liblzma/common/stream_decoder_mt.c
@@ -1707,6 +1707,19 @@ stream_decoder_mt_get_progress(void *coder_ptr,
}
+static lzma_ret
+stream_decoder_mt_set_memlimit_threading(void *code_ptr, uint64_t *old_limit,
+ uint64_t new_limit)
+{
+ struct lzma_stream_coder *coder = code_ptr;
+ // No need to lock mutex since this value is only
+ // read by the main thread
+ *old_limit = coder->memlimit_threading;
+ coder->memlimit_threading = new_limit;
+ return LZMA_OK;
+}
+
+
static lzma_ret
stream_decoder_mt_init(lzma_next_coder *next, const lzma_allocator *allocator,
const lzma_mt *options)
@@ -1745,6 +1758,8 @@ stream_decoder_mt_init(lzma_next_coder *next, const lzma_allocator *allocator,
next->get_check = &stream_decoder_mt_get_check;
next->memconfig = &stream_decoder_mt_memconfig;
next->get_progress = &stream_decoder_mt_get_progress;
+ next->set_memlimit_threading =
+ &stream_decoder_mt_set_memlimit_threading;
memzero(coder->filters, sizeof(coder->filters));
memzero(&coder->outq, sizeof(coder->outq));
diff --git a/src/liblzma/liblzma.map b/src/liblzma/liblzma.map
index 4462dac4..825fe0da 100644
--- a/src/liblzma/liblzma.map
+++ b/src/liblzma/liblzma.map
@@ -110,6 +110,7 @@ global:
lzma_microlzma_encoder;
lzma_file_info_decoder;
lzma_stream_decoder_mt;
+ lzma_memlimit_threading_set;
local:
*;
--
2.32.0
