Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement a log buffer to store log messages. #52

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

vsntk18
Copy link
Collaborator

@vsntk18 vsntk18 commented Jun 19, 2023

Implemented a global buffer to store log messages and passed this buffer from stage2 to svsm kernel. Replaced console logging with buffer logging.

Percpu line buffer will probably be necessary, and is not yet implemented. As a rust newbee, I appreciate your feedback on if I am using the right data structures in the right way. Thanks!

@00xc
Copy link
Member

00xc commented Jun 20, 2023

Probably a good idea to rebase from the main branch to fix the cargo fmt checks. Also, make sure your author email matches your Signed-off-by email!

EDIT: nevermind, no need for a rebase, but the fmt checks should be fixed.

@vsntk18
Copy link
Collaborator Author

vsntk18 commented Jun 20, 2023

Probably a good idea to rebase from the main branch to fix the cargo fmt checks. Also, make sure your author email matches your Signed-off-by email!

EDIT: nevermind, no need for a rebase, but the fmt checks should be fixed.

sorry, will fix the fmt checks. I was under the impression that commit hook reports the deviations.

src/log_buffer.rs Outdated Show resolved Hide resolved
src/log_buffer.rs Outdated Show resolved Hide resolved
@00xc
Copy link
Member

00xc commented Jun 20, 2023

sorry, will fix the fmt checks. I was under the impression that commit hook reports the deviations.

The git hook should warn you, but won't fix the issues automatically.

src/svsm.rs Outdated Show resolved Hide resolved
@vsntk18 vsntk18 force-pushed the svsm_log_buffer branch 5 times, most recently from 26e9a9f to 5648746 Compare June 23, 2023 19:03
@vsntk18
Copy link
Collaborator Author

vsntk18 commented Jun 23, 2023

Reworked the findings so far.
Added percpu line buffer implementation.

@vsntk18 vsntk18 force-pushed the svsm_log_buffer branch 3 times, most recently from 87b4c26 to fc7ff02 Compare June 29, 2023 19:47
@vsntk18
Copy link
Collaborator Author

vsntk18 commented Jun 29, 2023

Status so far on this PR:

  1. Implemented a global log buffer
  2. Added initialization and migration code for the log buffer.
  3. Implemented a percpu line buffer
  4. Added a feature called enable-console-log to print log messages to console.

@vsntk18 vsntk18 force-pushed the svsm_log_buffer branch 3 times, most recently from c51a833 to 97c017b Compare July 4, 2023 11:19
Copy link
Member

@joergroedel joergroedel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Provided some comments in the reviews. Please also move the logbuffer specific code into a separate sub-directory/module.

src/line_buffer.rs Outdated Show resolved Hide resolved
src/line_buffer.rs Outdated Show resolved Hide resolved
src/log_buffer.rs Outdated Show resolved Hide resolved
src/line_buffer.rs Outdated Show resolved Hide resolved
@vsntk18
Copy link
Collaborator Author

vsntk18 commented Jul 11, 2023

Provided some comments in the reviews. Please also move the logbuffer specific code into a separate sub-directory/module.

@vsntk18 vsntk18 closed this Jul 11, 2023
@vsntk18 vsntk18 reopened this Jul 11, 2023
@vsntk18
Copy link
Collaborator Author

vsntk18 commented Jul 11, 2023

In this PR:

  1. Moved log buffer implementation to separate directory.
  2. Moved line buffer implementation to src/cpu directory.
  3. Reworked line buffer to use len member instead of head and tail. However kept the implementation as u8 array not as a FixedString, since it seemed simple. May be this is still arguable.
  4. skipped all the intermediate utf8 checking as suggested by Carlos.

src/cpu/line_buffer.rs Outdated Show resolved Hide resolved
src/string.rs Outdated Show resolved Hide resolved
src/string.rs Outdated Show resolved Hide resolved
@joergroedel joergroedel self-assigned this Feb 16, 2024
@joergroedel joergroedel added needs-rebase The PR needs to be rebased to the latest upstream branch wait-for-review PR needs for approval by reviewers and removed wait-for-review PR needs for approval by reviewers labels Feb 16, 2024
@vsntk18 vsntk18 force-pushed the svsm_log_buffer branch 2 times, most recently from ecd3216 to 8fce96e Compare February 19, 2024 11:02
@joergroedel joergroedel removed the needs-rebase The PR needs to be rebased to the latest upstream branch label Feb 26, 2024
@00xc 00xc added the needs-rebase The PR needs to be rebased to the latest upstream branch label May 30, 2024
@vsntk18 vsntk18 force-pushed the svsm_log_buffer branch 4 times, most recently from 7476cee to 3d2c06d Compare June 6, 2024 17:17
@vsntk18
Copy link
Collaborator Author

vsntk18 commented Jun 6, 2024

Rebased the PR to the latest HEAD.

@joergroedel joergroedel removed the needs-rebase The PR needs to be rebased to the latest upstream branch label Jun 7, 2024
@vsntk18
Copy link
Collaborator Author

vsntk18 commented Jun 10, 2024

Needs rebase due to percpu related changes in 7e5a9a7

@vsntk18 vsntk18 force-pushed the svsm_log_buffer branch 4 times, most recently from 37851c2 to 4e8cd5e Compare June 17, 2024 07:59
}

pub fn write_log(&mut self, s: &FixedString<LINE_BUFFER_SIZE>) {
self.buf.write(&s.to_string());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This creates an intermediate allocation. Perhaps write() should take a generic char iterator which it can consume.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed write() interface to take char iterator as a parameter.

Comment on lines 51 to 50
// A method used for testing
pub fn read_log(&mut self) -> Vec<u8> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's only used for testing gate it behind #[cfg(test)] to avoid unused warnings.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done!

Comment on lines 41 to 43
pub unsafe fn migrate(&mut self, lb: *const SpinLock<LogBuffer>) {
unsafe {
self.buf = (*lb).lock().buf;
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not pass a regular reference to the spinlock?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, some compilation problems made me use that. But I have changed to use a regular reference now.

}
}

static mut LB: SpinLock<LogBuffer> = SpinLock::new(LogBuffer::new());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does it need to be mut?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not necessary. Removed it. Thanks!

Comment on lines 86 to 93
#[cfg(test)]
const BUF_SIZE: usize = 64;

#[cfg(test)]
use crate::types::LINE_BUFFER_SIZE;

#[test]
fn test_read_write_normal() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of these should be in a test module, e.g.

#[cfg(test)]
mod test {
    use crate::types::LINE_BUFFER_SIZE;

    const BUF_SIZE: usize = 64;

    #[test]
    fn test_read_write_normal() {
        // ...
    }
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done!

//
// Copyright (c) 2022-2024 SUSE LLC
//
// Author: Roy Hopkins <[email protected]>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably forgot to update author name :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, actually Roy created this file in one of the branches. I cherry picked it to my branch. So I will let it stay. :)

Comment on lines 83 to 84
#[test]
fn test_ring_one_string() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test and the ones below should also be in a test module.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done!


fn log(&self, record: &log::Record<'_>) {
let comp: &'static str = self.component;
let line_buf: &mut LineBuffer = &mut this_cpu().get_line_buffer();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no need to add the type here, RefMut implements Deref and DerefMut, so access to the LineBuffer methods is transparent.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed!

pub fn install_buffer_logger(component: &'static str) {
BUFFER_LOGGER
.init(&BufferLogger::new(component))
.expect("already initialized the logger");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say return the error here and panic at the caller, just like we had before.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done!

@00xc 00xc added the needs-rebase The PR needs to be rebased to the latest upstream branch label Jul 12, 2024
The log buffer is implemented using a ring buffer that contains the
characters within log entries. This patch introduces a generic
StringRingBuffer that can be used for safely managing the contents of
strings within a ring buffer that is suitable for use with the log
buffer. This separates the ring buffer implementation from the log and
allows implementation of ring buffer specific unit testing, which is
included within this patch.

Signed-off-by: Vasant Karasulli <[email protected]>
Signed-off-by: Roy Hopkins <[email protected]>
This log buffer is based on the string ring buffer in
utils/StringRingBuffer.

Signed-off-by: Vasant Karasulli <[email protected]>
Each individual log message gets written into the global log buffer
from the percpu line buffer.

Signed-off-by: Vasant Karasulli <[email protected]>
Introduce a new struct MigrateInfo which contains the data to
be migrated from stage2 to svsm kernel.

Signed-off-by: Vasant Karasulli <[email protected]>
This feature can be used to enable printing log messages to
console in addition to storing them in the log buffer.

Use ENABLE_CONSOLE_LOG=1 to enable this feature. By default,
this feature is disabled for release builds and enabled for
debug builds.

Signed-off-by: Vasant Karasulli <[email protected]>
Test the log_buffer by calling methods like write_log()
and read_log().

Signed-off-by: Vasant Karasulli <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-rebase The PR needs to be rebased to the latest upstream branch wait-for-review PR needs for approval by reviewers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants