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

Static scheduler #241

Draft
wants to merge 109 commits into
base: main
Choose a base branch
from
Draft

Static scheduler #241

wants to merge 109 commits into from

Conversation

lsk567
Copy link

@lsk567 lsk567 commented Jun 26, 2023

This is draft PR for tracking progress on a static scheduler corresponding to #1830.

Summary by CodeRabbit

  • New Features

    • Introduced a static scheduler for the threaded runtime that optimizes performance and scheduling efficiency.
    • Added circular buffer functionality to enhance data management.
  • Improvements

    • Enhanced scheduling logic with conditional compilation for optimizing reaction chains.
    • Updated tracepoints and buffer capacity to improve tracing granularity and performance.
  • Code Quality

    • Updated function names and logic to improve code readability and maintainability.
    • Conditional macros based on the scheduler setting for more flexible code behavior.
  • Build System

    • Expanded source lists in CMakeLists.txt to include new scheduler and utility files.
  • Performance

    • Optimized busy-wait loops and sleep functions to reduce unnecessary delays.

lsk567 and others added 30 commits June 26, 2023 17:15
FS: Add reactor local time and remove chain optimization
…nd-clear to sync-advance-clear to advance time at the end of the hyperperiod
Copy link
Contributor

coderabbitai bot commented Jul 3, 2024

Walkthrough

The changes span multiple core components of the system, with key modifications including the introduction of busy-wait loops in timing functions, new static scheduling logic, and the addition of a circular buffer utility. Enhancements also involve conditional compilation directives for optimization, updates in tracepoint management for better granularity, and relevant infrastructural changes in CMakeLists.txt files to integrate the new functionalities.

Changes

Files/Paths Change Summary
core/clock.c, core/reactor.c Modified timing functions to include busy-wait loops.
core/lf_token.c Updated function name from _lf_get_token to _lf_get_new_token.
core/reactor_common.c Introduced conditional compilation directives for reaction chain optimization.
core/threaded/CMakeLists.txt Added scheduler_static.c to threaded sources.
core/utils/CMakeLists.txt Added circular_buffer.c to utility sources.
core/utils/circular_buffer.c, Implemented circular buffer functions for initialization, memory management, and element operations.
include/api/reaction_macros.h Added conditional macros based on SCHEDULER settings and local timestamp access macros.
include/core/environment.h Added declarations for static scheduler-related arrays.
include/core/lf_types.h Introduced SCHED_STATIC type and handling logic for reactor local time.
include/core/reactor.h Included circular buffer header.
include/core/reactor_common.h Added macro definition REACTION_CHAIN_OPTIMIZATION based on SCHEDULER.
include/core/threaded/scheduler_... Conditional compilation for SCHED_STATIC and new static scheduler function declarations.
include/core/threaded/scheduler_... Defined the instruction set for PRET VM in C platforms.
include/core/trace.c Modified tracepoint functions and added new static scheduling instruction tracepoints.
include/core/trace.h Added trace functions for static scheduler instructions and increased trace buffer capacity.
test/CMakeLists.txt Replaced STATIC with SCHED_STATIC in add_library declaration.
util/tracing/trace_to_chrome.c Added scaling_factor for Chrome tracing events and mapped event types to instruction names.

Sequence Diagram(s)

sequenceDiagram
    actor Developer
    participant Core Clock as core/clock.c
    participant Reactor as core/reactor.c
    participant SchedulerAdded as include/core/threaded/scheduler_static_functions.h
    
    Developer->>Core Clock: Call lf_clock_cond_timedwait()
    Core Clock->>SchedulerAdded: Busy-wait until wakeup_time
    SchedulerAdded-->>Core Clock: Return true
    Core Clock-->>Developer: Returns true
    
    Developer->>Reactor: Call wait_until()
    Reactor->>Reactor: Busy-wait until wakeup_time
    Reactor-->>Developer: Returns 0
Loading
sequenceDiagram
    actor Developer
    participant ReactionCommon as core/reactor_common.c
    participant OptimizedScheduler as Scheduler with REACTION_CHAIN_OPTIMIZATION

    Developer->>ReactionCommon: Call schedule_output_reactions()
    alt REACTION_CHAIN_OPTIMIZATION Defined
        ReactionCommon->>OptimizedScheduler: Check downstream reaction conditions
        OptimizedScheduler->>OptimizedScheduler: Execute _lf_trigger_reaction for single downstream reaction
    else REACTION_CHAIN_OPTIMIZATION Not Defined
        ReactionCommon->>OptimizedScheduler: Directly queue reaction via _lf_trigger_reaction
    end
    OptimizedScheduler-->>Developer: Reaction scheduled
Loading

Tip

AI model upgrade

gpt-4o model for reviews and chat is now live

OpenAI claims that this model is better at understanding and generating code than the previous models. Please join our Discord Community to provide any feedback or to report any issues.


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@lsk567
Copy link
Author

lsk567 commented Jul 3, 2024

@coderabbitai review

Copy link
Contributor

coderabbitai bot commented Jul 3, 2024

Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 8

Outside diff range and nitpick comments (13)
include/core/utils/circular_buffer.h (2)

1-2: Include a brief description and author information.

Add a brief description and author information to the header file.

+#ifndef CIRCULAR_BUFFER_H
+#define CIRCULAR_BUFFER_H
+/**
+ * @file circular_buffer.h
+ * @brief Circular buffer data structure and function declarations.
+ * @author Your Name
+ */

18-24: Add documentation for each function.

Provide detailed documentation for each function, including parameters and return values.

+/**
+ * @brief Initialize the circular buffer.
+ * @param cb Pointer to the circular buffer.
+ * @param capacity Maximum number of items in the buffer.
+ * @param sz Size of each item in the buffer.
+ */
 void cb_init(circular_buffer *cb, size_t capacity, size_t sz);
 
+/**
+ * @brief Free the circular buffer.
+ * @param cb Pointer to the circular buffer.
+ */
 void cb_free(circular_buffer *cb);
 
+/**
+ * @brief Push an item to the back of the circular buffer.
+ * @param cb Pointer to the circular buffer.
+ * @param item Pointer to the item to be added.
+ */
 void cb_push_back(circular_buffer *cb, const void *item);
 
+/**
+ * @brief Pop an item from the front of the circular buffer.
+ * @param cb Pointer to the circular buffer.
+ * @param item Pointer to the item to be removed.
+ */
 void cb_pop_front(circular_buffer *cb, void *item);
 
+/**
+ * @brief Remove an item from the front of the circular buffer.
+ * @param cb Pointer to the circular buffer.
+ */
 void cb_remove_front(circular_buffer *cb);
 
+/**
+ * @brief Peek at the item at the front of the circular buffer.
+ * @param cb Pointer to the circular buffer.
+ * @return Pointer to the item at the front of the buffer.
+ */
 void* cb_peek(circular_buffer *cb);
 
+/**
+ * @brief Dump the events in the circular buffer.
+ * @param cb Pointer to the circular buffer.
+ */
 void cb_dump_events(circular_buffer *cb);
include/core/threaded/scheduler_instructions.h (3)

1-4: Include a brief description and author information.

Add a brief description and author information to the header file.

+#ifndef SCHEDULER_INSTRUCTIONS_H
+#define SCHEDULER_INSTRUCTIONS_H
+/**
+ * @file scheduler_instructions.h
+ * @brief Format of the instruction set for the PRET VM in C platforms.
+ * @author Shaokai Lin
+ */

43-46: Add documentation for the operand_t union.

Provide detailed documentation for the operand_t union, including members and usage.

+/**
+ * @brief A union representing a single operand for the PRET VM.
+ * @details This union can store either a register pointer or an immediate value.
+ */
 typedef union {
     reg_t* reg;
     imm_t imm;
 } operand_t;

51-60: Add documentation for the function pointer type.

Provide detailed documentation for the function_virtual_instruction_t function pointer type, including parameters and return values.

+/**
+ * @brief Virtual instruction function pointer.
+ * @param scheduler Pointer to the scheduler.
+ * @param worker_number The worker number.
+ * @param op1 First operand.
+ * @param op2 Second operand.
+ * @param op3 Third operand.
+ * @param debug Debug flag.
+ * @param pc Program counter.
+ * @param returned_reaction Pointer to the returned reaction.
+ * @param exit_loop Flag to indicate if the loop should exit.
+ */
 typedef void (*function_virtual_instruction_t)(
     lf_scheduler_t* scheduler, 
     size_t worker_number, 
     operand_t op1, 
     operand_t op2, 
     operand_t op3, 
     bool debug, 
     size_t* pc, 
     reaction_t** returned_reaction, 
     bool* exit_loop);
core/utils/circular_buffer.c (3)

36-46: Improve error message in cb_push_back.

The error message for a full buffer could be more informative by including the buffer's capacity.

void cb_push_back(circular_buffer *cb, const void *item)
{
    if(cb->count == cb->capacity){
-        lf_print("ERROR: Buffer is full. Some in-flight events will be overwritten!");
+        lf_print("ERROR: Buffer is full. Capacity: %zu. Some in-flight events will be overwritten!", cb->capacity);
    }
    memcpy(cb->head, item, cb->sz);
    cb->head = (char*)cb->head + cb->sz;
    if(cb->head == cb->buffer_end)
        cb->head = cb->buffer;
    cb->count++;
}

48-60: Improve error message in cb_pop_front.

The error message for an empty buffer could be more informative by including the buffer's current count.

void cb_pop_front(circular_buffer *cb, void *item)
{
    if(cb->count == 0){
        // handle error
-        lf_print("ERROR: Popping from an empty buffer!");
+        lf_print("ERROR: Popping from an empty buffer! Current count: %zu.", cb->count);
        return;
    }
    memcpy(item, cb->tail, cb->sz);
    cb->tail = (char*)cb->tail + cb->sz;
    if(cb->tail == cb->buffer_end)
        cb->tail = cb->buffer;
    cb->count--;
}

62-73: Improve error message in cb_remove_front.

The error message for an empty buffer could be more informative by including the buffer's current count.

void cb_remove_front(circular_buffer *cb)
{
    if(cb->count == 0){
        // handle error
-        lf_print("ERROR: Removing from an empty buffer!");
+        lf_print("ERROR: Removing from an empty buffer! Current count: %zu.", cb->count);
        return;
    }
    cb->tail = (char*)cb->tail + cb->sz;
    if(cb->tail == cb->buffer_end)
        cb->tail = cb->buffer;
    cb->count--;
}
core/threaded/scheduler_static.c (2)

3-25: Ensure consistency in copyright notices.

The copyright notices for the University of Texas at Dallas and the University of California at Berkeley should be consistent with other files in the repository.


482-484: Remove unused variable opcode.

The variable opcode is unused in the lf_sched_get_ready_reaction function. Removing it will improve code readability and maintainability.

- opcode = current_schedule[*pc].opcode; // FIXME: Opcode is unused.
util/tracing/trace_to_chrome.c (2)

54-62: Consider removing commented-out code.

The commented-out scaling_factor variable may be unnecessary. If it is not intended for future use, consider removing it to improve code clarity and maintainability.


80-148: Return a constant string for unrecognized event types.

In the default case of the switch statement, consider returning a constant string (e.g., const char* unknown = "UNKNOWN"; return unknown;) instead of a literal string to avoid potential issues with string literals.

core/reactor_common.c (1)

872-873: Avoid hacky solutions.

The comment indicates that the current solution is a temporary fix. Consider refactoring to implement a more robust solution.

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 7427d98 and 44313a2.

Files selected for processing (21)
  • core/clock.c (1 hunks)
  • core/lf_token.c (1 hunks)
  • core/reactor.c (1 hunks)
  • core/reactor_common.c (2 hunks)
  • core/threaded/CMakeLists.txt (1 hunks)
  • core/threaded/scheduler_static.c (1 hunks)
  • core/trace.c (4 hunks)
  • core/utils/CMakeLists.txt (1 hunks)
  • core/utils/circular_buffer.c (1 hunks)
  • include/api/reaction_macros.h (4 hunks)
  • include/core/environment.h (1 hunks)
  • include/core/lf_types.h (2 hunks)
  • include/core/reactor.h (1 hunks)
  • include/core/reactor_common.h (1 hunks)
  • include/core/threaded/scheduler_instance.h (4 hunks)
  • include/core/threaded/scheduler_instructions.h (1 hunks)
  • include/core/threaded/scheduler_static_functions.h (1 hunks)
  • include/core/trace.h (5 hunks)
  • include/core/utils/circular_buffer.h (1 hunks)
  • test/CMakeLists.txt (1 hunks)
  • util/tracing/trace_to_chrome.c (6 hunks)
Files not reviewed due to errors (1)
  • include/api/reaction_macros.h (no review received)
Files skipped from review due to trivial changes (3)
  • core/threaded/CMakeLists.txt
  • include/core/reactor.h
  • include/core/reactor_common.h
Additional comments not posted (31)
test/CMakeLists.txt (1)

1-1: Verify the definition and usage of SCHED_STATIC.

Ensure that SCHED_STATIC is defined and used correctly in the build process.

Verification successful

SCHED_STATIC is correctly defined and used in the codebase.

The macro SCHED_STATIC is defined in include/core/lf_types.h and is used in various files for conditional compilation, ensuring its proper integration in the build process.

  • Definition: include/core/lf_types.h
  • Usage: Multiple files including include/core/reactor_common.h, include/core/environment.h, include/api/reaction_macros.h, core/trace.c, and others.
Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify the definition and usage of SCHED_STATIC in the codebase.

# Test: Search for the definition of SCHED_STATIC. Expect: A valid definition.
rg --type-add 'cmake' --type 'cmake' $'SCHED_STATIC'

Length of output: 107



Script:

#!/bin/bash
# Description: Verify the definition and usage of SCHED_STATIC in the codebase.

# Test: Search for the definition of SCHED_STATIC. Expect: A valid definition.
rg 'SCHED_STATIC'

Length of output: 1173

core/utils/CMakeLists.txt (1)

1-1: Verify the implementation and integration of circular_buffer.c.

Ensure that circular_buffer.c is correctly implemented and integrated into the build process.

Verification successful

circular_buffer.c is correctly implemented and integrated into the build process.

  • The file core/utils/circular_buffer.c contains a valid implementation of a circular buffer.
  • The file is referenced in core/utils/CMakeLists.txt, ensuring it is included in the build process.
Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify the implementation and integration of circular_buffer.c in the codebase.

# Test: Search for the implementation of circular_buffer.c. Expect: A valid implementation file.
fd 'circular_buffer.c' --type f

Length of output: 60



Script:

#!/bin/bash
# Description: Verify the implementation and integration of circular_buffer.c in the codebase.

# Test: Check the implementation of circular_buffer.c
echo "Implementation of circular_buffer.c:"
cat core/utils/circular_buffer.c

# Test: Verify if circular_buffer.c is included in the build process
echo "References to circular_buffer.c in CMakeLists.txt:"
rg 'circular_buffer.c' --glob '**/CMakeLists.txt'

Length of output: 2702

core/clock.c (1)

58-60: Verify the necessity and performance implications of the busy-wait loop.

The busy-wait loop might have performance implications. Ensure that it is necessary and does not introduce performance issues.

core/utils/circular_buffer.c (2)

75-80: LGTM!

The function correctly handles the case when the buffer is empty.


82-93: LGTM!

The function correctly handles the buffer wrap-around and prints the events.

include/core/threaded/scheduler_static_functions.h (12)

15-15: LGTM!

The function prototype seems correct.


17-18: LGTM!

The function prototype seems correct.


19-20: LGTM!

The function prototype seems correct.


21-22: LGTM!

The function prototype seems correct.


23-24: LGTM!

The function prototype seems correct.


25-26: LGTM!

The function prototype seems correct.


27-28: LGTM!

The function prototype seems correct.


29-30: LGTM!

The function prototype seems correct.


31-32: LGTM!

The function prototype seems correct.


33-34: LGTM!

The function prototype seems correct.


35-36: LGTM!

The function prototype seems correct.


37-46: LGTM!

The function prototypes seem correct.

include/core/environment.h (1)

113-120: Verify the usage of new members in environment_t.

The new members related to static scheduling seem correct. However, ensure that their usage is consistent across the codebase.

include/core/threaded/scheduler_instance.h (2)

48-54: LGTM!

The forward declarations and includes for the static scheduler seem correct.


152-190: Verify the usage of new members in lf_scheduler_t.

The new members related to static scheduling seem correct. However, ensure that their usage is consistent across the codebase.

Verification successful

Usage of new members in lf_scheduler_t verified.

The new members related to static scheduling have been correctly integrated and are consistently used across the codebase.

  • pc is used in multiple functions within core/threaded/scheduler_static.c and include/core/threaded/scheduler_static_functions.h.
  • static_schedules, reactor_self_instances, num_reactor_self_instances, reaction_instances, and counters are referenced and utilized appropriately in the codebase.
Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify the usage of new members in `lf_scheduler_t`.

# Test: Search for the new members in the codebase. Expect: Consistent usage.
rg --type-add 'c:h' --type-add 'c:cpp' --type-add 'c:cc' --type c 'pc|static_schedules|reactor_self_instances|num_reactor_self_instances|reaction_instances|counters'

Length of output: 29444

include/core/lf_types.h (2)

49-56: LGTM!

The changes to define SCHED_STATIC and conditionally define REACTOR_LOCAL_TIME based on the SCHEDULER value are straightforward and align with the introduction of a static scheduler.


296-302: LGTM!

The changes to add members to the self_base_t struct for supporting the static scheduler are straightforward and align with the introduction of a static scheduler.

include/core/trace.h (5)

79-109: Addition of new trace event types for static scheduler instructions.

The new trace event types for static scheduler instructions are well-defined and follow the existing naming conventions.


175-205: Addition of string representations for new trace event types.

The string representations for the new static scheduler instructions match the new trace event types and are appropriately named.


255-256: Verify the necessity and impact of increasing the trace buffer capacity.

The trace buffer capacity has been increased significantly from 2048 to 131072. Ensure that this change is necessary and that it does not negatively impact memory usage or performance.


500-533: Addition of new tracepoint function declarations for static scheduler instructions.

The new tracepoint function declarations for static scheduler instructions are consistent with the existing function naming conventions and parameter types.


640-671: Addition of new tracepoint function definitions for static scheduler instructions when tracing is disabled.

The new tracepoint function definitions for static scheduler instructions are consistent with the existing function definitions for disabled tracing.

util/tracing/trace_to_chrome.c (2)

177-183: LGTM!

The code correctly handles static_scheduler_* event types, mapping them to instruction names and formatting them for output.


282-401: LGTM!

The code correctly handles static_scheduler_* event types, setting the appropriate pid and phase values for each case.

core/trace.c (1)

464-634: LGTM!

The new functions for tracing static scheduler instructions are consistent with existing tracing functions and correctly set the event type and invoke the tracepoint function with appropriate arguments.

core/reactor_common.c (1)

Line range hint 873-905: Ensure correctness of reaction chain optimization.

The new conditional compilation directive introduces a new logic branch for reaction chain optimization. Verify that the logic is correct and does not introduce any issues.

Verification successful

Reaction chain optimization logic is correct.

The introduced logic for reaction chain optimization correctly handles the queuing and execution of downstream reactions. The _lf_trigger_reaction function ensures proper queuing, and the conditions for immediate execution based on last_enabling_reaction are correctly implemented.

  • core/reactor_common.c: Lines 899-908
Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify the correctness of the new reaction chain optimization logic.

# Test: Search for the usage of REACTION_CHAIN_OPTIMIZATION. Expect: Correct logic implementation.
rg --type c 'REACTION_CHAIN_OPTIMIZATION'

Length of output: 169



Script:

#!/bin/bash
# Search for the definition and usage of `_lf_trigger_reaction` in the codebase.
rg '_lf_trigger_reaction'

# Search for the definition and usage of `last_enabling_reaction` in the codebase.
rg 'last_enabling_reaction'

Length of output: 1841



Script:

#!/bin/bash
# Retrieve the implementation of `_lf_trigger_reaction` from core/reactor.c
ast-grep --lang c --pattern 'void _lf_trigger_reaction(environment_t* env, reaction_t* reaction, int worker_number) {
  $$$
}' core/reactor.c

# Search for the usage of `last_enabling_reaction` within core/reactor_common.c to understand its context.
rg 'last_enabling_reaction' core/reactor_common.c

Length of output: 1573

Comment on lines +222 to +225
// FIXME (Shaokai): The function name is probably a misnomer.
// It should better be _lf_get_new_token.
// I also don't get why the token isn't simply returned, and
// _lf_free_token_value or _lf_done_using has to be called.
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider renaming _lf_get_token to _lf_get_new_token and clarify the function's logic.

The comments suggest that the function name _lf_get_token is a misnomer and should be renamed to _lf_get_new_token. Additionally, the logic of calling _lf_free_token_value or _lf_done_using should be clarified.

Would you like assistance in refactoring the function name and clarifying the logic?

core/reactor.c Outdated
Comment on lines 69 to 71
// return lf_clock_interruptable_sleep_until_locked(env, wakeup_time);
while (lf_time_physical() < wakeup_time);
return 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid using a busy-wait loop.

The busy-wait loop in the wait_until function can lead to high CPU usage. It is recommended to retain the original function call to lf_clock_interruptable_sleep_until_locked or replace it with a more efficient waiting mechanism.

- while (lf_time_physical() < wakeup_time);
+ return lf_clock_interruptable_sleep_until_locked(env, wakeup_time);
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// return lf_clock_interruptable_sleep_until_locked(env, wakeup_time);
while (lf_time_physical() < wakeup_time);
return 0;
return lf_clock_interruptable_sleep_until_locked(env, wakeup_time);

Comment on lines +103 to +104
// FIXME: Will there be problems if instant_t adds reg_t?
*dst = *src + op3.imm;
Copy link
Contributor

Choose a reason for hiding this comment

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

Clarify the FIXME comment regarding instant_t and reg_t.

The FIXME comment questions whether there will be problems if instant_t adds reg_t. This should be clarified and addressed to ensure correct functionality.

Would you like assistance in addressing this FIXME comment?

Comment on lines 251 to 267
// FIXME: There seems to be an overflow problem.
// When wakeup_time overflows but lf_time_physical() doesn't,
// _lf_interruptable_sleep_until_locked() terminates immediately.
reg_t *src = op1.reg;
instant_t current_time = lf_time_physical();
instant_t wakeup_time = *src + op2.imm;
instant_t wait_interval = wakeup_time - current_time;
// LF_PRINT_DEBUG("*** start_time: %lld, wakeup_time: %lld, op1: %lld, op2: %lld, current_physical_time: %lld\n", start_time, wakeup_time, *src, op2.imm, lf_time_physical());
LF_PRINT_DEBUG("*** [Line %zu] Worker %zu delaying, current_physical_time: %lld, wakeup_time: %lld, wait_interval: %lld", *pc, worker_number, current_time, wakeup_time, wait_interval);
if (wait_interval > 0) {
// if (wait_interval < SPIN_WAIT_THRESHOLD) {
// // Spin wait if the wait interval is less than 1 ms.
// while (lf_time_physical() < wakeup_time);
// } else {
// // Otherwise sleep.
// _lf_interruptable_sleep_until_locked(scheduler->env, wakeup_time);
// }
Copy link
Contributor

Choose a reason for hiding this comment

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

Address the FIXME comment regarding overflow in execute_inst_DU.

The FIXME comment indicates a potential overflow problem when wakeup_time overflows but lf_time_physical() doesn't. This should be addressed to ensure correct functionality.

Would you like assistance in addressing this FIXME comment?

Comment on lines +9 to +15
void *buffer; // data buffer
void *buffer_end; // end of data buffer
size_t capacity; // maximum number of items in the buffer
size_t count; // number of items in the buffer
size_t sz; // size of each item in the buffer
void *head; // pointer to head
void *tail; // pointer to tail
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure proper memory alignment and handling.

Ensure that the buffer memory is properly aligned and handled to avoid potential issues.

+    void *buffer;     // data buffer
+    void *buffer_end; // end of data buffer
+    size_t capacity;  // maximum number of items in the buffer
+    size_t count;     // number of items in the buffer
+    size_t sz;        // size of each item in the buffer
+    void *head;       // pointer to head
+    void *tail;       // pointer to tail
+    // Ensure proper memory alignment
+    alignas(alignof(max_align_t)) char data[];
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
void *buffer; // data buffer
void *buffer_end; // end of data buffer
size_t capacity; // maximum number of items in the buffer
size_t count; // number of items in the buffer
size_t sz; // size of each item in the buffer
void *head; // pointer to head
void *tail; // pointer to tail
void *buffer; // data buffer
void *buffer_end; // end of data buffer
size_t capacity; // maximum number of items in the buffer
size_t count; // number of items in the buffer
size_t sz; // size of each item in the buffer
void *head; // pointer to head
void *tail; // pointer to tail
// Ensure proper memory alignment
alignas(alignof(max_align_t)) char data[];

Comment on lines +34 to +35
typedef volatile uint64_t reg_t;
typedef uint64_t imm_t;
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure proper type safety and usage.

Ensure that the typedefs for reg_t and imm_t are used correctly and safely.

+typedef volatile uint64_t reg_t;
+typedef uint64_t imm_t;
+// Ensure proper type safety
+static_assert(sizeof(reg_t) == sizeof(imm_t), "reg_t and imm_t must be the same size");
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
typedef volatile uint64_t reg_t;
typedef uint64_t imm_t;
typedef volatile uint64_t reg_t;
typedef uint64_t imm_t;
// Ensure proper type safety
static_assert(sizeof(reg_t) == sizeof(imm_t), "reg_t and imm_t must be the same size");

Comment on lines +30 to +34
void cb_free(circular_buffer *cb)
{
free(cb->buffer);
// clear out other fields too, just to be safe
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Reset circular buffer attributes in cb_free.

To prevent potential use-after-free errors, reset other attributes of the circular buffer after freeing the memory.

void cb_free(circular_buffer *cb)
{
    free(cb->buffer);
    // clear out other fields too, just to be safe
+    cb->buffer = NULL;
+    cb->buffer_end = NULL;
+    cb->capacity = 0;
+    cb->count = 0;
+    cb->sz = 0;
+    cb->head = NULL;
+    cb->tail = NULL;
}
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
void cb_free(circular_buffer *cb)
{
free(cb->buffer);
// clear out other fields too, just to be safe
}
void cb_free(circular_buffer *cb)
{
free(cb->buffer);
// clear out other fields too, just to be safe
cb->buffer = NULL;
cb->buffer_end = NULL;
cb->capacity = 0;
cb->count = 0;
cb->sz = 0;
cb->head = NULL;
cb->tail = NULL;
}

Comment on lines +14 to +28
void cb_init(circular_buffer *cb, size_t capacity, size_t sz)
{
cb->buffer = malloc(capacity * sz);
if(cb->buffer == NULL) {
// handle error
lf_print("ERROR: Fail to allocate memory to circular buffer.");
return;
}
cb->buffer_end = (char *)cb->buffer + capacity * sz;
cb->capacity = capacity;
cb->count = 0;
cb->sz = sz;
cb->head = cb->buffer;
cb->tail = cb->buffer;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Improve error handling in cb_init.

If memory allocation fails, the function should free any partially allocated resources and return an error code.

void cb_init(circular_buffer *cb, size_t capacity, size_t sz)
{
    cb->buffer = malloc(capacity * sz);
    if(cb->buffer == NULL) {
        // handle error
        lf_print("ERROR: Fail to allocate memory to circular buffer.");
-        return;
+        cb->buffer_end = NULL;
+        cb->capacity = 0;
+        cb->count = 0;
+        cb->sz = 0;
+        cb->head = NULL;
+        cb->tail = NULL;
+        return -1;
    }
    cb->buffer_end = (char *)cb->buffer + capacity * sz;
    cb->capacity = capacity;
    cb->count = 0;
    cb->sz = sz;
    cb->head = cb->buffer;
    cb->tail = cb->buffer;
+    return 0;
}
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
void cb_init(circular_buffer *cb, size_t capacity, size_t sz)
{
cb->buffer = malloc(capacity * sz);
if(cb->buffer == NULL) {
// handle error
lf_print("ERROR: Fail to allocate memory to circular buffer.");
return;
}
cb->buffer_end = (char *)cb->buffer + capacity * sz;
cb->capacity = capacity;
cb->count = 0;
cb->sz = sz;
cb->head = cb->buffer;
cb->tail = cb->buffer;
}
void cb_init(circular_buffer *cb, size_t capacity, size_t sz)
{
cb->buffer = malloc(capacity * sz);
if(cb->buffer == NULL) {
// handle error
lf_print("ERROR: Fail to allocate memory to circular buffer.");
cb->buffer_end = NULL;
cb->capacity = 0;
cb->count = 0;
cb->sz = 0;
cb->head = NULL;
cb->tail = NULL;
return -1;
}
cb->buffer_end = (char *)cb->buffer + capacity * sz;
cb->capacity = capacity;
cb->count = 0;
cb->sz = sz;
cb->head = cb->buffer;
cb->tail = cb->buffer;
return 0;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants