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

cpumask: add a function like for_each_online_cpu #728

Open
wants to merge 1 commit into
base: rust
Choose a base branch
from

Conversation

Richardhongyu
Copy link

@Richardhongyu Richardhongyu commented Mar 26, 2022

This commit add a function return a struct implementing Iterator trait. Thus, this struct can be used as an iterator that returns an online cpu index each time.

Signed-off-by: Hongyu Li lihongyu1999@bupt.edu.cn

@Richardhongyu
Copy link
Author

The detail of this PR can be seen in #727.

@bjorn3
Copy link
Member

bjorn3 commented Mar 26, 2022

(missing Signed-Off-By)

@Richardhongyu
Copy link
Author

Thanks for reminding me:)

Copy link
Member

@ojeda ojeda left a comment

Choose a reason for hiding this comment

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

Some general notes:

  • There should be an example of usage (since this depends on a running kernel, wrap the code inside a fn -- please grep for # Example to see how it is done elsewhere).
  • Please use Markdown in documentation and links wherever possible.
  • Should we add other iterators, not just online ones?
  • More importantly, how will the index be used by users of the iterator? Should we have higher-level iterators/abstractions over the "final data" (e.g. per CPU data) or at least wrap the index in a type that is known to be a valid index?


fn next(&mut self) -> Option<u32> {
let next_cpu_id =
unsafe { bindings::cpumask_next(self.index, &bindings::__cpu_online_mask) };
Copy link
Member

Choose a reason for hiding this comment

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

Missing a couple // SAFETY comments.

Copy link
Author

Choose a reason for hiding this comment

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

It seems that these two unsafe blocks may have some problems. I noticebindings::__cpu_online_mask and bindings::nr_cpu_ids are static mut variables. They can change during the iteration due to the hotplug. They are only safe when CONFIG_HOTPLUG_CPU is n. I'm trying to fix this bug.

Copy link
Author

Choose a reason for hiding this comment

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

The missing // SAFETY comments are added in this version. Using lock to address the data race problem.

rust/kernel/cpumask.rs Outdated Show resolved Hide resolved
rust/kernel/cpumask.rs Outdated Show resolved Hide resolved
rust/kernel/cpumask.rs Outdated Show resolved Hide resolved
rust/kernel/cpumask.rs Outdated Show resolved Hide resolved
@Richardhongyu Richardhongyu force-pushed the rust branch 2 times, most recently from 882810d to 3ce79cd Compare March 27, 2022 05:03
@Richardhongyu
Copy link
Author

Some general notes:

  • There should be an example of usage (since this depends on a running kernel, wrap the code inside a fn -- please grep for # Example to see how it is done elsewhere).

I just add an example for this function.

  • Please use Markdown in the documentation and links wherever possible.

In the updated version, rust items in the documentation are properly linked.

  • Should we add other iterators, not just online ones?

Yes, I will add other similar iterators soon that are for_each_possible_cpu and for_each_present_cpu.

  • More importantly, how will the index be used by users of the iterator? Should we have higher-level iterators/abstractions over the "final data" (e.g. per CPU data) or at least wrap the index in a type that is known to be a valid index?

In the kernel, the CPU index is an int number and is commonly used in macros like per_cpu(var, cpu_index). I think we can wrap this macros in helpers. In this case, we may not have to use a higher-level for the final data.
I don't quiet understand "wrap the index in a type that is known to be a valid index". The online CPU index in each iteration is always valid. Could you explain it more specific?

@Richardhongyu
Copy link
Author

Yes, I will add other similar iterators soon that are for_each_possible_cpu and for_each_present_cpu.

I have added these two functions in this version.

@ojeda
Copy link
Member

ojeda commented Mar 27, 2022

I don't quiet understand "wrap the index in a type that is known to be a valid index". The online CPU index in each iteration is always valid. Could you explain it more specific?

What I meant is that the iterator will return a valid index, but it will be a plain integer. Thus we lose the fact that it is valid in the type system. For instance, it may be the case that the user has to pass that index to something else in order to do something useful with it -- and then the receiver will not know it is a valid index, so it may have to check again (or be unsafe). With a type, we could statically know it is a valid index rather than an unconstrained integer.

This is why I mentioned it would be better to have a complete use case or example, which will show how the index will be used and whether it is worth wrapping the index into its own type or not. Printing the index is fine as a simple example, but it does not show how it will be used.

Thanks for working on the PR, by the way!

@Richardhongyu
Copy link
Author

Thanks for the detailed explanation! I get your point now.

It's exciting to work in this community. I receive much fun and knowledge during this coding phase. :D

@Richardhongyu
Copy link
Author

Sorry for the delay. I'm interrupted by something else. After some struggling, I'm back again:)

The previously left unsolved problems are:

  1. adding the "// SAFETY" comment and
  2. wrapping the index into a known to be valid type.

These two problems are addressed as follows respectively:

  1. When adding the "SAFETY" comment, I find the unsafe block is unsafe. Because of the "HOTPLUG" config, the __cpu_online_mask may change, which can cause a data race. This can be fixed by using the cpus_read_lock. Meanwhile, __cpu_possible_mask will not change after booting, so PossibleCpusIndexIter doesn't need a lock while the OnlineCpusIndexIter and PresentCpusIndexIter need this lock.

  2. I wrap the returning CPU index value of the iterator into a struct named ValidCpuIndex. The name of this struct implies the index is valid. This struct only has a get method and the inside value is not public, therefore the value of it can not be changed. So it is always valid during the iteration of the CPU index iterator.

This commit add a function return a struct implementing Iterator trait. Thus, this struct can be used as an iterator that returns an online cpu index each time.

Signed-off-by: Li Hongyu <lihongyu1999@bupt.edu.cn>
@Richardhongyu Richardhongyu reopened this Jul 27, 2022
@Richardhongyu
Copy link
Author

Hi, @ojeda. I just rebase this PR on the lasted commit. BTW, Is there anything I can do to advance this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants