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

增加epoll机制 #455

Merged
merged 7 commits into from
Dec 25, 2023
Merged

Conversation

GnoCiYeH
Copy link
Member

  • 增加epoll机制
  • 添加事件等待队列,提升socket性能
  • 优化poll,删除不能poll的文件系统中的poll方法

- 增加epoll机制
- 添加事件等待队列,提升socket性能
- 优化poll,删除不能poll的文件系统中的poll方法
}

#[derive(Debug, PartialEq)]
pub enum EPollCtlOption {
Copy link
Member

Choose a reason for hiding this comment

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

这里需要加详细注释


/// 与C兼容的Epoll事件结构体
#[derive(Copy, Clone, Default)]
pub struct EPollEvent {
Copy link
Member

Choose a reason for hiding this comment

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

这里需要加详细的注释,描述字段含义以及相应的行为

}

/// ## epoll_ctl的具体实现
pub fn do_epoll_ctl(
Copy link
Member

Choose a reason for hiding this comment

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

加注释,参数定义、详细的逻辑解释

};

if op == EPollCtlOption::EpollCtlAdd {
// TODO: 循环检查?
Copy link
Member

Choose a reason for hiding this comment

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

循环检查什么内容?这里得把Linux对应的逻辑的链接放上来,不然的话下回就不记得了

Ok(())
}

fn ep_modify(
Copy link
Member

Choose a reason for hiding this comment

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

添加注释


bitflags! {
#[allow(dead_code)]
pub struct EPollEventType: u32 {
Copy link
Member

Choose a reason for hiding this comment

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

每个元素加注释

/// epoll_wait用到的等待队列
epoll_wq: WaitQueue,
/// 维护所有添加进来的socket的红黑树
epitem_rbr: RBTree<i32, Arc<EPollItem>>,
Copy link
Member

Choose a reason for hiding this comment

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

这个命名我觉得挺不好的,建议改成ep_items

}

#[derive(Debug)]
pub struct EPollItem {
Copy link
Member

@fslongjin fslongjin Nov 24, 2023

Choose a reason for hiding this comment

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

EpollItem和Epoll的关系需要注释写一下,以及这些结构体的成员的含义。其它的结构体也是。


// C标准的epoll_event大小为12字节,在内核我们不使用#[repr(packed)]来强制与C兼容,可以提高效率
let user_addr = user_event.add(res * 12);
if user_addr.is_null() {
Copy link
Member

Choose a reason for hiding this comment

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

这里有问题,这个is_null显然不会为true

}

// 因为C中的epoll_event大小为12字节,而在rust中为16字节
verify_area(events, 12 * max_events as usize)?;
Copy link
Member

@fslongjin fslongjin Nov 24, 2023

Choose a reason for hiding this comment

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

这样写可能造成内存安全问题,请改成UserBufferWriter,不然的话后面的do_epoll_wait里面容易越界,而且难以发现。
而且后面这种把用户虚拟地址一步步传下去的写法本身就很危险,写着写着,其他人都不知道这个地址是用户空间传来的了。就容易出问题,而且很难调试。不然为啥要userbuffer writer。早期调磁盘驱动bug,就是因为这样一步步传下去,结果都忘记了是用户地址了。然后出错了写坏了内核的其它位置,非常难顶。这样类似的写法一定要改过来。

Copy link
Member Author

Choose a reason for hiding this comment

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

这里是因为这个epoll_event的对其问题才这样写的,因为从用户空间传入的epoll_event的大小为12字节,而内核定义的结构体因为内存对齐,其大小为16字节,这样写是为了提升内核效率,因为只需要在这里手动管理一下,而不需要将内核结构体整个为了与C兼容而内存不对齐。这里如果直接通过UserBuffer转为内核对象数组的话是有问题的,所以通过验证地址来实现。

Copy link
Member Author

Choose a reason for hiding this comment

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

所以我在实现里面关于epoll_event的都是通过地址操作的,传入不深,会在注释标注

Copy link
Member

Choose a reason for hiding this comment

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

这里是因为这个epoll_event的对其问题才这样写的,因为从用户空间传入的epoll_event的大小为12字节,而内核定义的结构体因为内存对齐,其大小为16字节,这样写是为了提升内核效率,因为只需要在这里手动管理一下,而不需要将内核结构体整个为了与C兼容而内存不对齐。

这个时候应该提供一个用户的PosixXXXXX的结构,设置为packed,不然的话这种hard coding很容易在将来造成问题。

}

/// ### 将已经准备好的事件拷贝到用户空间
fn ep_send_events(
Copy link
Member

Choose a reason for hiding this comment

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

所有大的函数都需要把入参、返回值、功能写注释。

let wait_ret = Self::epoll_wait(epfd, epoll_event, max_events, timespec);

if wait_ret.is_err() && *wait_ret.as_ref().unwrap_err() != SystemError::EINTR {
// TODO: 恢复信号?
Copy link
Member

Choose a reason for hiding this comment

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

加链接

SYS_EPOLL_CREATE => Self::epoll_create(args[0] as i32),
SYS_EPOLL_CREATE1 => Self::epoll_create1(args[0]),

SYS_EPOLL_CTL => Self::epoll_ctl(
Copy link
Member

Choose a reason for hiding this comment

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

地址全部都要用userbuffer结构体去校验,然后把校验得到的结构体传下去。

Copy link
Member Author

Choose a reason for hiding this comment

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

emmm,内部都有校验的,也是上面提到的原因,这里涉及地址的地方基本上都是epoll_event结构体

Copy link
Member

@fslongjin fslongjin left a comment

Choose a reason for hiding this comment

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

总的来说,注释太少了,并且裸指针、内存越界问题没有关注到,一定要重视这个内存访问的问题,哪怕能跑,但是不代表以后不会出问题。巨锤🔨🔨🔨🔨🔨

// TODO:这里有可能会出现事件丢失的情况
// 如果用户传入的数组长度小于传入的max_event,到这里时如果已经到数组最大长度,但是未到max_event
// 会出现的问题是我们会把这个数据写入到后面的内存中,用户无法在传入的数组中拿到事件,而且写脏数据到了后面一片内存,导致事件丢失
// 出现这个问题的几率比较小,首先是因为用户的使用不规范,后因为前面地址校验是按照max_event来校验的,只会在两块内存连着分配时出现,但是也是需要考虑的
Copy link
Member Author

Choose a reason for hiding this comment

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

这里我还没有想好如何解决,但是问题出现的几率很小而且主要是因为用户态的使用不规范才会出现这样的问题。如果有合适解决方法cue我一下

Copy link
Member

Choose a reason for hiding this comment

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

这个我感觉只要不影响内核的空间安全就行。

@fslongjin fslongjin merged commit 4060997 into DragonOS-Community:master Dec 25, 2023
4 checks passed
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.

2 participants