Messages in this thread | | | Date | Mon, 10 Oct 2022 19:16:07 +0900 | Subject | Re: [PATCH v3] rapidio/tsi721: Replace flush_scheduled_work() with flush_work(). | From | Tetsuo Handa <> |
| |
On 2022/09/27 7:13, Tetsuo Handa wrote: > On 2022/09/27 0:06, Alan Stern wrote: >>> Alan Stern suggested to use cancel_work_sync() in >>> commit eef6a7d5c2f38ada ("workqueue: warn about flush_scheduled_work()") >>> and Tejun Heo suggested to use flush_work() in >>> https://lkml.kernel.org/r/YjivtdkpY+reW0Gt@slm.duckdns.org . >>> >>> Is there some reason to prefer one over the other? >>> I think that user-visible results between flush_work() and cancel_work_sync() >>> are the same because both wait until work completes. >> >> No, you haven't got it quite right. flush_work() waits until the work >> completes, but cancel_work_sync() first tries to cancel the work item. >> It then waits until the work item is either cancelled or completed. > > I know there is a difference if the cancellation was successful. > But unless cancel_work_sync() is called immediately after schedule_work(), > that work likely (e.g. 99%+) already started running or already completed. > >> >> If the cancellation is successful (i.e., it happens before the work item >> starts to run) then the call will return at that time and the work item >> will never run -- hence it will never complete. > > A difficult to judge thing is whether the owner/maintainer of that code wants > that work completed or cancelled. > Unlike e.g. https://lkml.kernel.org/r/Yy3byxFrfAfQL9xK@intel.com , > tsi721_remove() does not say whether pending works should run. >
It seems that Tejun is too busy to respond.
Although it is a bit worrisome that tsi721_db_dpc() unconditionally re-enables IDB interrupts when tsi721_remove() wants to disable all device interrupts (I don't know behavior/specification of tsi721 hardware), I think it is OK to go with flush_work() (as with other patches which removed flush_scheduled_work() usage) because flush_work() matches the behavior of flush_scheduled_work().
It is maintainer's responsibility to fix if re-enabling IDB interrupts is not safe, using a trick explained in e.g. commit a91b750fd6629354 ("net: rds: don't hold sock lock when cancelling work from rds_tcp_reset_callbacks()").
| |