Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230323103350.40569-1-cerasuolodomenico@gmail.com>
 <20230323103350.40569-4-cerasuolodomenico@gmail.com> <20230323150939.GA737760@cmpxchg.org>
In-Reply-To: <20230323150939.GA737760@cmpxchg.org>
From:   Suren Baghdasaryan <surenb@google.com>
Date:   Thu, 23 Mar 2023 09:43:16 -0700
Message-ID: <CAJuCfpG7SeTd7ki90-SHqRGUF9kGYEEUynjQpwjb7E2-3pM54g@mail.gmail.com>
Subject: Re: [PATCH v2 3/3] sched/psi: allow unprivileged polling of N*2s period
To:     Johannes Weiner <hannes@cmpxchg.org>
Cc:     Domenico Cerasuolo <cerasuolodomenico@gmail.com>,
        linux-kernel@vger.kernel.org, peterz@infradead.org,
        brauner@kernel.org, chris@chrisdown.name
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Mar 23, 2023 at 8:09=E2=80=AFAM Johannes Weiner <hannes@cmpxchg.org=
> wrote:
>
> On Thu, Mar 23, 2023 at 11:33:50AM +0100, Domenico Cerasuolo wrote:
> > @@ -151,6 +151,14 @@ struct psi_trigger {
> >
> >       /* Deferred event(s) from previous ratelimit window */
> >       bool pending_event;
> > +
> > +     /* Used to differentiate destruction action*/
> > +     enum psi_aggregators aggregator;
> > +};
> > +
> > +struct trigger_info {
> > +     struct list_head triggers;
> > +     u32 nr_triggers[NR_PSI_STATES - 1];
> >  };
> >
> >  struct psi_group {
> > @@ -186,8 +194,7 @@ struct psi_group {
> >       struct mutex trigger_lock;
> >
> >       /* Configured polling triggers */
> > -     struct list_head triggers;
> > -     u32 nr_triggers[NR_PSI_STATES - 1];
> > +     struct trigger_info trig_info[NR_PSI_AGGREGATORS];
> >       u32 poll_states;
> >       u64 poll_min_period;
>
> Thanks for trying out this variant, but I think this is grouping up
> unrelated things, and that makes the code more difficult to understand
> and maintan.
>
> The *only* thing that's shared between those two is the
> update_triggers() part. trig_info[PSI_AVGS] doesn't use trigger_lock.
> It also doesn't use poll_task, poll_wait, poll_wakeup, poll_scheduled,
> poll_min_period, polling_next_update and polling_until. All these
> things are specific to the rt polling thread.
>
> The rename in the previous version is a bit churny, but it's justified
> in order to keep unrelated things separate / make it obvious which
> parts belong together, and who is reading and writing which fields.
>
> So my vote would be on the previous version.

Hmm. Ok, but then I would suggest keeping RT trigger naming as is and
calling the new triggers based on averages as
avg_triggers/avg_nr_triggers/etc. This would limit the churn and since
we already have polling_total and avg_total, this naming would be
appropriate IMO. If we want to be even stricter, we could rename the
polling variables to poll_triggers/poll_nr_triggers/etc.  Some more
churn but then the names are very distinct.