netdev - Re: [PATCH v2 net 1/7] net/sched: taprio: fix too early schedules switching

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20231108222756.l3u6h6gllxnbypyn@skbuf>
Date: Thu, 9 Nov 2023 00:27:56 +0200
From: Vladimir Oltean <vladimir.oltean@....com>
To: Faizal Rahim <faizal.abdul.rahim@...ux.intel.com>
Cc: Vinicius Costa Gomes <vinicius.gomes@...el.com>,
	Jamal Hadi Salim <jhs@...atatu.com>,
	Cong Wang <xiyou.wangcong@...il.com>, Jiri Pirko <jiri@...nulli.us>,
	"David S . Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 net 1/7] net/sched: taprio: fix too early schedules
 switching

On Tue, Nov 07, 2023 at 06:20:17AM -0500, Faizal Rahim wrote:
> In the current taprio code for dynamic schedule change,
> admin/oper schedule switching happens immediately when
> should_change_schedules() is true. Then the last entry of
> the old admin schedule stops being valid anymore from
> taprio_dequeue_from_txq’s perspective.

Admittedly, I may have become a bit detached from this code base in the
past months, but I don't understand the reasoning here.

Could you please explain what makes the last entry of the old admin
schedule be invalid from taprio_dequeue_from_txq()'s perspective?

What I see is that when should_change_schedules() is true, we change
q->oper_sched and q->admin_sched through the switch_schedules() call,
but we don't change q->current_entry, so I fail to understand the
connection you are implying.

On the other hand (and I see I did mention this in the other thread),
it seems that taprio_skb_exceeds_queue_max_sdu() - called from the
enqueue() path - looks at q->oper_sched, and that's a valid reason why
we'd want to delay the schedule switch until admin's actual base time,
rather than the current oper's cycle_end_time.

But please, let's spare no expense in providing a proper problem
description, justification for the change and Fixes: tag. This is not
optional.

> To solve this, we have to delay the switch_schedules() call via
> the new cycle_time_correction variable. The variable serves 2
> purposes:
> 1. Upon entering advance_sched(), if the value is set to a
> non-initialized value, it indicates that we need to change
> schedule.
> 2. Store the cycle time correction value which will be used for
> negative or positive correction.

It needs to be stated much more clearly that only purpose 1 is relevant
here (I would even go as far as to omit its secondary purpose here).
The only reason we are using the correction variable is because it
happens that we'll need that in later changes.

> 
> Fixes: a3d43c0d56f1 ("taprio: Add support adding an admin schedule")

I believe that since the only observable problem has to do with
taprio_skb_exceeds_queue_max_sdu(), the Fixes: tag should be the commit
which added that logic. Which is:

Fixes: a878fd46fe43 ("net/sched: keep the max_frm_len information inside struct sched_gate_list")

> Signed-off-by: Faizal Rahim <faizal.abdul.rahim@...ux.intel.com>
> ---
>  net/sched/sch_taprio.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
> index 2e1949de4171..dee103647823 100644
> --- a/net/sched/sch_taprio.c
> +++ b/net/sched/sch_taprio.c
> @@ -41,6 +41,7 @@ static struct static_key_false taprio_have_working_mqprio;
>  #define TXTIME_ASSIST_IS_ENABLED(flags) ((flags) & TCA_TAPRIO_ATTR_FLAG_TXTIME_ASSIST)
>  #define FULL_OFFLOAD_IS_ENABLED(flags) ((flags) & TCA_TAPRIO_ATTR_FLAG_FULL_OFFLOAD)
>  #define TAPRIO_FLAGS_INVALID U32_MAX
> +#define INIT_CYCLE_TIME_CORRECTION S64_MIN

I would prefer naming it CYCLE_TIME_CORRECTION_INVALID or _UNSPEC.
It is not just used as the "initial" value.

>  
>  struct sched_entry {
>  	/* Durations between this GCL entry and the GCL entry where the
> @@ -75,6 +76,7 @@ struct sched_gate_list {
>  	ktime_t cycle_end_time;
>  	s64 cycle_time;
>  	s64 cycle_time_extension;
> +	s64 cycle_time_correction;
>  	s64 base_time;
>  };
>  
> @@ -940,8 +942,10 @@ static enum hrtimer_restart advance_sched(struct hrtimer *timer)
>  	admin = rcu_dereference_protected(q->admin_sched,
>  					  lockdep_is_held(&q->current_entry_lock));
>  
> -	if (!oper)
> +	if (!oper || oper->cycle_time_correction != INIT_CYCLE_TIME_CORRECTION) {

You could introduce even as early as this change a "static bool
sched_switch_pending(struct sched_gate_list *oper)" function, which
incorporates the entire body of this "if" expression.

> +		oper->cycle_time_correction = INIT_CYCLE_TIME_CORRECTION;
>  		switch_schedules(q, &admin, &oper);
> +	}
>  
>  	/* This can happen in two cases: 1. this is the very first run
>  	 * of this function (i.e. we weren't running any schedule
> @@ -981,7 +985,7 @@ static enum hrtimer_restart advance_sched(struct hrtimer *timer)
>  		 * schedule runs.
>  		 */
>  		end_time = sched_base_time(admin);
> -		switch_schedules(q, &admin, &oper);
> +		oper->cycle_time_correction = 0;
>  	}
>  
>  	next->end_time = end_time;
> @@ -1174,6 +1178,7 @@ static int parse_taprio_schedule(struct taprio_sched *q, struct nlattr **tb,
>  	}
>  
>  	taprio_calculate_gate_durations(q, new);
> +	new->cycle_time_correction = INIT_CYCLE_TIME_CORRECTION;
>  
>  	return 0;
>  }
> -- 
> 2.25.1
>