linux-kernel - Re: [PATCH 1/2] psi: add support for multi level pressure stall trigger

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]

Date:   Sat, 21 May 2022 15:23:01 +0800
From:   Chen Wandun <chenwandun@...wei.com>
To:     Alex Shi <seakeel@...il.com>,
        Suren Baghdasaryan <surenb@...gle.com>
CC:     LKML <linux-kernel@...r.kernel.org>,
        Johannes Weiner <hannes@...xchg.org>,
        Alex Shi <alexs@...nel.org>, Jonathan Corbet <corbet@....net>,
        "open list:DOCUMENTATION" <linux-doc@...r.kernel.org>
Subject: Re: [PATCH 1/2] psi: add support for multi level pressure stall
 trigger



在 2022/5/19 14:15, Alex Shi 写道:
>
> On 5/19/22 05:38, Suren Baghdasaryan wrote:
>> On Wed, May 18, 2022 at 3:29 AM Alex Shi <seakeel@...il.com> wrote:
>>>
>>>
>>> On 5/17/22 20:46, Chen Wandun wrote:
>>>>>>> This breaks the old ABI. And why you need this new function?
>>>>>> Both great points.
>>>>> BTW, I think the additional max_threshold parameter could be
>>>>> implemented in a backward compatible way so that the old API is not
>>>>> broken:
>>>>>
>>>>> arg_count = sscanf(buf, "some %u %u %u", &min_threshold_us,  &arg2, &arg3);
>>>>> if (arg_count < 2) return ERR_PTR(-EINVAL);
>>>>> if (arg_count < 3) {
>>>>>       max_threshold_us = INT_MAX;
>>>>>       window_us = arg2;
>>>>> } else {
>>>>>       max_threshold_us = arg2;
>>>>>       window_us = arg3;
>>>>> }
>>>> OK
>>>>
>>>> Thanks.
>>>>> But again, the motivation still needs to be explained.
>>>> we want do different operation for different stall level,
>>>> just as prev email explain, multi trigger is also OK in old
>>>> ways, but it is a litter complex.
>>> In fact, I am not keen for this solution, the older and newer
>>> interface is easy to be confused by users, for some resolvable
>>> unclear issues. It's not a good idea.
>> Maybe adding the max_threshold as an optional last argument will be
>> less confusing? Smth like this:
>>
>> some/full min_threshold window_size [max_threshold]
> It's already confused enough. :)
which point make you confused？
Interface suggest by Suren is compatible with current version,
I think it is more reasonable and there is no difficuty to understand it.
> BTW, I still don't see the strong reason for the pressure range.
Considering this case:
I divide pressure into multi levels, and each level corresponds to a
hander,  I have to register multi triggers and wait for fire events,
nowadays, these trigger is something like:
echo “some 150000 1000000” > /proc/pressure/memory
echo “some 350000 1000000” > /proc/pressure/memory
echo “some 550000 1000000” > /proc/pressure/memory
echo “some 750000 1000000” > /proc/pressure/memory

In the best case, stall pressure between 150000 and 350000,
only one trigger fire, and only one wakeup.

In any other case,  multi triggers fire and multi wakeup, but it
indeed is no need.

New implement make the fire and wakeup more precise,
userspace code will be more simple, no confusing fire event,
no need to filter fire event anymore, maybe minor performance
improved.

Thanks.
>
>>> Also, if we do decide to add it, there should be a warning in the
>> documentation that max_threshold usage might lead to a stall being
>> missed completely. In your example:
>>
>> echo "some 150000 350000 1000000" > /proc/pressure/memory
>>
>> If there is a stall of more than 350ms within a given window, that
>> trigger will not fire at all.
> Right.
> And what if others propose more pressure combinations?
> Maybe leave them to user space is more likely workable?
>
> Thanks
> Alex
> .