Received-SPF: pass (google.com: domain of linux-kernel+bounces-123934-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <56a93ec2-dc01-49be-b917-5134f5794062@intel.com>
Date: Thu, 28 Mar 2024 18:01:33 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] Documentation/x86: Document resctrl bandwidth control
 units are MiB
To: Tony Luck <tony.luck@intel.com>, <linux-doc@vger.kernel.org>
CC: 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>
References: <20240322182016.196544-1-tony.luck@intel.com>
Content-Language: en-US
From: Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20240322182016.196544-1-tony.luck@intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?UXVkZUVjdm1BbWp5SHNqcXZWK0FYTUhiOURwSG9WSGpPWk1BRnNWOEx3Yzhw?=
 =?utf-8?B?a3RoUFVoc0tNcmZtb0FmR1hlY2JPYUZSSHZvNmhiTkE5eEJXRkpNNDZLZ3lo?=
 =?utf-8?B?akJ1NW5tYXh4d3loWHI1MU9TcFkrN3dXV24zeTc3eWRncUxqWTdydTU1WjBa?=
 =?utf-8?B?dUJGYUJ5RUN2UGx0N2JqclNyb1FIUXRPdFh5UEZ6M25DWDV5YWtxYW9TQjQz?=
 =?utf-8?B?aTMrbWVlQk90UmhrWnhGZytkbnZTbVh3bVVtU1RsMFh0ZWN3NitrcGkyUXRC?=
 =?utf-8?B?aWJyUkVuUGwyYTRmaGdNNzV2aVdCVXc1Y1ViT3VWQ2RRNzNyTDhJbGpRNzZI?=
 =?utf-8?B?NFdyZDJsTml6Tk1FZHI1WXV3eHBRbWwxMHIwZDVvVE9zdG1JWFpwMnlweGJq?=
 =?utf-8?B?ZEUvNHFqUWMxWWNPOFhQVWI3UWh5a29sajBnMTZzaUlEY0FBQk04Z0FYVWFU?=
 =?utf-8?B?Yi9IdU04Rzk4M2RXSmV2bFdNY0Z1eU1VMlB4MklXekNqQkh4ejJUdVc2NWc5?=
 =?utf-8?B?ZFJzczVqTGNXdE12S3loWUhvRUpGMUg2YmZxR3VoRlN0Q1ZCaEc1Yytyc2Nu?=
 =?utf-8?B?c1RjWXREUHRLd0lNdytneHFYSmNTYzJDMUJNV0U5bmRDMEpBTGdEVnVoZ2N3?=
 =?utf-8?B?cWN4SUNYRjI4bVo2b1NGU3JnMkIzME05eHE3MU5IOTZMb1hTQkNCRklnODNN?=
 =?utf-8?B?YUxSYk9SOVdFYkEvenp0YkZQMWx5MlBYbDNtMDV5MFNxYUZrN2NJT0ZrVjBS?=
 =?utf-8?B?YWFkdldJeTJ4aHVmMG9KejJjWEJDRnRYcHgwYjNyWnhpV2Y4SVVUZUdqNGZt?=
 =?utf-8?B?bkVKbGl3RXlQSk5jN0JwbWN4b1ZHM3RScEw1SHZ6S3RSbjNIUVJFN0U1OE1i?=
 =?utf-8?B?MHdOQkM3djB1MGkra2pEdThINmU3Mi9CWVAvUXo1ejhPRnZ6eUVNNk44akNv?=
 =?utf-8?B?SzRrajRaNDlwOWhzS25hbGQrUWNXN0E2dnMzT09GRm1OVllQT0M5ODJtRU5k?=
 =?utf-8?B?T1REbjZ3OHAwYnZZTXZmNFVwUzJxeU9WVVBFeFJoTnpBejVjRWlvNmEyMFVo?=
 =?utf-8?B?KzhvZzRWR3NVVm1wUmorWld2Sk9SdUpOenE0UlI4cTdDMCtqWGYvVEdxYTNX?=
 =?utf-8?B?N09mTVBMNUdxWGUxYW1xclYyblBDVG42V1JGaUZYWlY1Uy80WGRTakFwMm9G?=
 =?utf-8?B?NDNTNzhkS0xtMFVnSFIrb1pWZDBDUlhPOE9TMEdxREU2NExwbDV4bkVTZWNk?=
 =?utf-8?B?UlgrQXgwV0NEUDY1RFc2T3N2M3Y1MysxMDN5WTRMTGM2SXdzdnl5MFFWc082?=
 =?utf-8?B?elQrQ1RLc1h5MnRoTUw4RkM0a2NFK1RTaEdhOG5WU3VhRW5lNy9RcXdVQmo5?=
 =?utf-8?B?MFFRY3hteXZSSnlpTG5RWHFsbkprOWJrR0dRVFRac0NrQkcvc3MyVU55U0xk?=
 =?utf-8?B?V3hmQXJxbi9NZmdub3B4QzUzSTAvTDAyYjdLZkdFUkNFSzdJaXExcGljZDBV?=
 =?utf-8?B?dHpiNXpQNUZmQm40T1VBWlJBQzNGLzRWbS9OdVgxZkZwdW1MK1ZIbWpXdm1S?=
 =?utf-8?B?M0E1M2pRay9Ea2NwQ1I5WGFOMTlzTThvQ0FkUm9VRnpiVC93T1F2TVR6Z3JR?=
 =?utf-8?B?Q0FxYUdWMHNDWEpoVzIyWEVZMnFxQVBVOWF3czZNN2VOQjJxdDZ0NnpVMWtH?=
 =?utf-8?B?eGEzQXE3WlNadzFJNXdDdDBrR29aK3ZOZ1FzeDhwTThKUXFTQ2wrWFNmZVlC?=
 =?utf-8?B?VUk2OGlraU1lQWFIN005bFJTWG5Kc3ZTQTMybHI4NU5BUVJwK1hzQjFpZTB1?=
 =?utf-8?B?WVFENU93RnV5TXB5L0hCd0xJaTZyZ0VFc2JkbGNMWHVTZ2dtQnJ4MUhGenJF?=
 =?utf-8?B?V09BZGdxT1hoYWtMY1BLaWpOUG5NYjE2YnBzdUFzOUhRVndWNlEvblBBdWZ4?=
 =?utf-8?B?SG1vZmhheVZVTVBZMGdtSTVUT3dmdzVEbXpjMWdNTjRKUE95K2VpcVBmSm56?=
 =?utf-8?B?Wm9mdGI0WkZLSnZ6RVhXUEZQanRRdVJ3R0N1eU9OTHJNKzZFTS9yVkFSeWZh?=
 =?utf-8?B?MnU3SW5RY1V6OVFFU3VQZUN2enhSVUNkdnJFRVNQZHIrWlJkbEtxdHZNR0tp?=
 =?utf-8?B?Q3J3ZU52bFhaNi9QWWt1NERSWlRwcnpWNVVUNHpMQmZKS0NhK2xJdU51Q1NL?=
 =?utf-8?B?M3c9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: a78bfada-5f46-412d-e693-08dc4f8bc853
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Mar 2024 01:01:36.0772
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: WTBfKV5izHaPMaxbxLoFM72ldAegq63kEhK/tSNrgiG8XNNMztuT9nvQQ66JooZWo8YTCoLsjHCrmSCYf6v3GP/33gLUnKoDXuyrDhKA3CI=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW4PR11MB5892
X-OriginatorOrg: intel.com

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.
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..."

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.

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.

Reinette