Skip to content

scheduler: guard against subdivision underflow on domain clear - #4088

Merged
lgirdwood merged 1 commit into
thesofproject:mainfrom
slawblauciak:ll_sched_numclients_underflow
Apr 26, 2021
Merged

lgirdwood merged 1 commit into
thesofproject:mainfrom
slawblauciak:ll_sched_numclients_underflow

Conversation

@slawblauciak

Copy link
Copy Markdown
Collaborator

In some situations the domain clear routine might encounter
a case, when the number of clients is decremented and 0,
as such, a subsequent subdivision will result in an integer underflow.

Signed-off-by: Slawomir Blauciak slawomir.blauciak@linux.intel.com

Fixes #3950
Fixes #3949
Fixes #3947

In some situations the domain clear routine might encounter
a case, when the number of clients is decremented and 0,
as such, a subsequent subdivision will result in an integer underflow.

Signed-off-by: Slawomir Blauciak <slawomir.blauciak@linux.intel.com>
@slawblauciak
slawblauciak requested a review from keyonjie April 23, 2021 15:28
@slawblauciak
slawblauciak requested a review from mrajwa as a code owner April 23, 2021 15:28
Comment on lines +334 to +335
if (atomic_read(&sch->domain->num_clients)) {
count = atomic_sub(&sch->domain->num_clients, 1);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if num_clients changes here between reading and subtracting. @lgirdwood @plbossart what do you think ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if num_clients changes here between reading and subtracting. @lgirdwood @plbossart what do you think ?

We hold the domain->lock during these checks so that should not happen.

BTW, we are kind of mess in this client/task management, I am working on refining them, please see the PR here: #4089

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, this is fine with the locks held.

@keyonjie keyonjie left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice observation @slawblauciak , we are somewhat mess in this client/task management part, better to do a thorough cleanup here, please see my PR #4089.

@lgirdwood lgirdwood left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good find @slawblauciak !

@lgirdwood

Copy link
Copy Markdown
Member

@keyonjie @slawblauciak can we use this in TGL 12 and integrate this alongside the cleanup in v1.8 ?

@slawblauciak

Copy link
Copy Markdown
Collaborator Author

@keyonjie @slawblauciak can we use this in TGL 12 and integrate this alongside the cleanup in v1.8 ?

Yeah, the plan is to use it with TGL 12 for sure. Not certain about Keyon's change yet.

@lgirdwood

Copy link
Copy Markdown
Member

@keyonjie lets clean up for v1.8 if things are messy today.

@lgirdwood
lgirdwood merged commit b500999 into thesofproject:main Apr 26, 2021
@keyonjie

Copy link
Copy Markdown
Contributor

@keyonjie lets clean up for v1.8 if things are messy today.

sounds good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

4 participants