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.

Sign up for GitHub

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

Jump to bottom

refactor: batch update bpf elements #678

Closed
wants to merge 4 commits into from
Closed

refactor: batch update bpf elements #678

wants to merge 4 commits into from

Conversation

Integral-Tech
Copy link
Contributor

Background

Checklist

Full Changelogs

  • Fix TODO: batch update bpf elements

@Integral-Tech Integral-Tech requested a review from a team as a code owner October 17, 2024 08:34
jschwinger233
jschwinger233 reviewed Oct 17, 2024
View reviewed changes
Copy link
Member

@jschwinger233 jschwinger233 left a comment

Choose a reason for hiding this comment

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

bpf单测挂了 你可以看看测试文件失败的那个是什么规则

@jschwinger233
Copy link
Member

而且这也并非update batch吧。。

@Integral-Tech
Copy link
Contributor Author

而且这也并非update batch吧。。

I found a TODO comment in tproxy.c, so I tried to fix it

jschwinger233
jschwinger233 requested changes Oct 17, 2024
View reviewed changes
Copy link
Member

@jschwinger233 jschwinger233 left a comment

Choose a reason for hiding this comment

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

BPF_MAP_UPDATE_BATCH 是用户态 API 才能用的，内核态不能用吧。另外重构引入了 bug。。。

感谢热情和贡献，我个人感觉目前 bpf 急需更多测试，否则稍不注意改出 bug 肉眼很难看出来。如果有兴趣要不要看看 bpf 单测是怎么做的，然后试着拓展一下？路由的逻辑还没100%覆盖，在完善测试的时候也正好熟悉dae业务逻辑，之后也好上手修改。

control/kern/tproxy.c Outdated Show resolved Hide resolved
Integral and others added 2 commits October 18, 2024 01:13
@Integral-Tech
Copy link
Contributor Author

@jschwinger233 Now BPF test passed :) Thanks for your review

@jschwinger233
Copy link
Member

我个人的想法是 #580 很快就要彻底删掉这些代码，加上似乎没有什么价值（可读性甚至更差了一点？），还会给 #580 带来冲突，所以。。。让 mzz 做决定吧，逃

@Integral-Tech
Copy link
Contributor Author

我个人的想法是 #580 很快就要彻底删掉这些代码，加上似乎没有什么价值（可读性甚至更差了一点？），还会给 #580 带来冲突，所以。。。让 mzz 做决定吧，逃

那就算了吧，我只是正好看到了这个 TODO

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jschwinger233 jschwinger233 jschwinger233 requested changes

Assignees

@Integral-Tech Integral-Tech

Labels
not-yet-tested refactor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Integral-Tech @jschwinger233