Re: [RFC v4 02/12] kthread: Add kthread_(un)block_work_queuing() and kthread_work_queuable()

Tejun Heo
2020-05-11T15:02:56Z

On Fri, May 08, 2020 at 04:46:52PM -0400, Lyude Paul wrote:
> Add some simple wrappers around incrementing/decrementing
> kthread_work.cancelling under lock, along with checking whether queuing
> is currently allowed on a given kthread_work, which we'll use want to
> implement work cancelling with DRM's vblank work helpers.

Am I correct in assuming that what you want is "cancel this and block
further queueing until the state is cleared"? I agree that'd be something
really useful to have. That said, There are a few areas that I think can be
improved upon:

* It'd be better if we separate this state into its own thing rather than
mixing with canceling state which has potential to make things really
confusing. It doesn't have to be a separate field unless you want disable
depth for work item disable (and I don't think you do). It can just be a
high bit in the same field but I think the two states should be separate
one way or the other.

* I'm always a bit skeptical about state querying interfaces which aren't
synchronized to anything. They're useful in many cases but also prone to
being misused. If you absoultely have to have them, can you please add
explicit comment explaining the lack of synchronization around it - ie.
unless you're the one setting and clearing the flag and queueing the task,
it isn't synchronized against anything.

* In the same vein, I'm not too sure about stand-alone block interface.
Unless I'm the sole queuer or there are further locking around queueing,
what good does setting blocking do? There's no way to guarantee that the
flag is seen by someone else trying to queue it and trying to flush the
work item after queueing doesn't help either. The only way to make that
interface meaningful is doing it together with cancel - so, you say "block
further queueing and cancel / flush whatever is in flight or queued",
which actually gives you a useful invariant.

* A simliar argument can be made about unblock too although that's an a lot
more relaxed situation in that unblocking and queueing oneself always
works and that the user might not care which future instance of queueing
will start succeeding.

Thanks.

--
tejun