cavs: budgeting for EDF tasks by jajanusz · Pull Request #3268 · thesofproject/sof

jajanusz · 2020-08-04T08:51:30Z

PLEASE test it on BYT this time.

2nd attempt to #3057
Reverted by #3245

jajanusz · 2020-08-04T11:32:15Z

@plbossart
You reverted wrong commits, scheduler changes are good, timer implementation is wrong ;)
Issue is caused by the fact that you use wallclock for everything and you break 'clocks' interface with it - cpu_timer_get should return cpu timer not exernal timer and I see that you have only one timer in use at all:

SHARED_DATA struct timer timer = {
	.id = TIMER3, /* external timer */
	.irq = IRQ_NUM_EXT_TIMER,
};

IDK what's there on BYT and if you want to actually add arch timers (if there are any) - who's responsible for BYT support?

jajanusz · 2020-08-04T11:40:32Z

IMO cpu_timer implementation in sof struct is broken - all non-cavs platforms return external timer instead of one in cpu.
Can I assume that all these platforms have internal timers and add something like that for BYT:

SHARED_DATA struct timer arch_timer = {
	.id = TIMER1, /* internal timer */
	.irq = IRQ_NUM_TIMER2,
};

@paulstelian97 @dbaluta
What about IMX*, can I use internal timers like the one above, and what ID/IRQ should they have?

paulstelian97 · 2020-08-04T12:40:17Z

@paulstelian97 @dbaluta
What about IMX*, can I use internal timers like the one above, and what ID/IRQ should they have?

As far as I could remember, there is only one timer which is used for the timestamps in traces and for DMA trace flushing. It's already defined for this purpose. Need to check though.

plbossart · 2020-08-04T14:58:17Z

@jajanusz see picture below there is again no trace on BYT/CHT. the successful test is a false positive, it's broken again.

jajanusz · 2020-08-04T15:39:33Z

@jajanusz see picture below there is again no trace on BYT/CHT. the successful test is a false positive, it's broken again.

Yea there are not traces, I know it, CI shows good, I just wrote that problem is not with scheduler, but with timers. All nonCAVS platform return external timer for call that should give internal timer (cpu timer), but I know nothing about BYT and I don't know if I should expect it to have it. Should we expect other platform to have it? It's in the core functionality, so either should be moved out of it (guarded with Kconfig etc) or supported correctly by all platforms.

plbossart · 2020-08-04T18:23:43Z

Yea there are not traces, I know it, CI shows good, I just wrote that problem is not with scheduler, but with timers. All nonCAVS platform return external timer for call that should give internal timer (cpu timer), but I know nothing about BYT and I don't know if I should expect it to have it. Should we expect other platform to have it? It's in the core functionality, so either should be moved out of it (guarded with Kconfig etc) or supported correctly by all platforms.

@lgirdwood @keyonjie need you to chime in here, I don't think we can rely on internal timers on Baytrail, where the core can run at 6 or 7 different speeds, adjusted depending on load. It's different to cAVS platforms where the goal was run to completion and power-gate.

plbossart · 2020-08-04T18:31:08Z

Yea there are not traces, I know it, CI shows good, I just wrote that problem is not with scheduler, but with timers. All nonCAVS platform return external timer for call that should give internal timer (cpu timer), but I know nothing about BYT and I don't know if I should expect it to have it. Should we expect other platform to have it? It's in the core functionality, so either should be moved out of it (guarded with Kconfig etc) or supported correctly by all platforms.

@jajanusz What I don't get is that the rest of the functionality seems unafffected, it's only the traces that are disabled? if indeed we used the wrong timer then shouldn't everything just go South? How does it impact the trace only?

jajanusz · 2020-08-05T08:56:21Z

Yea there are not traces, I know it, CI shows good, I just wrote that problem is not with scheduler, but with timers. All nonCAVS platform return external timer for call that should give internal timer (cpu timer), but I know nothing about BYT and I don't know if I should expect it to have it. Should we expect other platform to have it? It's in the core functionality, so either should be moved out of it (guarded with Kconfig etc) or supported correctly by all platforms.

@jajanusz What I don't get is that the rest of the functionality seems unafffected, it's only the traces that are disabled? if indeed we used the wrong timer then shouldn't everything just go South? How does it impact the trace only?

Because you return same timer in all calls (when there should be different timers) when I set timer handler for budgeting I overwrite probably some other handler that is already on wallclock. If we cannot rely on internal timers, then our god struct struct sof shouldn't have internal timer (or it should be guarded with #if CONFIG_HAVE_INTERNAL_TIMER).

jajanusz · 2020-08-05T10:14:11Z

Blocked by #3274

jajanusz · 2020-08-24T15:27:09Z

Cut budgeting with #ifdefs to not block this feature by waiting for blocking issue.

lgirdwood

Btw, do we have any accompanying documentation on how this will work in sof-docs given these new updates ?

lgirdwood · 2020-08-27T12:24:44Z

Are we basing the budget on 8000 timer_ticks ? If so, I think this would be better as a per platform CONFIG_ variable (given that non CAVS platforms will have a different timer).

It's just an example. This is defined as CONFIG and defaults to 8k.

ok, it's probably good to explain this in the Kconfig "help" for the option. I guess 8k is based on a % of total budget (based on max clock rate?). Just looking for detail so that the option can be easily tuned based on teh help.

Well it's already in help in kconfig - that it's within 1 systick IDK if we need to add more there.
It's not really % because when you switch from HPRO to LPRO you have 120k cycles in systick instead of 400k and budget for IPCs stays the same. It's just value enough to handle IPCs without stalling other tasks.

Add possibility to set budget for EDF tasks. It's needed to ensure that some long running task will not stall other tasks. Signed-off-by: Janusz Jankowski <janusz.jankowski@linux.intel.com>

IPC task may be time consuming and has high priority, what may make other tasks stall. To prevent this it should have limited cycles budget per systick. Signed-off-by: Janusz Jankowski <janusz.jankowski@linux.intel.com>

IDC task may be time consuming and has high priority, what may make other tasks stall. To prevent this it should have limited cycles budget per systick. Signed-off-by: Janusz Jankowski <janusz.jankowski@linux.intel.com>

jajanusz · 2020-09-10T08:12:04Z

Cut budgeting with #ifdefs to not block this feature by waiting for blocking issue.

Restored old implementation, cos @lyakh fixed clocks for BYT and it makes code simpler.

lgirdwood · 2020-09-10T11:41:02Z

@jajanusz as we are near to v1.6 cut off here, are you happy for this in v1.6 or would you prefer to wait for v1,7 ?

plbossart

Comments below, mainly to clarify some points that aren't clear to me - no change needed in the code itself.

plbossart · 2020-09-10T14:43:29Z


 struct edf_task_pdata {
 	void *ctx;
+	uint32_t budget;	/**< cycles per systick */


I am pretty sure in the past we defined variable ticks for EDF. Would this scale here if the tick duration is changed dynamically. And I read in a comment below that we use '120k cycles in systick instead of 400k', so shouldn't there be multiple budget definitions, one per systick value?

plbossart · 2020-09-10T14:45:38Z

+{
+	struct edf_task_pdata *edf_pdata = edf_sch_get_pdata(task);
+
+	if (!edf_pdata->budget)


maybe clarify with a comment in the structure definition that a budget of zero will result in a specific behavior. It's also not clear to me if this is a feature or an error condition.

plbossart · 2020-09-10T14:49:05Z


 struct edf_task_pdata {
 	void *ctx;
+	uint32_t budget;	/**< cycles per systick */


also explain that bugdet can be zero, and the difference in behavior between zero and non-zero values.

jajanusz · 2020-09-10T16:38:58Z

@jajanusz as we are near to v1.6 cut off here, are you happy for this in v1.6 or would you prefer to wait for v1,7 ?

We can leave it for 1.7, better to let it mature in repo, maybe some other issues will come out like with BYT, it touched a few things that were not used a lot so far (so are not stable).

jajanusz · 2020-11-26T15:11:19Z

Feature is not needed ATM and may impose too many sideffects without extensive long-term testing, if someone will need it in the feature than it can be continued from this PR.

jajanusz requested review from dbaluta, lbetlej, lgirdwood, mmaka1, mrajwa, plbossart and tlauda as code owners August 4, 2020 08:51

jajanusz changed the title ~~cavs: budgeting for EDF tasks~~ [WIP] cavs: budgeting for EDF tasks Aug 4, 2020

jajanusz force-pushed the edf-budget-2 branch from 5aa26d0 to 6eb2030 Compare August 4, 2020 10:25

jajanusz mentioned this pull request Aug 5, 2020

[BUG] Platform & CPU timer should be distinct #3274

Closed

jajanusz force-pushed the edf-budget-2 branch 2 times, most recently from bee1604 to dd9c573 Compare August 24, 2020 14:37

jajanusz force-pushed the edf-budget-2 branch 4 times, most recently from c130c20 to eb5c389 Compare August 24, 2020 20:00

lgirdwood reviewed Aug 27, 2020

View reviewed changes

jajanusz force-pushed the edf-budget-2 branch from eb5c389 to dd9c573 Compare September 8, 2020 17:33

jajanusz added 2 commits September 8, 2020 20:15

schedule: edf: cycles budget

229f1cc

Add possibility to set budget for EDF tasks. It's needed to ensure that some long running task will not stall other tasks. Signed-off-by: Janusz Jankowski <janusz.jankowski@linux.intel.com>

drivers: cavs: set budget for IPC task

20a6d02

IPC task may be time consuming and has high priority, what may make other tasks stall. To prevent this it should have limited cycles budget per systick. Signed-off-by: Janusz Jankowski <janusz.jankowski@linux.intel.com>

idc: set budget for IDC task

980163e

IDC task may be time consuming and has high priority, what may make other tasks stall. To prevent this it should have limited cycles budget per systick. Signed-off-by: Janusz Jankowski <janusz.jankowski@linux.intel.com>

jajanusz force-pushed the edf-budget-2 branch from dd9c573 to 980163e Compare September 8, 2020 18:20

lgirdwood approved these changes Sep 9, 2020

View reviewed changes

jajanusz changed the title ~~[WIP] cavs: budgeting for EDF tasks~~ cavs: budgeting for EDF tasks Sep 10, 2020

plbossart reviewed Sep 10, 2020

View reviewed changes

jajanusz closed this Nov 26, 2020

Conversation

jajanusz commented Aug 4, 2020

Uh oh!

jajanusz commented Aug 4, 2020

Uh oh!

jajanusz commented Aug 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paulstelian97 commented Aug 4, 2020

Uh oh!

plbossart commented Aug 4, 2020

Uh oh!

jajanusz commented Aug 4, 2020

Uh oh!

plbossart commented Aug 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

plbossart commented Aug 4, 2020

Uh oh!

jajanusz commented Aug 5, 2020

Uh oh!

jajanusz commented Aug 5, 2020

Uh oh!

jajanusz commented Aug 24, 2020

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lgirdwood Aug 27, 2020

Choose a reason for hiding this comment

Uh oh!

jajanusz Sep 3, 2020

Choose a reason for hiding this comment

Uh oh!

lgirdwood Sep 7, 2020

Choose a reason for hiding this comment

Uh oh!

jajanusz Sep 10, 2020

Choose a reason for hiding this comment

Uh oh!

jajanusz commented Sep 10, 2020

Uh oh!

lgirdwood commented Sep 10, 2020

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

plbossart Sep 10, 2020

Choose a reason for hiding this comment

Uh oh!

plbossart Sep 10, 2020

Choose a reason for hiding this comment

Uh oh!

plbossart Sep 10, 2020

Choose a reason for hiding this comment

Uh oh!

jajanusz commented Sep 10, 2020

Uh oh!

jajanusz commented Nov 26, 2020

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jajanusz commented Aug 4, 2020 •

edited

Loading

plbossart commented Aug 4, 2020 •

edited

Loading