Date: Fri, 14 Jun 2024 14:57:48 +0100
From: James Morse <james.morse@....com>
To: Dave Martin <Dave.Martin@....com>,
 Reinette Chatre <reinette.chatre@...el.com>
Cc: x86@...nel.org, linux-kernel@...r.kernel.org,
 Fenghua Yu <fenghua.yu@...el.com>, Thomas Gleixner <tglx@...utronix.de>,
 Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
 H Peter Anvin <hpa@...or.com>, Babu Moger <Babu.Moger@....com>,
 shameerali.kolothum.thodi@...wei.com,
 D Scott Phillips OS <scott@...amperecomputing.com>,
 carl@...amperecomputing.com, lcherian@...vell.com,
 bobo.shaobowang@...wei.com, tan.shaopeng@...itsu.com,
 baolin.wang@...ux.alibaba.com, Jamie Iles <quic_jiles@...cinc.com>,
 Xin Hao <xhao@...ux.alibaba.com>, peternewman@...gle.com,
 dfustini@...libre.com, amitsinght@...vell.com,
 David Hildenbrand <david@...hat.com>, Rex Nie <rex.nie@...uarmicro.com>
Subject: Re: [PATCH v1 17/31] x86/resctrl: Move mbm_cfg_mask to struct
 rdt_resource

Hi guys,

On 18/04/2024 16:26, Dave Martin wrote:
> On Wed, Apr 17, 2024 at 10:18:48PM -0700, Reinette Chatre wrote:
>> On 4/17/2024 7:41 AM, Dave Martin wrote:
>>> On Thu, Apr 11, 2024 at 10:39:06AM -0700, Reinette Chatre wrote:
>>>> On 4/11/2024 7:16 AM, Dave Martin wrote:
>>>>> On Mon, Apr 08, 2024 at 08:21:24PM -0700, Reinette Chatre wrote:
>>>>>> On 3/21/2024 9:50 AM, James Morse wrote:
>>>>>>> The mbm_cfg_mask field lists the bits that user-space can set when
>>>>>>> configuring an event. This value is output via the last_cmd_status
>>>>>>> file.
>>>>>>>
>>>>>>> Once the filesystem parts of resctrl are moved to live in /fs/, the
>>>>>>> struct rdt_hw_resource is inaccessible to the filesystem code. Because
>>>>>>> this value is output to user-space, it has to be accessible to the
>>>>>>> filesystem code.
>>>>>>>
>>>>>>> Move it to struct rdt_resource.

>>> Maybe, but the bits as defined by AMD BMEC look rather architecture and
>>> bus specific, and I am suspicious that there is no guaranteed clean
>>> mapping between MPAM's config and BMEC's config.
>>>
>>> MPAM currently just has "reads" and "writes" (or both), though as for
>>> BMEC, the meanings of these are not fully defined.  I suppose finer
>>> filtering granularity might be supported in future (at least, it is not
>>> explicitly ruled out).
>>>
>>> James' current approach seems to be to pick a single BMEC flag that's
>>> in the right sort of area for each MPAM bit (though not equivalent) and
>>> translate that bit across to drive a corresponding the MPAM bit.  But
>>> I'd say that this is arch-specific configuration masquerading as
>>> generic configuration IMHO and not really generic at all.
>>>
>>> See "untested: arm_mpam: resctrl: Allow monitors to be configured"
>>> https://git.kernel.org/pub/scm/linux/kernel/git/morse/linux.git/commit/?h=mpam/snapshot/v6.7-rc2&id=db0ac51f60675b6c4a54ccd24fa7198ec321c56d
>>>
>>> I guess this needs discussion with James, since there will have been
>>> additional thought process behind all this that is not captured; either
>>> way, I guess it could be resolved after this series, but it will need a
>>> decision before the MPAM support is merged (or at least, before MPAM
>>> exposes userspace support for event configuration upstream).
>>>
>>> (If this has already been discussed and James' current approach has
>>> already been agreed as the least worst option, then I guess I can live
>>> with it; I just find it icky, and it looks odd to have AMD specifics in
>>> a common header.)
>>
>> I am not aware of such a discussion.
>>
>> Sounds like a motivation to delay this portion of the changes in patch #8.
>>
>> Reinette
> 
> 
> Ack, I'll discuss this with James.
> 
> I guess the thing to do will be to keep the affected definitions in the
> x86 headers for now, and carry the exports in James' MPAM branch until
> we figure out whether it really makes sense to share them.

I'm afraid this ship has already sailed. Nothing about the 'mbm_total_bytes_config'
section of Documentation/arch/x86/resctrl.rst says that this interface is AMD specific. It
just happens not to be supported on Intel parts, meaning no-one can tell the difference today.

Because the documentation doesn't say "AMD only" or "consult the lid of the box for the
meaning of the bits" - I expect user-space can expect this to work in the same way on any
platform that supports these files. As such, it has become resctrl's interface to this
stuff. We certainly don't want two different ways of doing this.


As MPAM can configure the monitors like this, I'm stuck between a rock and a hard place. I
can't invent something new - because existing user-space expects BMEC, and making these
bits generic is being questioned because its secretly AMD specific.


Mapping the BMEC^Wresctrl bits to what MPAM supports is my best attempt at supporting
this. The MPAM driver is all about fitting MPAM into a shape that can be exposed via
resctrl, so this is hardly out of keeping. (I could make the same argument about the
MBA percentages...)

The same argument holds for X86's event numbers. Some agreement on IDs for events is
needed between the arch and fs code - and we may as well stick with the X86 numbers today
- its certainly more convenient for the X86 arch code. Where arm64/MPAM or any other
architecture extends this, I'd hope to do it via perf where numbered events are already
understood to be platform specific, and there is tooling to support this.

For configurable events on non-AMD architectures? We can kick that into the perf weeds -
but we need somewhere to hang the fs/resctrl code that drives these files. I think
'is_AMD()' is detestable, and as these bits are uapi, they should be exposed for all
architectures that support resctrl.


Thanks,

James

Powered by blists - more mailing lists