Re: [PATCH v2 3/3] sched/psi: allow unprivileged polling of N*2s period

[Johannes Weiner]

On Thu, Mar 23, 2023 at 09:43:16AM -0700, Suren Baghdasaryan wrote:
> On Thu, Mar 23, 2023 at 8:09 AM Johannes Weiner <hannes@xxxxxxxxxxx> 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.

IIRC Domenico had that in an earlier internal version. That was still
confusing because both sets of members are used for polling, not just
the rt-thread bits. I suggested to rename them all to make it clear
which poll bits are for the rt thread and which ones are from the
aggregator thread.

IMO code clarity trumps churn avoidance here.