Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
MIME-Version: 1.0
References: <20220516033524.3130816-1-chenwandun@huawei.com>
 <30b37eeb-e77b-882e-fc24-3367321a8ca3@gmail.com> <CAJuCfpE7fBsp8ntYVeLsW7Cd0Z09OmxN75X9Az_Qco0GJrz3Wg@mail.gmail.com>
 <CAJuCfpH-BDqsft1YvGFhkbR60VC0TJgfXKRVN+80e0iqQdhxpA@mail.gmail.com>
 <3a31521f-a68a-b2a9-baae-9a458ee17033@huawei.com> <070fe87d-43a0-5e4f-e4c7-c44782c2c195@gmail.com>
 <CAJuCfpH1mTxe5hmzZTe+AbPFse9heenx8uhGzCXE6fAh5G8SzA@mail.gmail.com> <29d66a46-d141-2d02-45dd-a8931786588e@gmail.com>
In-Reply-To: <29d66a46-d141-2d02-45dd-a8931786588e@gmail.com>
From:   Suren Baghdasaryan <surenb@google.com>
Date:   Thu, 19 May 2022 11:34:36 -0700
Message-ID: <CAJuCfpHpntz-viUQ+hAAWAkSSow7-TStr+yB3UEvqQ1WhAR22g@mail.gmail.com>
Subject: Re: [PATCH 1/2] psi: add support for multi level pressure stall trigger
To:     Alex Shi <seakeel@gmail.com>
Cc:     Chen Wandun <chenwandun@huawei.com>,
        LKML <linux-kernel@vger.kernel.org>,
        Johannes Weiner <hannes@cmpxchg.org>,
        Alex Shi <alexs@kernel.org>, Jonathan Corbet <corbet@lwn.net>,
        "open list:DOCUMENTATION" <linux-doc@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, May 18, 2022 at 11:15 PM Alex Shi <seakeel@gmail.com> wrote:
>
>
>
> On 5/19/22 05:38, Suren Baghdasaryan wrote:
> > On Wed, May 18, 2022 at 3:29 AM Alex Shi <seakeel@gmail.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. :)
> BTW, I still don't see the strong reason for the pressure range.
>
> > > 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?

Ok, sounds like userspace can handle the situation of multiple
triggers firing. Let's keep it simple as it is now, until we see a
strong need or convincing performance numbers for adding this new
trigger attribute.
Chen, if you can provide reasons why a userspace solution would be
prohibitive I would be happy to reconsider.
Thanks,
Suren.

>
> Thanks
> Alex