linux-kernel - Re: [PATCH V1 09/13] selftests/resctrl: Modularize fill

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: <2b7e2ef2-8e80-1e5e-41ac-4fc4726425ac@intel.com>
Date:   Wed, 11 Mar 2020 11:10:23 -0700
From:   Reinette Chatre <reinette.chatre@...el.com>
To:     Sai Praneeth Prakhya <sai.praneeth.prakhya@...el.com>,
        shuah@...nel.org, linux-kselftest@...r.kernel.org
Cc:     tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
        tony.luck@...el.com, babu.moger@....com, james.morse@....com,
        ravi.v.shankar@...el.com, fenghua.yu@...el.com, x86@...nel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH V1 09/13] selftests/resctrl: Modularize fill_buf for new
 CAT test case

Hi Sai,

On 3/11/2020 10:45 AM, Sai Praneeth Prakhya wrote:
> On Wed, 2020-03-11 at 08:44 -0700, Reinette Chatre wrote:
>> On 3/10/2020 6:04 PM, Sai Praneeth Prakhya wrote:
>>> On Tue, 2020-03-10 at 14:59 -0700, Reinette Chatre wrote:
>>>> On 3/6/2020 7:40 PM, Sai Praneeth Prakhya wrote:
>>>>> Currently fill_buf (in-built benchmark) runs as a separate process a
>>>>>
>>>>
> 
> [SNIP]
> 
>>>> Should buffer be freed on this error path?
>>>
>>> Yes, that's right.. my bad. Will fix it. But the right fix is,
>>> use_buffer_forever() should not return at all. It's meant to loop around
>>> the
>>> buffer _forever_.
>>>
>>>> I think the asymmetrical nature of the memory allocation and release
>>>> creates traps like this.
>>>>
>>>> It may be less error prone to have the pointer returned by init_buffer
>>>> and the acted on and released within fill_cache(), passed to
>>>> "use_buffer_forever()" as a parameter.  The buffer size is known here,
>>>> there is no need to keep an "end pointer" around.
>>>
>>> The main reason for having "startptr" as a global variable is to free
>>> memory
>>> when fill_buf is killed. fill_buf runs as a separate process (for test
>>> cases
>>> like MBM, MBA and CQM) and when user issues Ctrl_c or when the test kills
>>> benchmark_pid (i.e. fill_buf), the buffer is freed (please see
>>> ctrl_handler()).
>>
>> I see. Got it, thanks.
>>
>>> So, I thought, as "startptr" is anyways global, why pass it around as an
>>> argument? While making this change I thought it's natural to make "endptr"
>>> global as well because the function didn't really look good to just take
>>> endptr as an argument without startptr.
>>
>> Maintaining the end pointer is unusual. The start of the buffer and the
>> size are known properties that the end of the buffer can be computed
>> from. Not a problem, it just seems inconsistent that some of the buffer
>> functions operate on the start pointer and size while others operate on
>> the start pointer and end pointer.
> 
> Ok.. makes sense. I will try to make it consistent by using endptr all the
> time. One advantage of using endptr is that we could just compute endptr once
> and use it when needed by passing it as variable (will try to not make it
> global variable).

This may add unnecessary complexity because from what I can tell some of
those calls require buffer size and this would then require needing to
recompute the buffer size based on the start and end pointers. Do you
really need the end pointer? Can you not just use the start pointer and
buffer size?

Reinette