Received-SPF: pass (google.com: domain of linux-kernel+bounces-125133-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Fri, 29 Mar 2024 08:31:36 -0700
From: Tony Luck <tony.luck@intel.com>
To: Reinette Chatre <reinette.chatre@intel.com>
Cc: linux-doc@vger.kernel.org, Fenghua Yu <fenghua.yu@intel.com>,
	Peter Newman <peternewman@google.com>,
	James Morse <james.morse@arm.com>, Babu Moger <babu.moger@amd.com>,
	Drew Fustini <dfustini@baylibre.com>, x86@kernel.org,
	linux-kernel@vger.kernel.org, patches@lists.linux.dev
Subject: Re: [PATCH] Documentation/x86: Document resctrl bandwidth control
 units are MiB
Message-ID: <Zgbe2FFwyHMmmsyM@agluck-desk3>
References: <20240322182016.196544-1-tony.luck@intel.com>
 <56a93ec2-dc01-49be-b917-5134f5794062@intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <56a93ec2-dc01-49be-b917-5134f5794062@intel.com>

On Thu, Mar 28, 2024 at 06:01:33PM -0700, Reinette Chatre wrote:
> Hi Tony,
> 
> On 3/22/2024 11:20 AM, Tony Luck wrote:
> > The memory bandwidth software controller uses 2^20 units rather than
> > 10^6. See mbm_bw_count() which computes bandwidth using the "SZ_1M"
> > Linux define for 0x00100000.
> > 
> > Update the documentation to use MiB when describing this feature.
> > It's too late to fix the mount option "mba_MBps" as that is now an
> > established user interface.
> 
> I see that this is merged already but I do not think this is correct.

I was surprised that Ingo merged it without giving folks a chance to
comment.

> Shouldn't the implementation be fixed instead? Looking at the implementation
> the intent appears to be clear that the goal is to have bandwidth be
> MBps .... that is when looking from documentation to the define
> (MBA_MAX_MBPS) to the comments of the function you reference above
> mbm_bw_count(). For example, "...and delta bandwidth in MBps ..."
> and "...maintain values in MBps..."

Difficult to be sure of intent. But in general when people talk about
"megabytes" in the context of memory they mean 2^20. Storage capacity
on computers was originally in 2^20 units until the marketing teams
at disk drive manufacturers realized they could print numbers 4.8% bigger
on their products by using SI unit 10^6 Mega prefix (rising to 7.3% with
Giga and 10% with Tera).

It is clear that the code uses 2^20 as it converts from bytes using
a right shift by 20.

Fixing the code would change the legacy API. Folks with a schemata
file that sets a limit of 5000 MB/s would find their applications
throttled by an addtional 4.8% on upgrading to a kernel with this
"fix".

> To me this change creates significant confusion since it now contradicts
> with the source code and comments I reference above. Not to mention the
> discrepancy with user documentation.
> 
> If you believe that this should be MiB then should the
> source and comments not also be changed to reflect that? Or alternatively,
> why not just fix mbm_bw_count() to support the documentation and what
> it appears to be intended to do. If users have been using the interface
> expecting MBps then this seems more like a needed bugfix than 
> a needed documentation change.

I agree that the comments need to be fixed. I will spin up a patch.

> Finally, if you make documentation changes, please do build the
> documentation afterwards. This change introduces a warning:
> 
> Memory bandwidth Allocation specified in MiBps
> ---------------------------------------------
> .../linux/Documentation/arch/x86/resctrl.rst:583: WARNING: Title underline too short.

My bad. Ingo has already applied a fix to TIP x86/urgent. I assume that
will be merged to Linus soon.

https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?h=x86/urgent&id=91491e5fb09624116950f9f2e1767a42e1da786

-Tony