Re: [PATCH v7 07/10] fs/resctrl: Add user interface to enable/disable io_alloc feature

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Babu,

On 7/31/25 3:52 PM, Moger, Babu wrote:
> Hi Reinette,
> 
> On 7/21/2025 6:40 PM, Reinette Chatre wrote:
>> Hi Babu,
>>
>> On 7/10/25 10:16 AM, Babu Moger wrote:
>>> "io_alloc" feature in resctrl enables direct insertion of data from I/O
>>> devices into the cache.
>>>
>>> Introduce user interface to enable/disable io_alloc feature.
>>
>> I think it is worth a mention *why* a user may want to disable this feature and
>> why is not just always enabled. Here it can be highlighted that this feature
>> may take resources (CLOSID) away from general (CPU) cache allocation and since
>> this may be scarce enabling user to disable this feature supports different use cases.
>>
> 
> Sure.
> 
>>>
>>> On AMD systems, io_alloc feature is backed by SDCIAE (L3 Smart Data Cache
>>> Injection Allocation Enforcement). When enabled, SDCIAE directs all SDCI
>>> lines to be placed into the L3 cache partitions specified by the register
>>> corresponding to the highest CLOSID supported by the resource. With CDP
>>> enabled, io_alloc routes I/O traffic using the highest CLOSID assigned to
>>> the instruction cache (L3CODE).
>>
>> This is a lot of architecture specific text for a resctrl fs patch  ... I think
>> you are trying to motivate the resctrl fs implementation. Similar motivation
>> as proposed for cover letter can be used here to help explain the implementation
>> choices.
> 
> Updated the whole changelog.
> 
> fs/resctrl: Add user interface to enable/disable io_alloc feature
> 
> "io_alloc" feature in resctrl enables direct insertion of data from I/O
> devices into the cache.
> 
> Introduce user interface to enable/disable io_alloc feature.

The solution should be at end of changelog after description of problem it
solves.

> On AMD systems, when io_alloc is enabled, the highest CLOSID is reserved
> exclusively for I/O allocation traffic and is no longer available for
> general CPU cache allocation. This feature is disabled by default. Users

Changelog should always be in imperative tone and problem and solution should
be in separate paragraphs (above paragraph mixes problem and solution).  

For example, "Disable "io_alloc" feature by default to ensure all resources are
available for general CPU cache allocation. ..." Although I do not think this is
accurate since this patch does not do this?

> are encouraged to enable it only when running workloads that can benefit
> from this functionality.
> 
> Since CLOSIDs are managed by resctrl fs, it is least invasive to make the
> "io_alloc is supported by maximum supported CLOSID" part of the initial
> resctrl fs support for io_alloc. Take care not to expose this use of
> CLOSID for io_alloc to user space so that this is not required from other
> architectures that may support io_alloc differently in the future.
> 

The changelog requirements I refer to are documented in "Changelog" section
of Documentation/process/maintainer-tip.rst. I feel like this should be
familiar to you by now.

Reinette