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;
Date:   Thu, 23 Mar 2023 11:09:39 -0400
From:   Johannes Weiner <hannes@cmpxchg.org>
To:     Domenico Cerasuolo <cerasuolodomenico@gmail.com>
Cc:     linux-kernel@vger.kernel.org, peterz@infradead.org,
        surenb@google.com, brauner@kernel.org, chris@chrisdown.name
Subject: Re: [PATCH v2 3/3] sched/psi: allow unprivileged polling of N*2s
 period
Message-ID: <20230323150939.GA737760@cmpxchg.org>
References: <20230323103350.40569-1-cerasuolodomenico@gmail.com>
 <20230323103350.40569-4-cerasuolodomenico@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230323103350.40569-4-cerasuolodomenico@gmail.com>
Precedence: bulk

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.