| Message ID | 20230614180253.89958-5-jandryuk@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Intel Hardware P-States (HWP) support | expand |
On 14.06.2023 20:02, Jason Andryuk wrote: > --- a/tools/include/xenctrl.h > +++ b/tools/include/xenctrl.h > @@ -1909,16 +1909,20 @@ struct xc_get_cpufreq_para { > uint32_t cpuinfo_cur_freq; > uint32_t cpuinfo_max_freq; > uint32_t cpuinfo_min_freq; > - uint32_t scaling_cur_freq; > - > - char scaling_governor[CPUFREQ_NAME_LEN]; > - uint32_t scaling_max_freq; > - uint32_t scaling_min_freq; > - > - /* for specific governor */ > union { > - xc_userspace_t userspace; > - xc_ondemand_t ondemand; > + struct { > + uint32_t scaling_cur_freq; > + > + char scaling_governor[CPUFREQ_NAME_LEN]; > + uint32_t scaling_max_freq; > + uint32_t scaling_min_freq; > + > + /* for specific governor */ > + union { > + xc_userspace_t userspace; > + xc_ondemand_t ondemand; > + } u; > + } s; > } u; There's no comment in the header that this needs to mirror the sysctl struct. Does it really need changing? > --- a/tools/libs/ctrl/xc_pm.c > +++ b/tools/libs/ctrl/xc_pm.c > @@ -265,15 +265,10 @@ int xc_get_cpufreq_para(xc_interface *xch, int cpuid, > user_para->cpuinfo_cur_freq = sys_para->cpuinfo_cur_freq; > user_para->cpuinfo_max_freq = sys_para->cpuinfo_max_freq; > user_para->cpuinfo_min_freq = sys_para->cpuinfo_min_freq; > - user_para->scaling_cur_freq = sys_para->scaling_cur_freq; > - user_para->scaling_max_freq = sys_para->scaling_max_freq; > - user_para->scaling_min_freq = sys_para->scaling_min_freq; > user_para->turbo_enabled = sys_para->turbo_enabled; > > memcpy(user_para->scaling_driver, > sys_para->scaling_driver, CPUFREQ_NAME_LEN); > - memcpy(user_para->scaling_governor, > - sys_para->scaling_governor, CPUFREQ_NAME_LEN); Did you really mean to remove the copying of these 4 entities, rather than simply change the way the fields are accessed? Jan
On Thu, Jun 15, 2023 at 9:29 AM Jan Beulich <jbeulich@suse.com> wrote: > > On 14.06.2023 20:02, Jason Andryuk wrote: > > --- a/tools/include/xenctrl.h > > +++ b/tools/include/xenctrl.h > > @@ -1909,16 +1909,20 @@ struct xc_get_cpufreq_para { > > uint32_t cpuinfo_cur_freq; > > uint32_t cpuinfo_max_freq; > > uint32_t cpuinfo_min_freq; > > - uint32_t scaling_cur_freq; > > - > > - char scaling_governor[CPUFREQ_NAME_LEN]; > > - uint32_t scaling_max_freq; > > - uint32_t scaling_min_freq; > > - > > - /* for specific governor */ > > union { > > - xc_userspace_t userspace; > > - xc_ondemand_t ondemand; > > + struct { > > + uint32_t scaling_cur_freq; > > + > > + char scaling_governor[CPUFREQ_NAME_LEN]; > > + uint32_t scaling_max_freq; > > + uint32_t scaling_min_freq; > > + > > + /* for specific governor */ > > + union { > > + xc_userspace_t userspace; > > + xc_ondemand_t ondemand; > > + } u; > > + } s; > > } u; > > There's no comment in the header that this needs to mirror the sysctl > struct. Does it really need changing? Since this matched the other structure, I kept them in sync. The cppc/hwp data needs to be represented somehow, and it gets introduced in the same way for both later. If this doesn't get the new nested struct, then maybe fields could be placed into the single union. That would grow the overall struct and have unused fields for hwp. > > --- a/tools/libs/ctrl/xc_pm.c > > +++ b/tools/libs/ctrl/xc_pm.c > > @@ -265,15 +265,10 @@ int xc_get_cpufreq_para(xc_interface *xch, int cpuid, > > user_para->cpuinfo_cur_freq = sys_para->cpuinfo_cur_freq; > > user_para->cpuinfo_max_freq = sys_para->cpuinfo_max_freq; > > user_para->cpuinfo_min_freq = sys_para->cpuinfo_min_freq; > > - user_para->scaling_cur_freq = sys_para->scaling_cur_freq; > > - user_para->scaling_max_freq = sys_para->scaling_max_freq; > > - user_para->scaling_min_freq = sys_para->scaling_min_freq; > > user_para->turbo_enabled = sys_para->turbo_enabled; > > > > memcpy(user_para->scaling_driver, > > sys_para->scaling_driver, CPUFREQ_NAME_LEN); > > - memcpy(user_para->scaling_governor, > > - sys_para->scaling_governor, CPUFREQ_NAME_LEN); > > Did you really mean to remove the copying of these 4 entities, rather > than simply change the way the fields are accessed? Yes, it was intentional. The immediate following lines are: /* copy to user_para no matter what cpufreq governor */ BUILD_BUG_ON(sizeof(((struct xc_get_cpufreq_para *)0)->u) != sizeof(((struct xen_get_cpufreq_para *)0)->u)); memcpy(&user_para->u, &sys_para->u, sizeof(sys_para->u)); And this memcpy copies all the moved entities. I suppose the comment could change to "...no matter which cpufreq driver". Regards, Jason
On 15.06.2023 16:07, Jason Andryuk wrote: > On Thu, Jun 15, 2023 at 9:29 AM Jan Beulich <jbeulich@suse.com> wrote: >> >> On 14.06.2023 20:02, Jason Andryuk wrote: >>> --- a/tools/include/xenctrl.h >>> +++ b/tools/include/xenctrl.h >>> @@ -1909,16 +1909,20 @@ struct xc_get_cpufreq_para { >>> uint32_t cpuinfo_cur_freq; >>> uint32_t cpuinfo_max_freq; >>> uint32_t cpuinfo_min_freq; >>> - uint32_t scaling_cur_freq; >>> - >>> - char scaling_governor[CPUFREQ_NAME_LEN]; >>> - uint32_t scaling_max_freq; >>> - uint32_t scaling_min_freq; >>> - >>> - /* for specific governor */ >>> union { >>> - xc_userspace_t userspace; >>> - xc_ondemand_t ondemand; >>> + struct { >>> + uint32_t scaling_cur_freq; >>> + >>> + char scaling_governor[CPUFREQ_NAME_LEN]; >>> + uint32_t scaling_max_freq; >>> + uint32_t scaling_min_freq; >>> + >>> + /* for specific governor */ >>> + union { >>> + xc_userspace_t userspace; >>> + xc_ondemand_t ondemand; >>> + } u; >>> + } s; >>> } u; >> >> There's no comment in the header that this needs to mirror the sysctl >> struct. Does it really need changing? > > Since this matched the other structure, I kept them in sync. The > cppc/hwp data needs to be represented somehow, and it gets introduced > in the same way for both later. If this doesn't get the new nested > struct, then maybe fields could be placed into the single union. That > would grow the overall struct and have unused fields for hwp. I guess I need to leave this to the maintainers then. Still ... >>> --- a/tools/libs/ctrl/xc_pm.c >>> +++ b/tools/libs/ctrl/xc_pm.c >>> @@ -265,15 +265,10 @@ int xc_get_cpufreq_para(xc_interface *xch, int cpuid, >>> user_para->cpuinfo_cur_freq = sys_para->cpuinfo_cur_freq; >>> user_para->cpuinfo_max_freq = sys_para->cpuinfo_max_freq; >>> user_para->cpuinfo_min_freq = sys_para->cpuinfo_min_freq; >>> - user_para->scaling_cur_freq = sys_para->scaling_cur_freq; >>> - user_para->scaling_max_freq = sys_para->scaling_max_freq; >>> - user_para->scaling_min_freq = sys_para->scaling_min_freq; >>> user_para->turbo_enabled = sys_para->turbo_enabled; >>> >>> memcpy(user_para->scaling_driver, >>> sys_para->scaling_driver, CPUFREQ_NAME_LEN); >>> - memcpy(user_para->scaling_governor, >>> - sys_para->scaling_governor, CPUFREQ_NAME_LEN); >> >> Did you really mean to remove the copying of these 4 entities, rather >> than simply change the way the fields are accessed? > > Yes, it was intentional. > > The immediate following lines are: > /* copy to user_para no matter what cpufreq governor */ > BUILD_BUG_ON(sizeof(((struct xc_get_cpufreq_para *)0)->u) != > sizeof(((struct xen_get_cpufreq_para *)0)->u)); > > memcpy(&user_para->u, &sys_para->u, sizeof(sys_para->u)); ... this suggests that some matching is intended, yet it's not clear to me why then the hole struct-s aren't assumed to be matching / made match. > And this memcpy copies all the moved entities. Right, I should have gone to the source instead of going just from patch context, sorry. > I suppose the comment could change to "...no matter which cpufreq driver". Yeah, well, it really would be driver/governor then, I guess. Jan
On Thu, Jun 15, 2023 at 10:29 AM Jan Beulich <jbeulich@suse.com> wrote: > > On 15.06.2023 16:07, Jason Andryuk wrote: > > On Thu, Jun 15, 2023 at 9:29 AM Jan Beulich <jbeulich@suse.com> wrote: > >> > >> On 14.06.2023 20:02, Jason Andryuk wrote: > >>> --- a/tools/include/xenctrl.h > >>> +++ b/tools/include/xenctrl.h > >>> @@ -1909,16 +1909,20 @@ struct xc_get_cpufreq_para { > >>> uint32_t cpuinfo_cur_freq; > >>> uint32_t cpuinfo_max_freq; > >>> uint32_t cpuinfo_min_freq; > >>> - uint32_t scaling_cur_freq; > >>> - > >>> - char scaling_governor[CPUFREQ_NAME_LEN]; > >>> - uint32_t scaling_max_freq; > >>> - uint32_t scaling_min_freq; > >>> - > >>> - /* for specific governor */ > >>> union { > >>> - xc_userspace_t userspace; > >>> - xc_ondemand_t ondemand; > >>> + struct { > >>> + uint32_t scaling_cur_freq; > >>> + > >>> + char scaling_governor[CPUFREQ_NAME_LEN]; > >>> + uint32_t scaling_max_freq; > >>> + uint32_t scaling_min_freq; > >>> + > >>> + /* for specific governor */ > >>> + union { > >>> + xc_userspace_t userspace; > >>> + xc_ondemand_t ondemand; > >>> + } u; > >>> + } s; > >>> } u; > >> > >> There's no comment in the header that this needs to mirror the sysctl > >> struct. Does it really need changing? > > > > Since this matched the other structure, I kept them in sync. The > > cppc/hwp data needs to be represented somehow, and it gets introduced > > in the same way for both later. If this doesn't get the new nested > > struct, then maybe fields could be placed into the single union. That > > would grow the overall struct and have unused fields for hwp. > > I guess I need to leave this to the maintainers then. Still ... > > >>> --- a/tools/libs/ctrl/xc_pm.c > >>> +++ b/tools/libs/ctrl/xc_pm.c > >>> @@ -265,15 +265,10 @@ int xc_get_cpufreq_para(xc_interface *xch, int cpuid, > >>> user_para->cpuinfo_cur_freq = sys_para->cpuinfo_cur_freq; > >>> user_para->cpuinfo_max_freq = sys_para->cpuinfo_max_freq; > >>> user_para->cpuinfo_min_freq = sys_para->cpuinfo_min_freq; > >>> - user_para->scaling_cur_freq = sys_para->scaling_cur_freq; > >>> - user_para->scaling_max_freq = sys_para->scaling_max_freq; > >>> - user_para->scaling_min_freq = sys_para->scaling_min_freq; > >>> user_para->turbo_enabled = sys_para->turbo_enabled; > >>> > >>> memcpy(user_para->scaling_driver, > >>> sys_para->scaling_driver, CPUFREQ_NAME_LEN); > >>> - memcpy(user_para->scaling_governor, > >>> - sys_para->scaling_governor, CPUFREQ_NAME_LEN); > >> > >> Did you really mean to remove the copying of these 4 entities, rather > >> than simply change the way the fields are accessed? > > > > Yes, it was intentional. > > > > The immediate following lines are: > > /* copy to user_para no matter what cpufreq governor */ > > BUILD_BUG_ON(sizeof(((struct xc_get_cpufreq_para *)0)->u) != > > sizeof(((struct xen_get_cpufreq_para *)0)->u)); > > > > memcpy(&user_para->u, &sys_para->u, sizeof(sys_para->u)); > > ... this suggests that some matching is intended, yet it's not clear > to me why then the hole struct-s aren't assumed to be matching / made > match. The tools version replaces struct xen_$foo with xc_$foo typedefs. Maybe it's just to enforce the typedefs? Regards, Jason
diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h index dba33d5d0f..8aedb952a0 100644 --- a/tools/include/xenctrl.h +++ b/tools/include/xenctrl.h @@ -1909,16 +1909,20 @@ struct xc_get_cpufreq_para { uint32_t cpuinfo_cur_freq; uint32_t cpuinfo_max_freq; uint32_t cpuinfo_min_freq; - uint32_t scaling_cur_freq; - - char scaling_governor[CPUFREQ_NAME_LEN]; - uint32_t scaling_max_freq; - uint32_t scaling_min_freq; - - /* for specific governor */ union { - xc_userspace_t userspace; - xc_ondemand_t ondemand; + struct { + uint32_t scaling_cur_freq; + + char scaling_governor[CPUFREQ_NAME_LEN]; + uint32_t scaling_max_freq; + uint32_t scaling_min_freq; + + /* for specific governor */ + union { + xc_userspace_t userspace; + xc_ondemand_t ondemand; + } u; + } s; } u; int32_t turbo_enabled; diff --git a/tools/libs/ctrl/xc_pm.c b/tools/libs/ctrl/xc_pm.c index c3a9864bf7..f92542eaf7 100644 --- a/tools/libs/ctrl/xc_pm.c +++ b/tools/libs/ctrl/xc_pm.c @@ -265,15 +265,10 @@ int xc_get_cpufreq_para(xc_interface *xch, int cpuid, user_para->cpuinfo_cur_freq = sys_para->cpuinfo_cur_freq; user_para->cpuinfo_max_freq = sys_para->cpuinfo_max_freq; user_para->cpuinfo_min_freq = sys_para->cpuinfo_min_freq; - user_para->scaling_cur_freq = sys_para->scaling_cur_freq; - user_para->scaling_max_freq = sys_para->scaling_max_freq; - user_para->scaling_min_freq = sys_para->scaling_min_freq; user_para->turbo_enabled = sys_para->turbo_enabled; memcpy(user_para->scaling_driver, sys_para->scaling_driver, CPUFREQ_NAME_LEN); - memcpy(user_para->scaling_governor, - sys_para->scaling_governor, CPUFREQ_NAME_LEN); /* copy to user_para no matter what cpufreq governor */ BUILD_BUG_ON(sizeof(((struct xc_get_cpufreq_para *)0)->u) != diff --git a/tools/misc/xenpm.c b/tools/misc/xenpm.c index 1bb6187e56..ee8ce5d5f2 100644 --- a/tools/misc/xenpm.c +++ b/tools/misc/xenpm.c @@ -730,39 +730,39 @@ static void print_cpufreq_para(int cpuid, struct xc_get_cpufreq_para *p_cpufreq) printf("scaling_avail_gov : %s\n", p_cpufreq->scaling_available_governors); - printf("current_governor : %s\n", p_cpufreq->scaling_governor); - if ( !strncmp(p_cpufreq->scaling_governor, + printf("current_governor : %s\n", p_cpufreq->u.s.scaling_governor); + if ( !strncmp(p_cpufreq->u.s.scaling_governor, "userspace", CPUFREQ_NAME_LEN) ) { printf(" userspace specific :\n"); printf(" scaling_setspeed : %u\n", - p_cpufreq->u.userspace.scaling_setspeed); + p_cpufreq->u.s.u.userspace.scaling_setspeed); } - else if ( !strncmp(p_cpufreq->scaling_governor, + else if ( !strncmp(p_cpufreq->u.s.scaling_governor, "ondemand", CPUFREQ_NAME_LEN) ) { printf(" ondemand specific :\n"); printf(" sampling_rate : max [%u] min [%u] cur [%u]\n", - p_cpufreq->u.ondemand.sampling_rate_max, - p_cpufreq->u.ondemand.sampling_rate_min, - p_cpufreq->u.ondemand.sampling_rate); + p_cpufreq->u.s.u.ondemand.sampling_rate_max, + p_cpufreq->u.s.u.ondemand.sampling_rate_min, + p_cpufreq->u.s.u.ondemand.sampling_rate); printf(" up_threshold : %u\n", - p_cpufreq->u.ondemand.up_threshold); + p_cpufreq->u.s.u.ondemand.up_threshold); } printf("scaling_avail_freq :"); for ( i = 0; i < p_cpufreq->freq_num; i++ ) if ( p_cpufreq->scaling_available_frequencies[i] == - p_cpufreq->scaling_cur_freq ) + p_cpufreq->u.s.scaling_cur_freq ) printf(" *%d", p_cpufreq->scaling_available_frequencies[i]); else printf(" %d", p_cpufreq->scaling_available_frequencies[i]); printf("\n"); printf("scaling frequency : max [%u] min [%u] cur [%u]\n", - p_cpufreq->scaling_max_freq, - p_cpufreq->scaling_min_freq, - p_cpufreq->scaling_cur_freq); + p_cpufreq->u.s.scaling_max_freq, + p_cpufreq->u.s.scaling_min_freq, + p_cpufreq->u.s.scaling_cur_freq); printf("turbo mode : %s\n", p_cpufreq->turbo_enabled ? "enabled" : "disabled or n/a"); diff --git a/xen/drivers/acpi/pmstat.c b/xen/drivers/acpi/pmstat.c index 1bae635101..f5a9ac3f1a 100644 --- a/xen/drivers/acpi/pmstat.c +++ b/xen/drivers/acpi/pmstat.c @@ -258,37 +258,38 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op *op) cpufreq_driver.get ? cpufreq_driver.get(op->cpuid) : policy->cur; op->u.get_para.cpuinfo_max_freq = policy->cpuinfo.max_freq; op->u.get_para.cpuinfo_min_freq = policy->cpuinfo.min_freq; - op->u.get_para.scaling_cur_freq = policy->cur; - op->u.get_para.scaling_max_freq = policy->max; - op->u.get_para.scaling_min_freq = policy->min; + + op->u.get_para.u.s.scaling_cur_freq = policy->cur; + op->u.get_para.u.s.scaling_max_freq = policy->max; + op->u.get_para.u.s.scaling_min_freq = policy->min; if ( cpufreq_driver.name[0] ) - strlcpy(op->u.get_para.scaling_driver, + strlcpy(op->u.get_para.scaling_driver, cpufreq_driver.name, CPUFREQ_NAME_LEN); else strlcpy(op->u.get_para.scaling_driver, "Unknown", CPUFREQ_NAME_LEN); if ( policy->governor->name[0] ) - strlcpy(op->u.get_para.scaling_governor, + strlcpy(op->u.get_para.u.s.scaling_governor, policy->governor->name, CPUFREQ_NAME_LEN); else - strlcpy(op->u.get_para.scaling_governor, "Unknown", CPUFREQ_NAME_LEN); + strlcpy(op->u.get_para.u.s.scaling_governor, "Unknown", CPUFREQ_NAME_LEN); /* governor specific para */ - if ( !strncasecmp(op->u.get_para.scaling_governor, + if ( !strncasecmp(op->u.get_para.u.s.scaling_governor, "userspace", CPUFREQ_NAME_LEN) ) { - op->u.get_para.u.userspace.scaling_setspeed = policy->cur; + op->u.get_para.u.s.u.userspace.scaling_setspeed = policy->cur; } - if ( !strncasecmp(op->u.get_para.scaling_governor, + if ( !strncasecmp(op->u.get_para.u.s.scaling_governor, "ondemand", CPUFREQ_NAME_LEN) ) { ret = get_cpufreq_ondemand_para( - &op->u.get_para.u.ondemand.sampling_rate_max, - &op->u.get_para.u.ondemand.sampling_rate_min, - &op->u.get_para.u.ondemand.sampling_rate, - &op->u.get_para.u.ondemand.up_threshold); + &op->u.get_para.u.s.u.ondemand.sampling_rate_max, + &op->u.get_para.u.s.u.ondemand.sampling_rate_min, + &op->u.get_para.u.s.u.ondemand.sampling_rate, + &op->u.get_para.u.s.u.ondemand.up_threshold); } op->u.get_para.turbo_enabled = cpufreq_get_turbo_status(op->cpuid); diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h index 9d06e92d0f..bdcea99d71 100644 --- a/xen/include/public/sysctl.h +++ b/xen/include/public/sysctl.h @@ -317,16 +317,20 @@ struct xen_get_cpufreq_para { uint32_t cpuinfo_cur_freq; uint32_t cpuinfo_max_freq; uint32_t cpuinfo_min_freq; - uint32_t scaling_cur_freq; - - char scaling_governor[CPUFREQ_NAME_LEN]; - uint32_t scaling_max_freq; - uint32_t scaling_min_freq; - - /* for specific governor */ union { - struct xen_userspace userspace; - struct xen_ondemand ondemand; + struct { + uint32_t scaling_cur_freq; + + char scaling_governor[CPUFREQ_NAME_LEN]; + uint32_t scaling_max_freq; + uint32_t scaling_min_freq; + + /* for specific governor */ + union { + struct xen_userspace userspace; + struct xen_ondemand ondemand; + } u; + } s; } u; int32_t turbo_enabled;
Add a union and struct so that most of the scaling variables of struct xen_get_cpufreq_para are within in a binary-compatible layout. This allows cppc_para to live in the larger union and use uint32_ts - struct xen_cppc_para will be 10 uint32_t's. The new scaling struct is 3 * uint32_t + 16 bytes CPUFREQ_NAME_LEN + 4 * uint32_t for xen_ondemand = 11 uint32_t. That means the old size is retained, int32_t turbo_enabled doesn't move and it's binary compatible. Signed-off-by: Jason Andryuk <jandryuk@gmail.com> --- tools/include/xenctrl.h | 22 +++++++++++++--------- tools/libs/ctrl/xc_pm.c | 5 ----- tools/misc/xenpm.c | 24 ++++++++++++------------ xen/drivers/acpi/pmstat.c | 27 ++++++++++++++------------- xen/include/public/sysctl.h | 22 +++++++++++++--------- 5 files changed, 52 insertions(+), 48 deletions(-)