linux-kernel - Re: [PATCH] Documentation/x86: Document resctrl bandwidth control units are MiB

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zgbe2FFwyHMmmsyM@agluck-desk3>
Date: Fri, 29 Mar 2024 08:31:36 -0700
From: Tony Luck <tony.luck@...el.com>
To: Reinette Chatre <reinette.chatre@...el.com>
Cc: linux-doc@...r.kernel.org, Fenghua Yu <fenghua.yu@...el.com>,
	Peter Newman <peternewman@...gle.com>,
	James Morse <james.morse@....com>, Babu Moger <babu.moger@....com>,
	Drew Fustini <dfustini@...libre.com>, x86@...nel.org,
	linux-kernel@...r.kernel.org, patches@...ts.linux.dev
Subject: Re: [PATCH] Documentation/x86: Document resctrl bandwidth control
 units are MiB

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