MIME-Version: 1.0
References: <cover.1676067791.git.quic_sudaraja@quicinc.com>
 <CAJuCfpHWQ8NV=iR3BN+pt1c8FynCnRqyyriHb1gLxFgiNVrwjA@mail.gmail.com>
 <e944536f-a04c-5528-601e-d7f505a761e8@quicinc.com> <CAJuCfpGLkkS2yx0d9+2nYtEtxANSH5H3EgCmWZax4N-ieEBG7g@mail.gmail.com>
 <15cd8816-b474-0535-d854-41982d3bbe5c@quicinc.com> <CAJuCfpHihLgHCcsAqMJ_o2u7Ux9B5HFGsV2y_L2_5GXYAGYLnw@mail.gmail.com>
 <82406da2-799e-f0b4-bce0-7d47486030d4@quicinc.com> <CAJuCfpHrhO7_fMwNuu2hdQob=MPjZTW8eaJpNhEhPmDMqz2qTA@mail.gmail.com>
 <Y/ix53x8i/ViuBXf@dhcp22.suse.cz>
In-Reply-To: <Y/ix53x8i/ViuBXf@dhcp22.suse.cz>
From:   Suren Baghdasaryan <surenb@google.com>
Date:   Fri, 24 Feb 2023 13:07:57 -0800
Message-ID: <CAJuCfpG1c5v6qOotPV6t-o1T1p7EbOLs90eFnoEBjts+gafovA@mail.gmail.com>
Subject: Re: [PATCH] psi: reduce min window size to 50ms
To:     Michal Hocko <mhocko@suse.com>
Cc:     Sudarshan Rajagopalan <quic_sudaraja@quicinc.com>,
        David Hildenbrand <david@redhat.com>,
        Johannes Weiner <hannes@cmpxchg.org>,
        Mike Rapoport <rppt@kernel.org>,
        Oscar Salvador <osalvador@suse.de>,
        Anshuman Khandual <anshuman.khandual@arm.com>,
        mark.rutland@arm.com, will@kernel.org,
        virtualization@lists.linux-foundation.org, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-arm-msm@vger.kernel.org,
        Trilok Soni <quic_tsoni@quicinc.com>,
        Sukadev Bhattiprolu <quic_sukadev@quicinc.com>,
        Srivatsa Vaddagiri <quic_svaddagi@quicinc.com>,
        Patrick Daly <quic_pdaly@quicinc.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Feb 24, 2023 at 4:47 AM Michal Hocko <mhocko@suse.com> wrote:
>
> On Tue 14-02-23 11:34:30, Suren Baghdasaryan wrote:
> [...]
> > Your suggestion to have this limit configurable sounds like obvious
> > solution. I would like to get some opinions from other maintainers.
> > Johannes, WDYT? CC'ing Michal to chime in as well since this is mostly
> > related to memory stalls.
>
> I do not think that making this configurable helps much. Many users will
> be bound to distribution config and also it would be hard to experiment
> with a recompile cycle every time. This seems just too impractical.
>
> Is there any reason why we shouldn't allow any timeout? Shorter
> timeouts could be restricted to a priviledged context to avoid an easy
> way to swamp system by too frequent polling.

Hmm, ok. Maybe then we just ensure that only privileged users can set
triggers and remove the min limit (use a >0 check)?

>
> Btw. it seems that there is is only a limit on a single trigger per fd
> but no limits per user so it doesn't sound too hard to end up with too
> much polling even with a larger timeouts. To me it seems like we need to
> contain the polling thread to be bound by the cpu controller.

Hmm. We have one "psimon" thread per cgroup (+1 system-level one) and
poll_min_period for each thread is chosen as the min() of polling
periods between triggers created in that group. So, a bad trigger that
causes overly aggressive polling and polling thread being throttled,
might affect other triggers in that cgroup.
I would prefer to deny creation of a new trigger if it would cause too
much polling. However, that seems to be getting into the territory of
"implementing policy inside the kernel". Maybe we just limit trigger
creation to privileged processes only and let those privileged system
components worry about trigger creation policies? These system
processes can also limit cpu shares of all "psimon" threads using cpu
controller, if desired. WDYT?

> --
> Michal Hocko
> SUSE Labs