feat: implement sbrk and pipe syscalls by Ivans-11 · Pull Request #14 · rcore-os/tg-rcore-tutorial

Ivans-11 · 2026-01-27T16:09:42Z

sbrk syscall (ch4-ch7): Added heap_bottom and program_brk fields to Process, implemented change_program_brk() for dynamic heap management, added unmap() in AddressSpace of tg-kernel-vm.
pipe syscall (ch7-ch8): Created PipeRingBuffer (32KB ring buffer) in tg-easy-fs/pipe.rs, extended FileHandle to support pipes, changed fd_table to Mutex<Arc<FileHandle>> for sharing，added pipe_read and pipe_write methods in user_lib
update related test cases

Copilot

Pull request overview

This pull request implements two important syscalls for the rCore tutorial OS:

Purpose: Add sbrk (dynamic heap management) and pipe (inter-process communication) syscalls to support more advanced process features across chapters 4-8.

Changes:

Implemented sbrk syscall (ch4-ch7) with heap management using heap_bottom and program_brk fields, and added unmap() method for memory deallocation
Implemented pipe syscall (ch7-ch8) using a ring buffer-based pipe with 32-byte capacity, modified FileHandle to support both files and pipes, and changed fd_table to use Arc<FileHandle> for sharing
Added corresponding test cases and updated test configurations

Reviewed changes

Copilot reviewed 22 out of 27 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
tg-kernel-vm/src/space/mod.rs	Added unmap() method to remove virtual memory mappings
tg-kernel-vm/Cargo.toml	Version bump to 0.1.0-preview.2
tg-easy-fs/src/pipe.rs	New file implementing PipeRingBuffer with 32-byte ring buffer
tg-easy-fs/src/file.rs	Extended FileHandle to support both files and pipes, changed offset to Cell for interior mutability
tg-easy-fs/src/lib.rs	Exported pipe module and make_pipe function
tg-easy-fs/Cargo.toml	Version bump to 0.1.0-preview.2
tg-easy-fs/README.md	Added documentation for pipe support
tg-syscall/src/user.rs	Added sbrk() and pipe() syscall wrappers
tg-syscall/src/kernel/mod.rs	Added sbrk and pipe handler registration
ch4/src/process.rs	Added heap_bottom, program_brk fields and change_program_brk() method
ch4/src/main.rs	Added sbrk syscall handler
ch5/src/process.rs	Added heap management fields and implementation
ch5/src/main.rs	Added sbrk syscall handler
ch6/src/process.rs	Added heap management fields and implementation
ch6/src/main.rs	Added sbrk syscall handler, removed `mut` from file lock
ch7/src/process.rs	Added heap management and changed fd_table to use Arc
ch7/src/main.rs	Added sbrk and pipe syscall handlers, updated file operations
ch8/src/process.rs	Changed fd_table to use Arc for pipe sharing
ch8/src/main.rs	Added pipe syscall handler, updated file operations
user/src/lib.rs	Added pipe_read() and pipe_write() helper functions
user/src/bin/sbrk.rs	Test case for sbrk syscall
user/src/bin/pipetest.rs	Basic pipe functionality test
user/src/bin/pipe_large_test.rs	Large data transfer test for pipes
user/src/bin/ch6_usertest.rs	Added sbrk test to ch6 test suite
user/src/bin/ch8_usertest.rs	Added pipetest to ch8 test suite
user/cases.toml	Updated test configurations for all chapters
Cargo.toml	Updated version dependencies

Comments suppressed due to low confidence (1)

ch8/src/process.rs:51

The Process struct in ch8 is missing the heap_bottom and program_brk fields that were added in ch4-ch7. This means the sbrk syscall won't work properly in ch8, as there's no place to store or track the heap state. These fields need to be added to the Process struct and properly initialized in from_elf and copied in fork.

/// 进程。
pub struct Process {
    /// 不可变
    pub pid: ProcId,
    /// 可变
    pub address_space: AddressSpace<Sv39, Sv39Manager>,
    /// 文件描述符表
    pub fd_table: Vec<Option<Mutex<Arc<FileHandle>>>>,
    /// 信号模块
    pub signal: Box<dyn Signal>,
    /// 分配的锁以及信号量
    pub semaphore_list: Vec<Option<Arc<Semaphore>>>,
    pub mutex_list: Vec<Option<Arc<dyn MutexTrait>>>,
    pub condvar_list: Vec<Option<Arc<Condvar>>>,
}

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-27T16:29:48Z

+                };
+            }
+            vpn = vpn + 1;
+        }


The unmap implementation clears page table entries but does not perform a TLB flush (sfence.vma). After unmapping pages, the TLB may still contain stale mappings, which could cause the CPU to access freed physical pages. A TLB flush should be added after modifying the page tables.

Suggested change

}

}

// 在修改页表后刷新 TLB，防止使用已失效的映射

unsafe {

core::arch::asm!("sfence.vma");

}

Copilot · 2026-01-27T16:29:49Z

+            let read_fd = current.fd_table.len();
+            current.fd_table.push(Some(Mutex::new(read_end)));
+            let write_fd = current.fd_table.len();
+            current.fd_table.push(Some(Mutex::new(write_end)));
+            // 将 read_fd 写入 pipe[0]
+            if let Some(mut ptr) = current
+                .address_space
+                .translate::<usize>(VAddr::new(pipe), WRITEABLE)
+            {
+                unsafe { *ptr.as_mut() = read_fd };
+            } else {
+                return -1;
+            }
+            // 将 write_fd 写入 pipe[1]
+            if let Some(mut ptr) = current
+                .address_space
+                .translate::<usize>(VAddr::new(pipe + core::mem::size_of::<usize>()), WRITEABLE)
+            {
+                unsafe { *ptr.as_mut() = write_fd };
+            } else {
+                return -1;
+            }


If the first address translation succeeds (line 441-445) but the second fails (line 450-457), the function returns -1 but the pipe file descriptors have already been added to the fd_table. This leaves the process with a partially-created pipe (only the read end in the fd_table) that cannot be used properly. Consider either removing the added file descriptors on error, or performing both translations before adding any file descriptors.

Suggested change

let read_fd = current.fd_table.len();

current.fd_table.push(Some(Mutex::new(read_end)));

let write_fd = current.fd_table.len();

current.fd_table.push(Some(Mutex::new(write_end)));

// 将 read_fd 写入 pipe[0]

if let Some(mut ptr) = current

.address_space

.translate::<usize>(VAddr::new(pipe), WRITEABLE)

{

unsafe { *ptr.as_mut() = read_fd };

} else {

return -1;

}

// 将 write_fd 写入 pipe[1]

if let Some(mut ptr) = current

.address_space

.translate::<usize>(VAddr::new(pipe + core::mem::size_of::<usize>()), WRITEABLE)

{

unsafe { *ptr.as_mut() = write_fd };

} else {

return -1;

}

// 计算即将分配的文件描述符编号，但暂时不修改 fd_table

let read_fd = current.fd_table.len();

let write_fd = read_fd + 1;

// 先进行两次地址转换，若失败则不对 fd_table 做任何修改

let mut ptr0 = if let Some(ptr) = current

.address_space

.translate::<usize>(VAddr::new(pipe), WRITEABLE)

{

ptr

} else {

return -1;

};

let mut ptr1 = if let Some(ptr) = current

.address_space

.translate::<usize>(

VAddr::new(pipe + core::mem::size_of::<usize>()),

WRITEABLE,

)

{

ptr

} else {

return -1;

};

// 地址转换均成功后再向 fd_table 中添加管道端点

current.fd_table.push(Some(Mutex::new(read_end)));

current.fd_table.push(Some(Mutex::new(write_end)));

// 将 read_fd 写入 pipe[0]

unsafe { *ptr0.as_mut() = read_fd };

// 将 write_fd 写入 pipe[1]

unsafe { *ptr1.as_mut() = write_fd };

Copilot · 2026-01-27T16:29:49Z

+
+    /// 所有写端是否都已关闭
+    pub fn all_write_ends_closed(&self) -> bool {
+        self.write_end.as_ref().unwrap().upgrade().is_none()


The method all_write_ends_closed() will panic with unwrap() if write_end is None. This can happen if set_write_end() is never called. Consider handling this case gracefully or documenting the invariant that set_write_end must be called before all_write_ends_closed.

Suggested change

self.write_end.as_ref().unwrap().upgrade().is_none()

match self.write_end.as_ref() {

Some(weak) => weak.upgrade().is_none(),

None => false,

}

Copilot · 2026-01-27T16:29:49Z

+    /// 取消指定 VPN 范围的映射
+    pub fn unmap(&mut self, range: Range<VPN<Meta>>) {
+        // 从 areas 中移除该范围（可能需要拆分现有区域）
+        let mut new_areas = Vec::new();
+        for area in self.areas.drain(..) {
+            if area.end <= range.start || area.start >= range.end {
+                // 不重叠，保留原区域
+                new_areas.push(area);
+            } else {
+                // 有重叠，需要拆分
+                if area.start < range.start {
+                    new_areas.push(area.start..range.start);
+                }
+                if area.end > range.end {
+                    new_areas.push(range.end..area.end);
+                }
+            }
+        }
+        self.areas = new_areas;
+
+        // 清除页表项（将 PTE 设为无效，即写入 0）
+        let mut vpn = range.start;
+        while vpn < range.end {
+            // 使用 visitor 找到 PTE 并清除
+            if let Some(pte_ptr) = self.find_pte_mut(vpn) {
+                unsafe {
+                    core::ptr::write_bytes(
+                        pte_ptr as *mut u8,
+                        0,
+                        core::mem::size_of::<page_table::Pte<Meta>>(),
+                    )
+                };
+            }
+            vpn = vpn + 1;
+        }
+    }


The unmap implementation does not free the allocated physical pages. It only clears the page table entries, but the physical memory remains allocated. This causes a memory leak. The implementation should call page_manager.deallocate() or similar to free the physical pages before clearing the PTEs.

Copilot · 2026-01-27T16:29:50Z

+    /// 写入一个字节
+    pub fn write_byte(&mut self, byte: u8) {
+        self.status = RingBufferStatus::Normal;
+        self.arr[self.tail] = byte;
+        self.tail = (self.tail + 1) % RING_BUFFER_SIZE;
+        if self.tail == self.head {
+            self.status = RingBufferStatus::Full;
+        }
+    }
+
+    /// 读取一个字节
+    pub fn read_byte(&mut self) -> u8 {
+        self.status = RingBufferStatus::Normal;
+        let c = self.arr[self.head];
+        self.head = (self.head + 1) % RING_BUFFER_SIZE;
+        if self.head == self.tail {
+            self.status = RingBufferStatus::Empty;
+        }
+        c
+    }


The write_byte and read_byte methods do not check if the buffer is full or empty before writing/reading. If write_byte is called when the buffer is full, it will overwrite data. If read_byte is called when the buffer is empty, it will return stale data. The caller is responsible for checking available_write/available_read first, but this could lead to bugs if the API is misused. Consider adding assertions or returning Option/Result to enforce correct usage.

Copilot · 2026-01-27T16:29:50Z

+        if self.pipe {
+            assert!(self.writable() && self.buffer.is_some());
+            let want_to_write = buf.len();
+            let mut buf_iter = buf.into_iter();
+            let mut already_write = 0usize;
+            let mut ring_buffer = self.buffer.as_ref().unwrap().lock();
+            let loop_write = ring_buffer.available_write();
+            if loop_write == 0 {
+                return -2; // 缓冲区满，需等待
+            }
+            // 写入尽可能多的数据
+            for _ in 0..loop_write {
+                if let Some(byte_ref) = buf_iter.next() {
+                    ring_buffer.write_byte(unsafe { *byte_ref });
+                    already_write += 1;
+                    if already_write == want_to_write {
+                        return want_to_write as _;
+                    }
+                } else {
+                    return already_write as _;
+                }
            }
-            total_write_size as _
+            // 缓冲区写满但还没写完，返回已写入的字节数
+            already_write as _


The pipe write implementation does not check if all read ends are closed. In typical Unix semantics, writing to a pipe with no readers should cause a SIGPIPE signal or return an error (EPIPE). The current implementation will succeed even when no readers exist, which deviates from standard pipe behavior and may lead to unexpected behavior.

Copilot · 2026-01-27T16:29:50Z

+            let (read_end, write_end) = make_pipe();
+            let read_fd = current.fd_table.len();
+            current.fd_table.push(Some(Mutex::new(read_end)));
+            let write_fd = current.fd_table.len();
+            current.fd_table.push(Some(Mutex::new(write_end)));
+            // 将 read_fd 写入 pipe[0]
+            if let Some(mut ptr) = current
+                .address_space
+                .translate::<usize>(VAddr::new(pipe), WRITEABLE)
+            {
+                unsafe { *ptr.as_mut() = read_fd };
+            } else {
+                return -1;
+            }
+            // 将 write_fd 写入 pipe[1]
+            if let Some(mut ptr) = current
+                .address_space
+                .translate::<usize>(VAddr::new(pipe + core::mem::size_of::<usize>()), WRITEABLE)
+            {
+                unsafe { *ptr.as_mut() = write_fd };
+            } else {
+                return -1;


If the first address translation succeeds but the second fails, the function returns -1 but the pipe file descriptors have already been added to the fd_table. This leaves the process with a partially-created pipe (only the read end in the fd_table) that cannot be used properly. Consider either removing the added file descriptors on error, or performing both translations before adding any file descriptors.

Suggested change

let (read_end, write_end) = make_pipe();

let read_fd = current.fd_table.len();

current.fd_table.push(Some(Mutex::new(read_end)));

let write_fd = current.fd_table.len();

current.fd_table.push(Some(Mutex::new(write_end)));

// 将 read_fd 写入 pipe[0]

if let Some(mut ptr) = current

.address_space

.translate::<usize>(VAddr::new(pipe), WRITEABLE)

{

unsafe { *ptr.as_mut() = read_fd };

} else {

return -1;

}

// 将 write_fd 写入 pipe[1]

if let Some(mut ptr) = current

.address_space

.translate::<usize>(VAddr::new(pipe + core::mem::size_of::<usize>()), WRITEABLE)

{

unsafe { *ptr.as_mut() = write_fd };

} else {

return -1;

// First, translate both user-space addresses. Only proceed if both succeed.

let read_ptr_opt = current

.address_space

.translate::<usize>(VAddr::new(pipe), WRITEABLE);

let write_ptr_opt = current

.address_space

.translate::<usize>(

VAddr::new(pipe + core::mem::size_of::<usize>()),

WRITEABLE,

);

if read_ptr_opt.is_none() || write_ptr_opt.is_none() {

return -1;

}

let (read_end, write_end) = make_pipe();

let read_fd = current.fd_table.len();

current.fd_table.push(Some(Mutex::new(read_end)));

let write_fd = current.fd_table.len();

current.fd_table.push(Some(Mutex::new(write_end)));

// 将 read_fd 写入 pipe[0]

if let Some(mut ptr) = read_ptr_opt {

unsafe { *ptr.as_mut() = read_fd };

}

// 将 write_fd 写入 pipe[1]

if let Some(mut ptr) = write_ptr_opt {

unsafe { *ptr.as_mut() = write_fd };

Copilot · 2026-01-27T16:29:51Z

+use alloc::sync::{Arc, Weak};
+use spin::Mutex;
+
+const RING_BUFFER_SIZE: usize = 32;


The RING_BUFFER_SIZE is set to 32 bytes, but according to the PR description, it should be 32KB (32 * 1024 bytes). This will severely limit the pipe capacity and may cause frequent blocking.

Suggested change

const RING_BUFFER_SIZE: usize = 32;

const RING_BUFFER_SIZE: usize = 32 * 1024;

Copilot · 2026-01-27T16:29:51Z

+/// 缓冲区快照，用于恢复状态
+#[derive(Copy, Clone)]
+pub struct BufferSnapshot {
+    head: usize,
+    tail: usize,
+    status: RingBufferStatus,
+}
+
+/// 管道环形缓冲区
+pub struct PipeRingBuffer {
+    arr: [u8; RING_BUFFER_SIZE],
+    head: usize,
+    tail: usize,
+    status: RingBufferStatus,
+    write_end: Option<Weak<FileHandle>>,
+}
+
+impl PipeRingBuffer {
+    /// 创建一个管道环形缓冲区
+    pub fn new() -> Self {
+        Self {
+            arr: [0; RING_BUFFER_SIZE],
+            head: 0,
+            tail: 0,
+            status: RingBufferStatus::Empty,
+            write_end: None,
+        }
+    }
+
+    /// 设置写端
+    pub fn set_write_end(&mut self, write_end: &Arc<FileHandle>) {
+        self.write_end = Some(Arc::downgrade(write_end));
+    }
+
+    /// 写入一个字节
+    pub fn write_byte(&mut self, byte: u8) {
+        self.status = RingBufferStatus::Normal;
+        self.arr[self.tail] = byte;
+        self.tail = (self.tail + 1) % RING_BUFFER_SIZE;
+        if self.tail == self.head {
+            self.status = RingBufferStatus::Full;
+        }
+    }
+
+    /// 读取一个字节
+    pub fn read_byte(&mut self) -> u8 {
+        self.status = RingBufferStatus::Normal;
+        let c = self.arr[self.head];
+        self.head = (self.head + 1) % RING_BUFFER_SIZE;
+        if self.head == self.tail {
+            self.status = RingBufferStatus::Empty;
+        }
+        c
+    }
+
+    /// 可读取的字节数
+    pub fn available_read(&self) -> usize {
+        if self.status == RingBufferStatus::Empty {
+            0
+        } else if self.tail > self.head {
+            self.tail - self.head
+        } else {
+            self.tail + RING_BUFFER_SIZE - self.head
+        }
+    }
+
+    /// 可写入的字节数
+    pub fn available_write(&self) -> usize {
+        if self.status == RingBufferStatus::Full {
+            0
+        } else {
+            RING_BUFFER_SIZE - self.available_read()
+        }
+    }
+
+    /// 所有写端是否都已关闭
+    pub fn all_write_ends_closed(&self) -> bool {
+        self.write_end.as_ref().unwrap().upgrade().is_none()
+    }
+
+    /// 保存当前状态快照
+    pub fn save_state(&self) -> BufferSnapshot {
+        BufferSnapshot {
+            head: self.head,
+            tail: self.tail,
+            status: self.status,
+        }
+    }
+
+    /// 恢复到之前的状态
+    pub fn restore_state(&mut self, snapshot: BufferSnapshot) {
+        self.head = snapshot.head;
+        self.tail = snapshot.tail;
+        self.status = snapshot.status;
+    }


The BufferSnapshot struct and its related methods (save_state, restore_state) are defined but never used in the codebase. This appears to be dead code that should either be removed or used for some intended purpose.

Ivans-11 added 2 commits January 27, 2026 00:25

feat: add sbrk syscall

7bc0f2f

feat: add pipe syscall

23b3ddc

Ivans-11 requested a review from Copilot January 27, 2026 16:11

Copilot started reviewing on behalf of Ivans-11 January 27, 2026 16:11 View session

Copilot AI reviewed Jan 27, 2026

View reviewed changes

fix: some extra change

22c6a0a

Ivans-11 merged commit 57b795e into rcore-os:lab Jan 27, 2026
1 of 6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement sbrk and pipe syscalls#14

feat: implement sbrk and pipe syscalls#14
Ivans-11 merged 3 commits into
rcore-os:labfrom
Ivans-11:lab

Ivans-11 commented Jan 27, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jan 27, 2026

Uh oh!

Copilot AI Jan 27, 2026

Uh oh!

Copilot AI Jan 27, 2026

Uh oh!

Copilot AI Jan 27, 2026

Uh oh!

Copilot AI Jan 27, 2026

Uh oh!

Copilot AI Jan 27, 2026

Uh oh!

Copilot AI Jan 27, 2026

Uh oh!

Copilot AI Jan 27, 2026

Uh oh!

Copilot AI Jan 27, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

-        }
+        }
+        // 在修改页表后刷新 TLB，防止使用已失效的映射
+        unsafe {
+            core::arch::asm!("sfence.vma");
+        }

-        self.write_end.as_ref().unwrap().upgrade().is_none()
+        match self.write_end.as_ref() {
+            Some(weak) => weak.upgrade().is_none(),
+            None => false,
+        }

	const RING_BUFFER_SIZE: usize = 32;
	const RING_BUFFER_SIZE: usize = 32 * 1024;

Conversation

Ivans-11 commented Jan 27, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants