Message-ID: <alpine.DEB.2.21.1802202355240.24268@nanos.tec.linutronix.de>
Date:   Wed, 21 Feb 2018 00:21:44 +0100 (CET)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Reinette Chatre <reinette.chatre@...el.com>
cc:     fenghua.yu@...el.com, tony.luck@...el.com, gavin.hindman@...el.com,
        vikas.shivappa@...ux.intel.com, dave.hansen@...el.com,
        mingo@...hat.com, hpa@...or.com, x86@...nel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH V2 13/22] x86/intel_rdt: Support schemata write -
 pseudo-locking core

Reinette,

On Tue, 20 Feb 2018, Reinette Chatre wrote:
> On 2/20/2018 9:15 AM, Thomas Gleixner wrote:
> > On Tue, 13 Feb 2018, Reinette Chatre wrote:
> > 
> > Are you really sure that the life time rules of plr are correct vs. an
> > application which still has the locked memory mapped? i.e. the following
> > operation:
> 
> You are correct. I am not preventing an administrator from removing the
> pseudo-locked region if it is in use. I will fix that.

The removal is fine and you cannot prevent it w/o introducing a mess, but
you have to make sure that the PLR and the mapped memory are not
vanishing. The refcount rules I outlined are exactly doing that.

> Thank you so much for taking the time to do this thorough review and to
> make these suggestions. While I am still digesting the details I do
> intend to follow all (as well as the ones earlier I did not explicitly
> respond to).

Make your mind up and tell me where I'm wrong before you implement the crap
I suggested blindly, as that will just cause the next reviewer (me or
someone else) to tell _you_ that it is crap :)

> Keeping the CLOSID associated with the pseudo-locked region will surely
> make the above simpler since CLOSID's are association with resource
> groups (represented by the directories). I would like to highlight that
> on some platforms there are only a few (for example, 4) CLOSIDs
> available. Not releasing a CLOSID would thus reduce available CLOSIDs
> that are already limited. These platforms do have smaller possible
> bitmasks though (for example, 8 possible bits), which may make light of
> this concern. I thus just add it as informational to the consequence of
> this simplification.

Yes. If you have 4 CLOSIDs and only 8 CBM bits it really does not matter
much.

> > Now the remaining thing is the memory allocation and the mmap itself. I
> > really dislike the preallocation of memory right at setup time. Ideally
> > that should be an allocation of the application itself, but the horrid
> > wbinvd stuff kinda prevents that. With that restriction we are more or less
> > bound to immediate allocation and population.
> 
> Acknowledged. I am not sure if the current permissions would support
> such a dynamic setup though. At this time the system administrator is
> the one that sets up the pseudo-locked region and can through
> permissions of the character device provide access to these regions to
> user space applications.

You still would need some interface, e.g. character device which allows you
to hand in the pointer to the user allocated memory and do the cache
priming. So you could use the same permission setup for that character
device.

The other problem is that we'd need to have MAP_CONTIG first so you
actually can allocate physically contigous memory from user space. Mike is
working on that, but it's not available today. The only way to do so today
(with lots of waste) would be MAP_HUGETLB, which might be an acceptable
constraint up to the point where MAP_CONTIG is available.

Though this all depends on the ability to remove the wbinvd
requirement. But even if we can remove that we'd still need to be aware
that the cache priming loop which needs to run with interrupts disabled is
expensive as well and can introduce undesired latencies. Needs all some
thought...

Thanks,

	tglx

Powered by blists - more mailing lists