Page MenuHomeFreeBSD

Fix few issues of LinuxKPI workqueue
ClosedPublic

Authored by mav on Aug 6 2017, 11:11 PM.

Details

Summary

Testing drm-next Skylake suspend/resume I found problems with power management reference counting. After few suspend/resume cycles reference counter went negative. Investigation of that problem lead me to LinuxKPI workqueue wrappers, and in particular incorrect status reported by work cancel functions: they reported "successful" cancellation for works already completed in normal way. This patch fixes that plus some more edge cases around cancellation.

Test Plan

This patch removes bunch of drm-next error messages during suspend/resume cycle.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Thank you for reviewing this code.

sys/compat/linuxkpi/common/src/linux_work.c
313 ↗(On Diff #31681)

This return value in the executing state does not match the comment above the function. Can you update the comment above the function?

403 ↗(On Diff #31681)

This return value in the executing state does not match the comment above the function. Can you update the comment above the function?

sys/compat/linuxkpi/common/src/linux_work.c
313 ↗(On Diff #31681)

I'd say it does match. In case of WORK_ST_EXEC work has already completed or at least started to execute, so this is not a cancellation from the caller point of view. It is merely a draining to guarantee there are not memory references left.

403 ↗(On Diff #31681)

Same as above.

sys/compat/linuxkpi/common/src/linux_work.c
313 ↗(On Diff #31681)

OK

483 ↗(On Diff #31681)

Do you need to add:

case WORK_ST_CANCEL:

Here after your changes?

sys/compat/linuxkpi/common/src/linux_work.c
483 ↗(On Diff #31681)

It's a good question, but I have no answer to it. This state is not obvious. If we managed to cancel callout or task, then this state is in fact idle. May be we could add another atomic to change WORK_ST_CANCEL to to WORK_ST_IDLE in that case and then indeed add it here? What do you think?

sys/compat/linuxkpi/common/src/linux_work.c
483 ↗(On Diff #31681)

Pending in this case only means queued. The below one is do the other part. I guess that means WORK_ST_CANCEL is not a work queued state?

This revision was automatically updated to reflect the committed changes.