| Message ID | 867ba9728179ba21ff8f8aca97d416b72ccd63d9.1736248242.git.viresh.kumar@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Rust bindings for cpufreq and OPP core + sample driver | expand |
On 07-01-25, 12:59, Greg KH wrote: > On Tue, Jan 07, 2025 at 04:51:35PM +0530, Viresh Kumar wrote: > > Add a function to calculate number of entries in the cpufreq table. This > > will be used by the Rust implementation. > > Again, why is Rust unique here? Why wouldn't the C code also need this? How about this: cpufreq: Add cpufreq_table_len() The last entry of a cpufreq table is marked by setting the frequency field to a special value: CPUFREQ_TABLE_END. The C code manages to traverse the table by checking the frequency field, until it reaches CPUFREQ_TABLE_END. The Rust cpufreq bindings though will need to know the length of the cpufreq table in advance, for example to check against an invalid index value. Provide a helper to calculate number of entries in the cpufreq table. will be used by the Rust implementation. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > > --- > > include/linux/cpufreq.h | 11 +++++++++++ > > 1 file changed, 11 insertions(+) > > > > diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h > > index 7fe0981a7e46..6b882ff4dc24 100644 > > --- a/include/linux/cpufreq.h > > +++ b/include/linux/cpufreq.h > > @@ -783,6 +783,17 @@ bool cpufreq_boost_enabled(void); > > int cpufreq_enable_boost_support(void); > > bool policy_has_boost_freq(struct cpufreq_policy *policy); > > > > +static inline unsigned int cpufreq_table_len(struct cpufreq_frequency_table *freq_table) > > +{ > > + struct cpufreq_frequency_table *pos; > > + unsigned int count = 0; > > + > > + cpufreq_for_each_entry(pos, freq_table) > > + count++; > > No locking is needed? No, the cpufreq table is implemented as an array and is never altered once created.
On Wed, Jan 08, 2025 at 04:42:53PM +0530, Viresh Kumar wrote: > On 07-01-25, 12:59, Greg KH wrote: > > On Tue, Jan 07, 2025 at 04:51:35PM +0530, Viresh Kumar wrote: > > > Add a function to calculate number of entries in the cpufreq table. This > > > will be used by the Rust implementation. > > > > Again, why is Rust unique here? Why wouldn't the C code also need this? > > How about this: > > cpufreq: Add cpufreq_table_len() > > The last entry of a cpufreq table is marked by setting the frequency > field to a special value: CPUFREQ_TABLE_END. The C code manages to > traverse the table by checking the frequency field, until it reaches > CPUFREQ_TABLE_END. > > The Rust cpufreq bindings though will need to know the length of the > cpufreq table in advance, for example to check against an invalid index > value. > > Provide a helper to calculate number of entries in the cpufreq table. > will be used by the Rust implementation. Odd, why can't Rust also know where CPUFREQ_TABLE_END is? Why do you have to do extra work here? That feels wrong. thanks, greg k-h
On 08-01-25, 12:50, Greg KH wrote: > Odd, why can't Rust also know where CPUFREQ_TABLE_END is? Why do you > have to do extra work here? That feels wrong. Well, it can, sure. The freq table the Rust code receives is part of the C code: bindings::cpufreq_frequency_table. Since it is a C managed pointer, I thought it is better to do the parsing in C code itself. In case the implementation of the struct changes in future (unlikely though), we would only need to fix the C code and not Rust, which looks to be the right expectation.
On Thu, Jan 09, 2025 at 10:11:17AM +0530, Viresh Kumar wrote: > On 08-01-25, 12:50, Greg KH wrote: > > Odd, why can't Rust also know where CPUFREQ_TABLE_END is? Why do you > > have to do extra work here? That feels wrong. > > Well, it can, sure. > > The freq table the Rust code receives is part of the C code: > bindings::cpufreq_frequency_table. Since it is a C managed pointer, I thought it > is better to do the parsing in C code itself. In case the implementation of the > struct changes in future (unlikely though), we would only need to fix the C code > and not Rust, which looks to be the right expectation. Then why not make the C code use this function as well, to keep all cpufreq drivers from having to manually walk the list and that way both C and Rust drivers all do the same thing? That makes more sense to me, there's no reason you can't change C code today first to make things more unified, in fact, that's usually a better idea overall anyway. thanks, greg k-h
On 09-01-25, 08:35, Greg KH wrote: > Then why not make the C code use this function as well, to keep all > cpufreq drivers from having to manually walk the list and that way both > C and Rust drivers all do the same thing? That makes more sense to me, > there's no reason you can't change C code today first to make things > more unified, in fact, that's usually a better idea overall anyway. I investigated a bit on this.. - The cpufreq core normally gets (from cpufreq governor's for example) a frequency value to be matched against in the freq-table, and the loop which run over the freq-table is already optimized enough (it checks for CPUFREQ_TABLE_END) for this. Using length in this loop won't improve it anymore. - The cpufreq core then calls cpufreq driver's callbacks and passes an index to the freq-table, which the drivers don't need to verify against table length, since the index came from the core itself. - The same happens on the Rust side, where the cpufreq core calls the target_index() callback of the driver. While writing the Rust code, I thought maybe I should validate that the index is within limits (before I do pointer manipulation in Rust code). And so required this extra function (which C code never uses). - Now I can either keep doing this verification in the Rust code (and so keep the new API, only used by Rust code). Or I can just remove the verification and trust that the index passed by the cpufreq-drivers is correct (since they have received them from the cpufreq C code). What should I do ?
On Mon, Jan 13, 2025 at 01:00:40PM +0530, Viresh Kumar wrote: > On 09-01-25, 08:35, Greg KH wrote: > > Then why not make the C code use this function as well, to keep all > > cpufreq drivers from having to manually walk the list and that way both > > C and Rust drivers all do the same thing? That makes more sense to me, > > there's no reason you can't change C code today first to make things > > more unified, in fact, that's usually a better idea overall anyway. > > I investigated a bit on this.. > > - The cpufreq core normally gets (from cpufreq governor's for example) > a frequency value to be matched against in the freq-table, and the > loop which run over the freq-table is already optimized enough (it > checks for CPUFREQ_TABLE_END) for this. Using length in this loop > won't improve it anymore. > > - The cpufreq core then calls cpufreq driver's callbacks and passes an > index to the freq-table, which the drivers don't need to verify > against table length, since the index came from the core itself. > > - The same happens on the Rust side, where the cpufreq core calls the > target_index() callback of the driver. While writing the Rust code, > I thought maybe I should validate that the index is within limits > (before I do pointer manipulation in Rust code). And so required > this extra function (which C code never uses). > > - Now I can either keep doing this verification in the Rust code (and > so keep the new API, only used by Rust code). Or I can just remove > the verification and trust that the index passed by the > cpufreq-drivers is correct (since they have received them from the > cpufreq C code). > > What should I do ? I would trust the C code, keeping the apis the same between the two types of drivers. If in the future you want to fix up the api, then fix both drivers. Don't try to have unique apis for only Rust drivers if you can help it at all, that is just going to drive us all crazy over time. And really, you might want to fix up the api, that too sounds crazy :) thanks, greg k-h
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index 7fe0981a7e46..6b882ff4dc24 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -783,6 +783,17 @@ bool cpufreq_boost_enabled(void); int cpufreq_enable_boost_support(void); bool policy_has_boost_freq(struct cpufreq_policy *policy); +static inline unsigned int cpufreq_table_len(struct cpufreq_frequency_table *freq_table) +{ + struct cpufreq_frequency_table *pos; + unsigned int count = 0; + + cpufreq_for_each_entry(pos, freq_table) + count++; + + return count; +} + /* Find lowest freq at or above target in a table in ascending order */ static inline int cpufreq_table_find_index_al(struct cpufreq_policy *policy, unsigned int target_freq,
Add a function to calculate number of entries in the cpufreq table. This will be used by the Rust implementation. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- include/linux/cpufreq.h | 11 +++++++++++ 1 file changed, 11 insertions(+)