Re: [PATCH 1/2] intel_idle: Add C1 demotion on/off sysfs knob

[Artem Bityutskiy <dedekind1@gmail.com>]

Hi Rafael,

thanks for reply!

On Wed, 2025-02-12 at 21:11 +0100, Rafael J. Wysocki wrote:
> 
> > +static ssize_t c1_demotion_store(struct device *dev,
> > +                                struct device_attribute *attr,
> > +                                const char *buf, size_t count)
> > +{
> > +       int err;
> > +       bool enable;
> > +
> > +       err = kstrtobool(buf, &enable);
> > +       if (err)
> > +               return err;
> > +
> > +       mutex_lock(&c1_demotion_mutex);
> > +       /* Enable/disable C1 demotion on all CPUs */
> > +       on_each_cpu(c1_demotion_toggle, &enable, 1);
> > +       mutex_unlock(&c1_demotion_mutex);
> 
> This is not the only place where MSR_PKG_CST_CONFIG_CONTROL gets
> updated.  The other one is drivers/platform/x86/intel/pmc/cnp.c
> 
> There is no real conflict because the PMC core thing happens during
> system suspend/resume on client platforms, but this is kind of
> duplicated code.  Any chance to consolidate this?

Thanks for pointing this out.

I'd propose to leave it as is because it is such a small amount of
duplication and also trivial. Since the two code paths do not interfere
with each other, I am not sure consolidation is worth it in this case.

I was also hoping that this patch would be backported by OS vendors,
because it makes a very significant difference on recent Intel
server platforms, so I wanted to keep it simple for easier backporting.

But I did not dare to CC stable, because it may not be perceived as fix.
However, in practice enabling C1 demotion fixes performance issues on
recent Xeons in some workloads.

If you really think the MSR read and write code should be consolidated,
I would propose to do this as a separate patch-set on top, so this one
stays simple and easier to backport.

To recap:

* I propose not to consolidate it.
* If you insist, I propose to do it on top of this one.

Please, let me know.

> > +static ssize_t c1_demotion_show(struct device *dev,
> > +                               struct device_attribute *attr, char *buf)
> > +{
> > +       unsigned long long msr_val;
> > +
> > +       rdmsrl(MSR_PKG_CST_CONFIG_CONTROL, msr_val);
> > +       return sysfs_emit(buf, "%d\n", !!(msr_val & NHM_C1_AUTO_DEMOTE));
> 
> This reads the register on the current CPU with the assumption that
> the specific bit value will be the same for all CPUs.  Is this always
> true?

Yes, anything else would be either a BIOS bug or a user toggling the MSR
bits directly via /dev/msr. I was trying to keep the driver simple and
avoid detecting different values, because the only think we could do in
that case is just print a warning.

>   What about systems with more than one package?  Do they always
> initialize this bit to the same value in all packages?

Yes, anything else would be a misconfiguration or bug.

>   I guess so,
> but then I would add a comment documenting this assumption and the
> reasons for it.

I'll add, thanks!

> I'm not sure if the attr name is clear enough.  I guess reading the
> doc is really necessary to get an idea of what this is about, but it
> might indicate that the demotion is done in hardware, like
> "hw_c1_demotion".

Well, let's see. I propose two approaches for naming this attribute.

1. Follow the Intel name, documented in the Intel SDM: c1_demotion.
2. Try to come up with a better and more intuitive name. I would suggest one of
these:
* demotion_to_c1
* auto_demotion_to_c1

I followed approach #1. I agree that C1 demotion is not an intuitive name,
because it sounds like C1 is being demoted. But I am used to it, and it is also
old and documented in the SDM.

However, I can rename it if you direct me this way. In that case, I would
propose one of the above two. Please, let me know.

Thank you!