compression: Replace global variables with per-fd context #6913

seokhun-eom24 · 2025-07-30T00:43:32Z

Previous, global variables were used for compress_read from file.
compress_read can be called without compress_init when global variables were set by other fd's compress_init.
And when multiple files call compress_read, it caused a problem that the previous fd's state was affected by the current fd's state.

This patch replaces global variables with per-fd context to prevent this problem.
Maintain all live contexts in a dq_queue_t with helpers to find/add/remove by fd.
Store the comp_ctx in the driver's filep->f_priv for the opened fd.
Driver's ioctl args are not changed.
This patch enables concurrent decompression by eliminating global varialbe and isolating each operation's context.

Additionaly adds explicit comp_open() and comp_close() functions for drivers.

API changes in compress_read.h

compress_uninit(void) → compress_uninit(int filfd)
get_compression_header(void) → get_compression_header(int filfd)
New: initialize_comp_ctx_list(void) (called once during system init)

ewoodev · 2025-07-30T01:07:38Z

os/drivers/compression/compress.c

+	comp_semtake(&dev->exclsem);
+	if (dev->refs) {
+		comp_semgive(&dev->exclsem);
+		return -EMFILE; // We allow only 1 ref count;


lzma and miniz allow multiple calling.
we don't need to block this driver

Thanks.
I'll fix it and modify compress_read.c too.

Taejun-Kwon

VFS layer already check validation of fileep in fs_ioctl, so you can remove all DEBUGASSERT(filep) from ioctl.

Taejun-Kwon · 2025-07-30T00:51:40Z

os/drivers/compression/compress.c

+{
+	struct comp_dev_s *dev = filep->f_inode->i_private;
+
+	if (!dev) {


EINVAL is not proper value here.
You can add DEBUGASSERT(dev != NULL), comp_dev_s must be added when driver registered.

Taejun-Kwon · 2025-07-30T01:14:31Z

os/drivers/compression/compress.c


+struct comp_dev_s {
+	bool refs;
+	sem_t exclsem;


I think you can add 'int running' here.
Then you can use it as below.

case COMPIOC_FCOMP_INIT: if ((char *)arg == NULL) { return -EINVAL; } if (dev->running) { return -EBUSY; } ... break; case COMPIOC_FCOMP_DEINIT: if (!dev->running) { return -EACCES; //or -EIO }

Also instead of refs, I think 'reserved' is better name.

Good idea.
I will refer to the code when I edit it later.

seokhun-eom24 · 2025-07-30T04:40:55Z

VFS layer already check validation of fileep in fs_ioctl, so you can remove all DEBUGASSERT(filep) from ioctl.

I checked. Thank you.

jeongarmy · 2025-07-30T07:18:23Z

os/drivers/compression/compress.c

 };

+struct comp_dev_s {
+	sem_t sem;


Please include header for semaphore

Thank you for check.

jeongarmy · 2025-07-30T07:58:20Z

os/drivers/compression/compress.c

+	DEBUGASSERT(dev != NULL);
+
+	comp_semtake(&dev->sem);
+	if (dev->crefs == (uint8_t)255) {


Please change this to UINT8_MAX or use defined value like COMP_REF_MAX

Fixed using COMP_CREF_MAX.

jeongarmy · 2025-07-31T04:09:56Z

os/drivers/compression/compress.c

 {
-	(void)register_driver(COMP_DRVPATH, &compress_fops, 0666, NULL);
+	int ret;
+	struct comp_dev_s *dev = (struct comp_dev_s *)kmm_malloc(sizeof(struct comp_dev_s));


crefs initialization is missing.
Please change kmm_malloc to kmm_zalloc or initialize crefs to 0

I'll fix this. Thank you.

jeongarmy · 2025-08-07T07:24:52Z

os/drivers/compression/compress.c

 ****************************************************************************/
 void compress_register(void)
 {
+	initialize_comp_ctx_list();


I think we need to set flag to check initialization. If compress_register is called multiple times during running by mistake, an assert or error seems to be occured because of dq_init.

Fixed to avoid double initialization.

Previous, global variables were used for compress_read from file. compress_read can be called without compress_init when global variables were set by other fd's compress_init. And when multiple files call compress_read, it caused a problem that the previous fd's state was affected by the current fd's state. This patch replaces global variables with per-fd context to prevent this problem. Maintain all live contexts in a dq_queue_t with helpers to find/add/remove by fd. Store the comp_ctx in the driver's filep->f_priv for the opened fd. Driver's ioctl args are not changed. This patch enables concurrent decompression by eliminating global varialbe and isolating each operation's context. Additionaly adds explicit comp_open() and comp_close() functions for drivers. * API changes in compress_read.h 1. compress_uninit(void) → compress_uninit(int filfd) 2. get_compression_header(void) → get_compression_header(int filfd) 3. New: initialize_comp_ctx_list(void) (called once during system init) Signed-off-by: seokhun-eom <[email protected]>

sunghan-chang

@seokhun-eom24 Could you leave how to verify the change in commit description?

seokhun-eom24 · 2025-08-29T00:35:36Z

@seokhun-eom24 Could you leave how to verify the change in commit description?

I'll add test case for compression drivers concurrently with new commit.

seokhun-eom24 · 2025-09-01T08:12:55Z

Hello, @kishore-sn
Can you review this PR?

seokhun-eom24 mentioned this pull request Jul 30, 2025

os/drivers: Modify to return errno from compression ioctl #6911

Merged

ewoodev reviewed Jul 30, 2025

View reviewed changes

Taejun-Kwon reviewed Jul 30, 2025

View reviewed changes

seokhun-eom24 force-pushed the 250730-add-compress-drive-open-close branch 3 times, most recently from 8e61fd1 to 85b15b0 Compare July 30, 2025 05:55

jeongarmy reviewed Jul 30, 2025

View reviewed changes

seokhun-eom24 force-pushed the 250730-add-compress-drive-open-close branch from 85b15b0 to 4769ae5 Compare July 30, 2025 07:49

jeongarmy reviewed Jul 30, 2025

View reviewed changes

seokhun-eom24 force-pushed the 250730-add-compress-drive-open-close branch 2 times, most recently from bbd03dc to 9315c58 Compare July 30, 2025 10:01

jeongarmy reviewed Jul 31, 2025

View reviewed changes

seokhun-eom24 marked this pull request as draft July 31, 2025 22:13

seokhun-eom24 force-pushed the 250730-add-compress-drive-open-close branch from 9315c58 to fcd08c8 Compare August 4, 2025 03:46

seokhun-eom24 marked this pull request as ready for review August 4, 2025 03:46

seokhun-eom24 force-pushed the 250730-add-compress-drive-open-close branch 3 times, most recently from 928bea9 to 851d16d Compare August 4, 2025 04:49

seokhun-eom24 changed the title ~~drivers/compression: implement comp_open, comp_close~~ compression: Replace global variables with per-fd context Aug 4, 2025

jeongarmy reviewed Aug 7, 2025

View reviewed changes

seokhun-eom24 force-pushed the 250730-add-compress-drive-open-close branch 2 times, most recently from 1cbf808 to 1e95268 Compare August 11, 2025 00:20

seokhun-eom24 force-pushed the 250730-add-compress-drive-open-close branch from 1e95268 to 45b9603 Compare August 20, 2025 07:50

seokhun-eom24 marked this pull request as draft August 21, 2025 05:30

seokhun-eom24 force-pushed the 250730-add-compress-drive-open-close branch from 45b9603 to 6f002e7 Compare August 21, 2025 06:03

seokhun-eom24 marked this pull request as ready for review August 21, 2025 06:35

sunghan-chang reviewed Aug 28, 2025

View reviewed changes

compression: Replace global variables with per-fd context #6913

Are you sure you want to change the base?

compression: Replace global variables with per-fd context #6913

Uh oh!

Conversation

seokhun-eom24 commented Jul 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

API changes in compress_read.h

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Taejun-Kwon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seokhun-eom24 commented Jul 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeongarmy Aug 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sunghan-chang left a comment

Choose a reason for hiding this comment

Uh oh!

seokhun-eom24 commented Aug 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seokhun-eom24 commented Sep 1, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

seokhun-eom24 commented Jul 30, 2025 •

edited

Loading

jeongarmy Aug 7, 2025 •

edited

Loading

seokhun-eom24 commented Aug 29, 2025 •

edited

Loading