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, 22 Jun 2023 09:53:52 -0500
From:   David Vernet <void@manifault.com>
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     linux-kernel@vger.kernel.org, mingo@redhat.com,
        juri.lelli@redhat.com, vincent.guittot@linaro.org,
        rostedt@goodmis.org, dietmar.eggemann@arm.com, bsegall@google.com,
        mgorman@suse.de, bristot@redhat.com, vschneid@redhat.com,
        joshdon@google.com, roman.gushchin@linux.dev, tj@kernel.org,
        kernel-team@meta.com
Subject: Re: [RFC PATCH 2/3] sched/fair: Add SWQUEUE sched feature and
 skeleton calls
Message-ID: <20230622145352.GB113759@maniforge>
References: <20230613052004.2836135-1-void@manifault.com>
 <20230613052004.2836135-3-void@manifault.com>
 <20230621124933.GE2053369@hirez.programming.kicks-ass.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230621124933.GE2053369@hirez.programming.kicks-ass.net>
User-Agent: Mutt/2.2.10 (2023-03-25)
Precedence: bulk

On Wed, Jun 21, 2023 at 02:49:33PM +0200, Peter Zijlstra wrote:
> On Tue, Jun 13, 2023 at 12:20:03AM -0500, David Vernet wrote:
> 
> I can't help but read this thing as software-queue :/ Can we please pick
> a better name?

Yes, I'll think of a better one. Suggestions welcome if you have a
preference.

> > @@ -6368,6 +6390,9 @@ enqueue_task_fair(struct rq *rq, struct task_struct *p, int flags)
> >  	if (!task_new)
> >  		update_overutilized_status(rq);
> >  
> > +	if (sched_feat(SWQUEUE))
> > +		swqueue_enqueue(rq, p, flags);
> > +
> >  enqueue_throttle:
> >  	assert_list_leaf_cfs_rq(rq);
> >  
> > @@ -6449,6 +6474,9 @@ static void dequeue_task_fair(struct rq *rq, struct task_struct *p, int flags)
> >  dequeue_throttle:
> >  	util_est_update(&rq->cfs, p, task_sleep);
> >  	hrtick_update(rq);
> > +
> > +	if (sched_feat(SWQUEUE))
> > +		swqueue_remove_task(p);
> >  }
> >  
> >  #ifdef CONFIG_SMP
> 
> _enqueue() should obviously be complemented by _dequeue(). This naming
> is offensive :-)

Ack

> > @@ -8155,12 +8183,18 @@ done: __maybe_unused;
> >  
> >  	update_misfit_status(p, rq);
> >  
> > +	if (sched_feat(SWQUEUE))
> > +		swqueue_remove_task(p);
> > +
> >  	return p;
> >  
> >  idle:
> >  	if (!rf)
> >  		return NULL;
> >  
> > +	if (sched_feat(SWQUEUE) && swqueue_pick_next_task(rq, rf))
> > +		return RETRY_TASK;
> > +
> >  	new_tasks = newidle_balance(rq, rf);
> >  
> >  	/*
> 
> That's either not correct or insufficient or both.
> 
> It fails to consider the whole core-scheduling mess. But it also fails
> to consider the regular (non optimized) pick case that should do newidle
> through put_prev_task_balance() -> balance_fair().
> 
> I think placing the pick call in newidle_balance() itself is the
> simplest solution.

Yep, not sure where I went off the rails here -- the pick call clearly
belongs in newidle_balance(). I'll also make sure we handle core sched
correctly as well. Thanks for pointing those out.