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

[Abdul Rahim, Faizal]

On 12/11/2023 6:31 pm, Simon Horman wrote:
> 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.
> >
> > 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.
> >
> > Fixes: a3d43c0d56f1 ("taprio: Add support adding an admin schedule")
> > Signed-off-by: Faizal Rahim <faizal.abdul.rahim@xxxxxxxxxxxxxxx>
> > ---
> >   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
> >   
> >   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) {
>
> Hi Faizal,
>
> The first condition above expects that oper may be NULL, but the line below
> dereferences it unconditionally. This doesn't seem correct, one way or the
> other.

Thanks for pointing this out, sorry for this blunder.
Will update.

> As flagged by Smatch and Coccinelle.
>
> > +		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
>
> ...