Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1584455 > unrolled thread

Re: [RFC][PATCH] cpufreq: User/admin documentation update and consolidation

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC][PATCH] cpufreq: User/admin documentation update and  consolidation Viresh Kumar <viresh.kumar@linaro.org> - 2017-02-20 11:00 +0100
    Re: [RFC][PATCH] cpufreq: User/admin documentation update and consolidation "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-02-20 15:10 +0100
      Re: [RFC][PATCH] cpufreq: User/admin documentation update and consolidation "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-02-21 01:00 +0100
      Re: [RFC][PATCH] cpufreq: User/admin documentation update and  consolidation Viresh Kumar <viresh.kumar@linaro.org> - 2017-02-21 06:10 +0100
        Re: [RFC][PATCH] cpufreq: User/admin documentation update and consolidation "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-02-22 02:00 +0100

#1584455 — Re: [RFC][PATCH] cpufreq: User/admin documentation update and consolidation

On 18-02-17, 02:36, Rafael J. Wysocki wrote:
> +CPU Initialization
> +==================
> +


> +Next, the scaling driver's ``->init()`` callback is invoked with the policy
> +pointer of the new CPU passed to it as the argument.  If the policy object
> +pointed to by it is new

The callbacks don't need to do anything special for a new policy.
Infact, ->init() is only called for new policies or policies which
don't have any active CPUs as of now.

> , that callback is expected to initialize the performance
> +scaling hardware interface for the given CPU (or, more precisely, for the set of
> +CPUs sharing the hardware interface it belongs to, represented by its policy
> +object) and to set parameters of the policy, like the minimum and maximum
> +frequencies supported by the hardware, the table of available frequencies (if
> +the set of supported P-states is not a continuous range), and the mask of CPUs
> +that belong to the same policy.

Maybe we should explicitly mention that both online and offline CPUs
should be set in the mask ?

> That mask is then used by the core to populate
> +the policy pointers for all of the CPUs in it.
> +
> +The next major initialization step for a new policy object is to attach a
> +scaling governor to it (to begin with, that is the default scaling governor
> +determined by the kernel configuration, but it may be changed later
> +via ``sysfs``).  First, a pointer to the new policy object is passed to the
> +governor's ``->init()`` callback which is expected to initialize all of the
> +data structures necessary to handle the given policy and, possibly, to add
> +a governor ``sysfs`` interface to it.  Next, the governor is started by
> +invoking its ``->start()`` callback.

The rest of it looked good. Nice work Rafael :)

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

[toc] | [next] | [standalone]

#1584636 — Re: [RFC][PATCH] cpufreq: User/admin documentation update and consolidation

On Monday, February 20, 2017 03:26:08 PM Viresh Kumar wrote:
> On 18-02-17, 02:36, Rafael J. Wysocki wrote:
> > +CPU Initialization
> > +==================
> > +
> 
> 
> > +Next, the scaling driver's ``->init()`` callback is invoked with the policy
> > +pointer of the new CPU passed to it as the argument.  If the policy object
> > +pointed to by it is new
> 
> The callbacks don't need to do anything special for a new policy.
> Infact, ->init() is only called for new policies or policies which
> don't have any active CPUs as of now.

Yes, it is called for new and inactive policies.

For new policies it has to populate policy->cpus (because otherwise the core
doesn't know what CPUs should be there), which quite arguably doesn't have
to (or even doesn't need to) be done for inactive policies (because they already
have policy->real_cpus and policy->related_cpus populated).

I would even argue that ->init() should not update policy->cpus for inactive
(but not new) policies.

> > , that callback is expected to initialize the performance
> > +scaling hardware interface for the given CPU (or, more precisely, for the set of
> > +CPUs sharing the hardware interface it belongs to, represented by its policy
> > +object) and to set parameters of the policy, like the minimum and maximum
> > +frequencies supported by the hardware, the table of available frequencies (if
> > +the set of supported P-states is not a continuous range), and the mask of CPUs
> > +that belong to the same policy.
> 
> Maybe we should explicitly mention that both online and offline CPUs
> should be set in the mask ?

I can do that.

But offline CPUs can only be set in that mask if the policy is actually new.

> > That mask is then used by the core to populate
> > +the policy pointers for all of the CPUs in it.
> > +
> > +The next major initialization step for a new policy object is to attach a
> > +scaling governor to it (to begin with, that is the default scaling governor
> > +determined by the kernel configuration, but it may be changed later
> > +via ``sysfs``).  First, a pointer to the new policy object is passed to the
> > +governor's ``->init()`` callback which is expected to initialize all of the
> > +data structures necessary to handle the given policy and, possibly, to add
> > +a governor ``sysfs`` interface to it.  Next, the governor is started by
> > +invoking its ``->start()`` callback.
> 
> The rest of it looked good. Nice work Rafael :)
> 
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

Thanks!

Cheers,
Rafael

[toc] | [prev] | [next] | [standalone]

#1584965 — Re: [RFC][PATCH] cpufreq: User/admin documentation update and consolidation

On Monday, February 20, 2017 02:58:27 PM Rafael J. Wysocki wrote:
> On Monday, February 20, 2017 03:26:08 PM Viresh Kumar wrote:
> > On 18-02-17, 02:36, Rafael J. Wysocki wrote:
> > > +CPU Initialization
> > > +==================
> > > +
> > 
> > 
> > > +Next, the scaling driver's ``->init()`` callback is invoked with the policy
> > > +pointer of the new CPU passed to it as the argument.  If the policy object
> > > +pointed to by it is new
> > 
> > The callbacks don't need to do anything special for a new policy.
> > Infact, ->init() is only called for new policies or policies which
> > don't have any active CPUs as of now.
> 
> Yes, it is called for new and inactive policies.
> 
> For new policies it has to populate policy->cpus (because otherwise the core
> doesn't know what CPUs should be there), which quite arguably doesn't have
> to (or even doesn't need to) be done for inactive policies (because they already
> have policy->real_cpus and policy->related_cpus populated).
> 
> I would even argue that ->init() should not update policy->cpus for inactive
> (but not new) policies.
> 
> > > , that callback is expected to initialize the performance
> > > +scaling hardware interface for the given CPU (or, more precisely, for the set of
> > > +CPUs sharing the hardware interface it belongs to, represented by its policy
> > > +object) and to set parameters of the policy, like the minimum and maximum
> > > +frequencies supported by the hardware, the table of available frequencies (if
> > > +the set of supported P-states is not a continuous range), and the mask of CPUs
> > > +that belong to the same policy.
> > 
> > Maybe we should explicitly mention that both online and offline CPUs
> > should be set in the mask ?
> 
> I can do that.
> 
> But offline CPUs can only be set in that mask if the policy is actually new.

Well, drivers actually always set them, so let me put it differently.

It is not very useful to update a policy that previously was inactive from
->init(), because the related_cpus mask is only populated if the policy is new
and the symbolic links from CPU directories in sysfs to it are only created
once etc.

So it would be OK to skip updating the policy in ->init() in that case.

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1585045

On 20-02-17, 14:58, Rafael J. Wysocki wrote:
> Yes, it is called for new and inactive policies.
> 
> For new policies it has to populate policy->cpus (because otherwise the core
> doesn't know what CPUs should be there), which quite arguably doesn't have
> to (or even doesn't need to) be done for inactive policies (because they already
> have policy->real_cpus and policy->related_cpus populated).
 
> I would even argue that ->init() should not update policy->cpus for inactive
> (but not new) policies.

I agree, but I am not aware of any platforms that we have today which
does any similar checks in ->init(). And I wouldn't encourage drivers
to have such special handling at all. Why make it complex?

The way you have written it earlier suggests that the drivers should
check if the policy is active or not (by looking at related_cpus mask)
and set the cpus mask only for new policies. I was just worried that
new driver authors will actually try to write special code in init()
for that and if we really want that to be the case.

-- 
viresh

[toc] | [prev] | [next] | [standalone]

#1585832 — Re: [RFC][PATCH] cpufreq: User/admin documentation update and consolidation

On Tuesday, February 21, 2017 10:30:10 AM Viresh Kumar wrote:
> On 20-02-17, 14:58, Rafael J. Wysocki wrote:
> > Yes, it is called for new and inactive policies.
> > 
> > For new policies it has to populate policy->cpus (because otherwise the core
> > doesn't know what CPUs should be there), which quite arguably doesn't have
> > to (or even doesn't need to) be done for inactive policies (because they already
> > have policy->real_cpus and policy->related_cpus populated).
>  
> > I would even argue that ->init() should not update policy->cpus for inactive
> > (but not new) policies.
> 
> I agree, but I am not aware of any platforms that we have today which
> does any similar checks in ->init(). And I wouldn't encourage drivers
> to have such special handling at all. Why make it complex?

Well, IMO we should not invoke the same callback in two different cases and
then do tricks in the core to make one of them work.  That's just too [edited]
ugly.

At least it doesn't help to understand the code flow.

> The way you have written it earlier suggests that the drivers should
> check if the policy is active or not (by looking at related_cpus mask)
> and set the cpus mask only for new policies. I was just worried that
> new driver authors will actually try to write special code in init()
> for that and if we really want that to be the case.

That would not be a bug IMO.  Some of them could just check related_cpus
and do nothing if it is set I suppose?

Anyway, this isn't a driver API document, so the description is not super-precise.

Thanks,
Rafael

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web