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
