cpumask: add a function like for_each_online_cpu

[Richardhongyu]

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 [email protected]

[Richardhongyu]

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

[bjorn3]

(missing Signed-Off-By)

[Richardhongyu]

Thanks for reminding me:)

[ojeda]

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?

[ojeda]

Missing a couple // SAFETY comments.

[Richardhongyu]

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.

[Richardhongyu]

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

[Richardhongyu]

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]

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]

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]

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]

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.

[Richardhongyu]

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