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

file对epoll add和epoll remove的处理,扩展性较差 #896

Open
fslongjin opened this issue Aug 21, 2024 · 12 comments
Open

file对epoll add和epoll remove的处理,扩展性较差 #896

fslongjin opened this issue Aug 21, 2024 · 12 comments
Assignees
Labels
A-fs Area: 文件系统 A-network Area: 网络子系统 In-progress

Comments

@fslongjin
Copy link
Member

问题

File对象的add_epoll和remove_epoll方法,里面对文件类型进行了特判。
我认为可以统一处理这个事情,给IndexNode这个trait加add_epoll()和remove_epoll()函数会比较好。

@Godones 另一个问题,对于代码:https://github.com/DragonOS-Community/DragonOS/pull/894/files#diff-08d0c277f75d9d64222b3634cc6c86f177e35c79e7cda7ca1161a1211cde4922R522
不能认为,“不是pipe,且不是socket的,那么这种inode就是EventFdInode”
这个跟上面Add的时候的语义完全就对应不上,需要修改。

抄送:@GnoCiYeH @Chiichen

@fslongjin fslongjin added the enhancement New feature or request label Aug 21, 2024
@dragonosbot dragonosbot added the needs-triage 这个问题可能需要分类处理。如果已经完成分类,请移除它。 label Aug 21, 2024
@github-actions github-actions bot removed the enhancement New feature or request label Aug 21, 2024
@Godones
Copy link
Contributor

Godones commented Aug 22, 2024

问题

File对象的add_epoll和remove_epoll方法,里面对文件类型进行了特判。 我认为可以统一处理这个事情,给IndexNode这个trait加add_epoll()和remove_epoll()函数会比较好。

@Godones 另一个问题,对于代码:https://github.com/DragonOS-Community/DragonOS/pull/894/files#diff-08d0c277f75d9d64222b3634cc6c86f177e35c79e7cda7ca1161a1211cde4922R522 不能认为,“不是pipe,且不是socket的,那么这种inode就是EventFdInode” 这个跟上面Add的时候的语义完全就对应不上,需要修改。

抄送:@GnoCiYeH @Chiichen

这里并不是说“不是pipe,且不是socket的,那么这种inode就是EventFdInode”,因为这里只是尝试向下转换为EventFdInode,如果成功了,那它就是EventFdInode,如果不是,那就按照原来的方式返回SystemError::ENOSYS

@Godones
Copy link
Contributor

Godones commented Aug 22, 2024

给IndexNode这个trait加add_epoll()和remove_epoll()函数后,我觉得可以对这些进行统一,因为还有其它类型的Inode也会使用这些函数。

@GnoCiYeH
Copy link
Member

问题

File对象的add_epoll和remove_epoll方法,里面对文件类型进行了特判。 我认为可以统一处理这个事情,给IndexNode这个trait加add_epoll()和remove_epoll()函数会比较好。

@Godones 另一个问题,对于代码:https://github.com/DragonOS-Community/DragonOS/pull/894/files#diff-08d0c277f75d9d64222b3634cc6c86f177e35c79e7cda7ca1161a1211cde4922R522 不能认为,“不是pipe,且不是socket的,那么这种inode就是EventFdInode” 这个跟上面Add的时候的语义完全就对应不上,需要修改。

抄送:@GnoCiYeH @Chiichen

只有pollable的iNode才支持epoll,这里感觉统一加在iNode里有点奇怪,是否新加一个trait:Pollable来管理这个更好?

@Godones
Copy link
Contributor

Godones commented Aug 22, 2024

@GnoCiYeH 请问一下在进程进行epoll时,判断是否有就绪事件的代码如下:

 // 判断epoll上有没有就绪事件
let mut available = epoll_guard.ep_events_available();

这只是简单判断了队列是否为空。这意味着如果一个文件有就绪事件,它需要主动把事件放到队列中。这是怎么实现的?

@GnoCiYeH
Copy link
Member

@GnoCiYeH 请问一下在进程进行epoll时,判断是否有就绪事件的代码如下:

 // 判断epoll上有没有就绪事件
let mut available = epoll_guard.ep_events_available();

这只是简单判断了队列是否为空。这意味着如果一个文件有就绪事件,它需要主动把事件放到队列中。这是怎么实现的?

这个目前是暴力实现的,比如我把一个epitem绑定在socket上,那网卡来数据时,就会通知这个socket对应的epoll。

@Godones
Copy link
Contributor

Godones commented Aug 22, 2024

这个地方应该感觉需要做一个重构吧,内核中不止socket会产生事件。

@GnoCiYeH
Copy link
Member

这个地方应该感觉需要做一个重构吧,内核中不止socket会产生事件。

是的,要是可以在wq里面注册回调来实现是最好的,但是目前wq没有这样的功能。这种方式rust也不太好设计。。

@Godones
Copy link
Contributor

Godones commented Aug 22, 2024

是否可以在poll的时候主动轮询一下?

@GnoCiYeH
Copy link
Member

是否可以在poll的时候主动轮询一下?

epoll最核心的地方在于:有数据来才唤醒。在poll的时候主动轮询解决不了问题,因为poll这个动作是在唤醒epoll后来做的,但是问题在于该如何唤醒epoll?

@Godones
Copy link
Contributor

Godones commented Aug 22, 2024

是否可以在poll的时候主动轮询一下?

epoll最核心的地方在于:有数据来才唤醒。在poll的时候主动轮询解决不了问题,因为poll这个动作是在唤醒epoll后来做的,但是问题在于该如何唤醒epoll?

我大概理解怎么唤醒了,按照现在的pipe相关的代码,在对特殊文件进行操作的时候,可以检查是否需要唤醒epoll,比如pipe文件就在读写操作的时候进行了唤醒。

@fslongjin
Copy link
Member Author

@Godones

这个pr添加的宏,可以在wq被唤醒之后,检查条件是否满足,如果满足才返回。
不过目前只应用到了wq,而event wq没有用到。虽然但是我感觉这两个wq将来是不是可以统一一下。
#889

@Samuka007
Copy link
Member

问题

File对象的add_epoll和remove_epoll方法,里面对文件类型进行了特判。 我认为可以统一处理这个事情,给IndexNode这个trait加add_epoll()和remove_epoll()函数会比较好。
@Godones 另一个问题,对于代码:https://github.com/DragonOS-Community/DragonOS/pull/894/files#diff-08d0c277f75d9d64222b3634cc6c86f177e35c79e7cda7ca1161a1211cde4922R522 不能认为,“不是pipe,且不是socket的,那么这种inode就是EventFdInode” 这个跟上面Add的时候的语义完全就对应不上,需要修改。
抄送:@GnoCiYeH @Chiichen

只有pollable的iNode才支持epoll,这里感觉统一加在iNode里有点奇怪,是否新加一个trait:Pollable来管理这个更好?

+1
是否Pollable应该是一个类型绑定的Inode属性,目前依赖错误返回来判断是否Pollable也影响到socket的实现的简洁度

@Samuka007 Samuka007 added In-progress A-fs Area: 文件系统 A-network Area: 网络子系统 and removed needs-triage 这个问题可能需要分类处理。如果已经完成分类,请移除它。 labels Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-fs Area: 文件系统 A-network Area: 网络子系统 In-progress
Projects
None yet
Development

No branches or pull requests

6 participants