On Mon, May 11, 2020 at 4:49 PM Tejun Heo <tj@xxxxxxxxxx> wrote: > > Hello, > > On Fri, May 08, 2020 at 04:46:51PM -0400, Lyude Paul wrote: > > +bool kthread_queue_flush_work(struct kthread_work *work, > > + struct kthread_flush_work *fwork); > > +void __kthread_flush_work_fn(struct kthread_work *work); > > As an exposed interface, this doesn't seem great. What the user wants to say > is "wait for the current instance of this guy" and the interface is asking > them to queue an extra work item whose queueing return state should be > checked and depending on that result wait on its internal completion. > > I'm skeptical this is a good idea in general given that unless you define > "this instance" at the time of queueing the work item which is being > waited-upon, there's no way to guarantee that the instance you're queueing > the flush work item on is the instance you want unless the queuer is holding > external synchronization which prevents the instance from running. That's a > really confusing semantics to expose in the interface. > > What the above means is that the ordering that you want is only defined > through your own locking and that maybe suggests that the sequencing should > be implemented on that side too. It may be a bit more code but a sequence > counter + wait queue might be the better solution here. Aside from this, flush_$stuff interfaces are very easy to deadlock. That's why e.g. flush_work() for normal workqueues has lockdep annotations (lockdep doesn't see through wait/wake_up dependencies without some help because cross-release didn't land for real). So I think if we need something like this, it needs to be a lot more explicit, and come with the right lockdep annotations. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ Nouveau mailing list Nouveau@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/nouveau
