Re: [PATCH V2 01/10] x86/CPU: Expose if cache is inclusive of lower level caches

[Reinette Chatre]

Hi Borislav,

On 8/6/2019 8:57 AM, Borislav Petkov wrote:
> On Mon, Aug 05, 2019 at 10:57:04AM -0700, Reinette Chatre wrote:
>> What do you think?
> 
> Actually, I was thinking about something a lot simpler: something
> along the lines of adding the CPUID check in a helper function which
> rdt_pseudo_lock_init() calls. If the cache is not inclusive - and my
> guess is it would suffice to check any cache but I'd prefer you correct
> me on that - you simply return error and rdt_pseudo_lock_init() returns
> early without doing any futher init.
> 
> How does that sound?

I am a bit cautious about this. When I started this work I initially
added a helper function to resctrl that calls CPUID to determine if the
cache is inclusive. At that time I became aware of a discussion
motivating against scattered CPUID calls and motivating for one instance
of CPUID information:
http://lkml.kernel.org/r/alpine.DEB.2.21.1906162141301.1760@xxxxxxxxxxxxxxxxxxxxxxx

My interpretation of the above resulted in a move away from calling
CPUID in resctrl to the patch you are reviewing now.

I do indeed prefer a simple approach and would change to what you
suggest if you find it to be best.

To answer your question about checking any cache: this seems to be
different between L2 and L3. On the Atom systems where L2 pseudo-locking
works well the L2 cache is not inclusive. We are also working on
supporting cache pseudo-locking on L3 cache that is not inclusive.

I could add the single CPUID check during rdt_pseudo_lock_init(),
checking on any CPU should work. I think it would be simpler (reasons
below) to initialize that single system-wide setting in
rdt_pseudo_lock_init() and keep the result locally in the pseudo-locking
code, that can be referred to when the user requests the pseudo-locked
region.

Simpler for two reasons:
* Prevent future platform specific code within rdt_pseudo_lock_init()
trying to pick when L3 is allowed to be inclusive or not.
* rdt_pseudo_lock_init() does not currently make decision whether
platform supports pseudo-locking - this is currently done when user
requests a pseudo-lock region. rdt_pseudo_lock_init() does
initialization of things that should not fail and for which resctrl's
mount should not proceed if it fails. A platform not supporting
pseudo-locking should not prevent resctrl from being mounted and used
for cache allocation.

Thank you very much

Reinette