netdev - Re: [PATCH net v1] net/sched: taprio: fix cycle time extension logic

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] [day] [month] [year] [list]

Message-ID: <20230602161455.zle4k6th3qoee4ho@skbuf>
Date: Fri, 2 Jun 2023 19:14:55 +0300
From: Vladimir Oltean <vladimir.oltean@....com>
To: Tan Tee Min <tee.min.tan@...ux.intel.com>
Cc: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@...el.com>,
	netdev@...r.kernel.org, vinicius.gomes@...el.com, kuba@...nel.org,
	pabeni@...hat.com, linux-kernel@...r.kernel.org,
	edumazet@...gle.com
Subject: Re: [PATCH net v1] net/sched: taprio: fix cycle time extension logic

On Fri, Jun 02, 2023 at 03:14:06PM +0800, Tan Tee Min wrote:
> Do you mean a separate patch to fix the recalculation issue for
> gate_close_time[tc] and budget[tc] in advance_sched()?

Yes. You might be asking: "separate from what"?

I notice one other unrelated change in your patch is to delay the
advance_sched() -> switch_schedules() call via this new sched_change_pending
variable, and that is probably a good change. But it needs its own patch
with its own context, problem description and solution description.

> > I see an issue here: you need to set q->sched_changed = true whenever
> > should_change_schedules() returned true and not just for the last entry,
> > correct? Because there could be a schedule change which needs to happens
> > during entry 2 out of 4 of the current one (truncation case - negative
> > correction). In that case, I believe that should_change_schedules()
> > keeps shouting at you "change! change! change!" but you only call
> > switch_schedules() when you've reached entry 4 with the "next" pointer,
> > which is not what the standard suggests should be done.
> > 
> > IIUC, the standard says that when an admin and an oper sched meet, the
> > decision of what to do near the AdminBaseTime - whether to elongate the
> > next-to-last cycle of the oper sched or to let the last cycle run but to
> > cut it short - depends on the OperCycleTimeExtension. In a nutshell,
> > that variable says what is the maximum positive correction applicable to
> > the last sched entry and to the cycle time. If a positive correction
> > larger than that would be necessary (relative to the next-to-last cycle),
> > the decision is to just let the last cycle run for how long it can.
> 
> Based on my understanding, here are the two cases when the Oper and Admin
> cycle boundaries donâ€™t line up perfectly:-
> 1/ The final Oper cycle before first Admin cycle is smaller than
>    OperCycleTimeExtension:
>    - Then extend the final oper cycle rather than restart a very short final
>      oper cycle.

Yes, this should result in a positive correction - a new cycle is not
begun, but the last entry of the next-to-last cycle just lasts longer.

> 2/ The final Oper cycle before first Admin cycle is greater than
>    OperCycleTimeExtension:
>    - Then it won't extend the final Oper cycle and restart the final Oper
>      cycle instead, then it will be truncated at Admin base time.

Yes, this should result in a negative correction - a new cycle is begun,
but whose effective length will be shorter because it will be truncated
as you say.

> I think you are saying the scenario 2/ above, right?
> Let me rework the solution and come back with the proper fixes.

Yes, I'm saying that in situation 2, you're not allowing the schedule to
be truncated where it should be, I believe.