On Fri, 14 Jul 2023, Reinette Chatre wrote: > On 7/14/2023 3:22 AM, Ilpo Järvinen wrote: > > On Fri, 14 Jul 2023, Wieczor-Retman, Maciej wrote: > >> On 14.07.2023 01:00, Reinette Chatre wrote: > >>> Hi Ilpo, > >>> > >>> On 7/13/2023 6:19 AM, Ilpo Järvinen wrote: > >>>> MBA and MBM tests to use megabytes to represent span. CMT test uses > >>>> bytes. The difference requires run_benchmark() to size the buffer > >>>> differently based on the test name, which in turn requires passing the > >>>> test name into run_benchmark(). > >>>> > >>>> Convert MBA and MBM tests to use internally bytes like CMT test to > >>>> remove the internal inconsistency between the tests. Remove the test > >>>> dependent buffer sizing from run_benchmark(). > >>> > >>> If I understand correctly the intention is to always use bytes internally > >>> and only convert to megabytes when displayed to user space. The above > >>> implies that this takes care of the conversion but there still seems > >>> to be places that that do not follow my understanding. For example, > >>> resctrl_val.c:measure_vals() converts to megabytes before proceeding. > >> > >> Doesn't the use case inside resctrl_val.c:measure_vals() satisfy > >> the idea of only displaying data to the user space? From my > >> understanding it reads the number of bytes and only converts to > >> MB when printing the value. Or did I miss some detail there? > > > > It's for printing there yes. > > > > But it's not about span in the first place so I'm not sure why it is > > related. > > > > If this change is just about how "span" is interpreted by the different > tests then the changelog could be more specific to not create expectation > that with this change there are no longer "bytes vs megabytes" internal > inconsistency between MBA, MBM, and CMT tests. The shortlog and changelog are already pretty specific in mentioning "span" a few times :-). I added yet another "span" into the changelog's 2nd paragraph. Your general observation about the other MB/bytes inconsistency is still a good one so I added it also to my todo list, it just doesn't belong to this patch (IMHO). -- i.
