Add support for rdma cgroup introduced in Linux Kernel 4.11 #2883
Conversation
flouthoc
force-pushed
the
master
branch
3 times, most recently
from
2f865ee to
c357a29
Compare
libcontainer/cgroups/fs/rdma.go
Outdated
| } | ||
|
|
||
| // Parse raw string to RdmaEntry | ||
| func ParseRdmaKV(raw string, entry *cgroups.RdmaEntry) { |
There was a problem hiding this comment.
Why this function is public?
There was a problem hiding this comment.
@kolyshkin Following function is intentionally public as being used by rdma controller in fs2. I didnt wanted to have redundant code in both implementations of fs and fs2 and these functions are exclusive to Rdma so i thought i'd rather keep them with rdma and not move to fscommon
There was a problem hiding this comment.
fscommon can hold the helper for RDMA. fs2 imports fscommon better than fs package. just my two cents.
There was a problem hiding this comment.
@fuweid Oh so idea was since ParseRdmaKV and ConvertRdmaEntry uses types from package cgroups having them in fscommon will introduce import cycles. Since both of these functions are only going to be used only by rdma therefore i thought its better to keep them with rdma implementation only. But let me know if you guys think otherwise we can figure out a way. 😄
There was a problem hiding this comment.
I prefer to keep the duplicate code in separate packages (non-binding). :)
There was a problem hiding this comment.
I think this should go to fscommon, which naturally contains the stuff that's common between fs and fs2.
There was a problem hiding this comment.
Which is a problem, as described above (02eb723#r610598989).
To solve it, I think we should move OpenFile/ReadFile etc from fscommon to cgroups/utils.
There was a problem hiding this comment.
Resolved in latest commit.
|
Looks good in general, left some comments. We also need some integration tests for that. |
flouthoc
force-pushed
the
master
branch
2 times, most recently
from
eaa7eee to
09a39b6
Compare
|
@kolyshkin Thanks for the quick review, i have fixed almost everything. 3 comments are still unresolved but i have commented the reason on the review itself. Could you please take a look. |
|
I see there's a typo in the commit message ("kernal" instead of "kernel"); perhaps good to fix that in case we want to find it back later if someone searches for |
|
@kolyshkin Resolved all of them just a small doubt in first one i have tried clarifying in review itself. |
|
@kolyshkin I have made requested changes could you PTAL. |
|
@kolyshkin Could you please let me know if anything is pending here from my side. |
flouthoc
force-pushed
the
master
branch
2 times, most recently
from
1480de6 to
3af910d
Compare
Signed-off-by: Aditya Rajan <[email protected]>
|
@AkihiroSuda thanks :) resolved in latest commit. |
RDMA controller/cgroup support is added with Linux kernel 4.11.
Similar format is used by cgroup-v2
Reference
https://www.kernel.org/doc/Documentation/cgroup-v1/rdma.txt