log_dump: Add api to get size of the log dump #6394

ritesh55555 · 2024-09-11T12:10:50Z

This commit adds an api to get the size of the compressed log dump.

sunghan-chang

Let's remove log_dump.c from the title. You don't make a change only in that file.

apps/examples/log_dump/log_dump_main.c

sunghan-chang · 2024-09-11T12:25:25Z

os/kernel/log_dump/log_dump.c

+size_t log_dump_get_size(void)
+{
+	size_t size = 0;
+	struct log_dump_chunk_s* head = (struct log_dump_chunk_s *)sq_peek(&log_dump_chunks);


In code readability, head != tail? It looks strange.
How about renaming the variable like log_node?
It does not have the head always.

sunghan-chang · 2024-09-11T12:26:48Z

os/kernel/log_dump/log_dump.c

+	while (head != (struct log_dump_chunk_s *)sq_tail(&log_dump_chunks)) {
+		head = (struct log_dump_chunk_s *)sq_next((sq_entry_t *)head);
+		size += CONFIG_LOG_DUMP_CHUNK_SIZE;
+	}


Don't you need to do lock / unlock for log dump?

If tail should be fixed, I think we can get it once. Not every while loop.

How about calculating current size at every node allocation and free in some static global variable? We already need to get current size to adjust node total size based on free heap size.

sunghan-chang · 2024-09-11T12:31:33Z

os/kernel/log_dump/log_dump.c

+	 * Note: It is recommended to use this log_dump_get_size api after
+	 * log_dump_read_wake() is performed, so that it has correct value of last_comp_block_size.


App will use this API and App can't know whether log_dump_read_wake is performed or not. This recommendation looks redundant. Is there any other way to do it automatically?

sunghan-chang · 2024-09-11T12:34:58Z

os/kernel/log_dump/log_dump.c

+	 * Note: It is recommended to use this log_dump_get_size api after
+	 * log_dump_read_wake() is performed, so that it has correct value of last_comp_block_size.
+	 * Else it might add old last_comp_block_size which is already present in log_dump_chunk list */
+	size += last_comp_block_size;


You return compressed log size + uncompressed log size. When we read saved log dump, don't we compress uncompressed log?? If we don't, then how can we know how many data should be uncompressed and how many data should be used as it is?

Earlier, when we were opening the procfs file of log dump, it was compressing the partially filled log buffer. Now the compressing the partially filled log buffer has been shifted to when log dump save is stopped . So that whenever we stop log dump process, we will compress the partially filled log buffer and update the size.
two cases while getting log dump size:

When log dump save is true and log dump is saving character : In this case , log dump size will return the size of compressed logs till now in the log_dump_chuck and it will not include last_comp_block_size becuase it is not valid.

when log dump save is false and log dump is not saving : In this case , log dump size will include compressed logs in log_dump_chunk + last_comp_block_size becuase after log dump save is stopped we will compress the last partially filled logs.

sunghan-chang · 2024-09-11T12:36:42Z

@ritesh55555 One question, after getting the size using new API, is there any chance that node size is changed?
If it can be, app will do abnormal operation.

apps/examples/log_dump/log_dump_main.c

ritesh55555 · 2024-09-12T07:36:12Z

@ritesh55555 One question, after getting the size using new API, is there any chance that node size is changed? If it can be, app will do abnormal operation.

It is recommended that log dump size and log dump read should be done after stopping log dump save process. Else app might not get the value wanted .
Apart from that if app tries to get log dump size during log dump process is happening, then it will get the size of compressed logs till that point.

apps/examples/log_dump/log_dump_main.c

kishore-sn · 2024-09-12T08:40:57Z

os/kernel/log_dump/log_dump.c

+	 * is compressed and size should include last_comp_block_size */
+	if (is_started_to_save == false) {
+		return log_dump_chunk_node_cnt * CONFIG_LOG_DUMP_CHUNK_SIZE + compress_curbytes + last_comp_block_size;
+	} else {


The else case means, top_logsave has not been called. So, if we return some size here and there is some timegap before read, then the size will differ. It is better to return error code here. This api must be called only when log dump has been stopped.

Here you added the else case to check stop first. How about adding it at the read function as well? Because it's the same.

apps/examples/log_dump/log_dump_main.c

kishore-sn · 2024-09-12T09:47:37Z

@ritesh55555 Pls add the output of the log dump app in the commit description.

apps/examples/log_dump/log_dump_main.c

os/fs/procfs/fs_procfslogsave.c

os/kernel/log_dump/log_dump.c

- This commit adds an api to get the size of the compressed log dump. It also places the work of compressing the partially filled log buffer when log save is stopped.

- This commit add code in log_dump_main app to verify working of log_dump_get_size() api. It calculates total length of compressed logs by continuously reading throught a fixed buffer and then verify the size with the value returned from log_dump_get_size() api. followings are the logs from log_dump_main app: (Note the compressed output is commented because it gives garbage value , we are only printing to check log dump size comparison with total read value from buffer) TASH>>log_dump TASH>>This Log Should NOT be saved!!! ********************* LOG DUMP START ********************* This Log Should be saved!!! log_dump_size = 2892, and total_read = 2892, Both should be equal ********************* LOG DUMP END ***********************

sunghan-chang · 2024-09-13T10:09:17Z

os/kernel/log_dump/log_dump.c

+			/* Should we return error from here?? because partially filled uncompressed buffer
+			 * might not be compressed but log_dump save has stopped now, so operation is success */
+		}
+	} else if (strncmp(buffer, LOGDUMP_GET_SIZE, strlen(LOGDUMP_GET_SIZE) + 1) == 0) {


There are 3 strncmp s to check a command. It looks not good.
Because the data is a just character, how about using switch / case?

sunghan-chang · 2024-09-13T10:11:01Z

os/kernel/log_dump/log_dump.c

+	 * is compressed and size should include last_comp_block_size */
+	if (is_started_to_save == false) {
+		return log_dump_chunk_node_cnt * CONFIG_LOG_DUMP_CHUNK_SIZE + compress_curbytes + last_comp_block_size;
+	} else {


Here you added the else case to check stop first. How about adding it at the read function as well? Because it's the same.

sunghan-chang · 2024-09-13T10:12:18Z

@ritesh55555 I left two comments, but I think it's different topic against this PR so that you can do that with new PR or new commit.

sunghan-chang reviewed Sep 11, 2024

View reviewed changes

ritesh55555 force-pushed the log_dump_size branch from bfaf74a to 2e37810 Compare September 11, 2024 12:23

ritesh55555 changed the title ~~log_dump/log_dump.c: Add api to get size of the log dump~~ log_dump: Add api to get size of the log dump Sep 11, 2024

sunghan-chang reviewed Sep 11, 2024

View reviewed changes

ewoodev reviewed Sep 11, 2024

View reviewed changes

apps/examples/log_dump/log_dump_main.c Outdated Show resolved Hide resolved

ritesh55555 force-pushed the log_dump_size branch from 2e37810 to ee10c01 Compare September 12, 2024 07:33

ritesh55555 force-pushed the log_dump_size branch 2 times, most recently from 8046bc3 to aa3b9f9 Compare September 12, 2024 07:59

kishore-sn reviewed Sep 12, 2024

View reviewed changes

ritesh55555 force-pushed the log_dump_size branch 2 times, most recently from 2e567e6 to 10cd880 Compare September 12, 2024 09:05

kishore-sn reviewed Sep 12, 2024

View reviewed changes

apps/examples/log_dump/log_dump_main.c Outdated Show resolved Hide resolved

ritesh55555 force-pushed the log_dump_size branch from 10cd880 to 4f2fb0a Compare September 12, 2024 09:29

kishore-sn previously approved these changes Sep 12, 2024

View reviewed changes

Tirtha-Samsung reviewed Sep 12, 2024

View reviewed changes

apps/examples/log_dump/log_dump_main.c Show resolved Hide resolved

ritesh55555 dismissed kishore-sn’s stale review via 4622c3c September 12, 2024 10:05

ritesh55555 force-pushed the log_dump_size branch from 4f2fb0a to 4622c3c Compare September 12, 2024 10:05

sunghan-chang reviewed Sep 12, 2024

View reviewed changes

apps/examples/log_dump/log_dump_main.c Show resolved Hide resolved

os/fs/procfs/fs_procfslogsave.c Show resolved Hide resolved

os/kernel/log_dump/log_dump.c Outdated Show resolved Hide resolved

os/kernel/log_dump/log_dump.c Show resolved Hide resolved

log_dump: Add api to get size of the log dump

6e1161e

- This commit adds an api to get the size of the compressed log dump. It also places the work of compressing the partially filled log buffer when log save is stopped.

ritesh55555 force-pushed the log_dump_size branch from 4622c3c to 6e1161e Compare September 12, 2024 11:33

sunghan-chang reviewed Sep 13, 2024

View reviewed changes

kishore-sn mentioned this pull request Sep 16, 2024

Fix issues in Logdump module #6400

Open

kishore-sn approved these changes Sep 16, 2024

View reviewed changes

sunghan-chang approved these changes Sep 16, 2024

View reviewed changes

kishore-sn merged commit 2f722e6 into Samsung:master Sep 17, 2024
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

log_dump: Add api to get size of the log dump #6394

log_dump: Add api to get size of the log dump #6394

ritesh55555 commented Sep 11, 2024

sunghan-chang left a comment

sunghan-chang Sep 11, 2024

sunghan-chang Sep 11, 2024

sunghan-chang Sep 11, 2024

sunghan-chang Sep 11, 2024

ritesh55555 Sep 12, 2024

sunghan-chang commented Sep 11, 2024

ritesh55555 commented Sep 12, 2024

kishore-sn Sep 12, 2024

ritesh55555 Sep 12, 2024

sunghan-chang Sep 13, 2024

kishore-sn commented Sep 12, 2024

sunghan-chang Sep 13, 2024

sunghan-chang Sep 13, 2024

sunghan-chang commented Sep 13, 2024

		* Note: It is recommended to use this log_dump_get_size api after
		* log_dump_read_wake() is performed, so that it has correct value of last_comp_block_size.

log_dump: Add api to get size of the log dump #6394

log_dump: Add api to get size of the log dump #6394

Conversation

ritesh55555 commented Sep 11, 2024

sunghan-chang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunghan-chang commented Sep 11, 2024

ritesh55555 commented Sep 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kishore-sn commented Sep 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunghan-chang commented Sep 13, 2024