On 02/13/2010 03:03 AM, David Howells wrote:> Tejun Heo <tj@kernel.org> wrote:>> - requeue = slow_work_sleep_till_thread_needed(>> - &object->fscache.work, &timeout);>> - } while (timeout > 0 && !requeue);>> + timeout = schedule_timeout(timeout);>> + } while (timeout > 0);> > Okay, how do you stop the workqueue from having all its threads> blocking on pending work? The reason the code you've removed> interacts with the slow work facility in this way is that there can> be a dependency whereby an executing work item depends on something> that is queued. This code allows the thread to be given back to the> pool and processing deferred.

How deep the dependency chain can be? As I wrote in the patchdescription, wake-me-up-on-another-enqueue can be implemented insimilar way but I wasn't sure how useful it would be. If thedependency chain is strictly bound and significantly shorter than theallowed concurrency, it might be better to just leave them sleep.

If it's mainly because there can be many concurrent long waiters (butno dependency), implementing staggered timeout might be better option.I wasn't sure about the requirement there.

> Note that just creating more threads isn't a good answer - that can> run you out of resources instead.

It depends. The only resource taken up by an idle kthread is smallamount of memory and it can definitely be traded off against codecomplexity and processing overhead. Anyways, this really depends onwhat is the concurrency requirement there, can you please explain whatwould the bad cases be?

> I'm not sure that's a good idea - something like a tar command can> create thousands of objects, all of which will start undergoing> state changes.

The default concurrency level for slow-work is pretty low. Is itexpected to be tuned to a very high value in certain configurations?

> Why did you do this? Is it because cmwq does _not_ prevent reentrance to> executing work items? I take it that's why you can get away with this:

and yes, I used it as a cheap way to avoid reentrance. For mostcases, it works just fine. For slow work, it might not be enough.

> - slow_work_enqueue(&object->work);> + if (fscache_get_object(object) >= 0)> + if (!queue_work(fscache_object_wq, &object->work))> + fscache_put_object(object);> > One of the reasons I _don't_ want to use the old workqueue facility is that it> doesn't manage reentrancy. That can end up tying up multiple threads for one> long-duration work item.

Yeap, it's a drawback of the workqueue API although I don't think itwould be big enough to warrant a completely separate workpoolmechanism. It's usually enough to implement synchronization from thecallback or guarantee that running works don't get queued some otherway. What would happen if fscache object works are reentered? Wouldthere be correctness issues? How likely are they to get scheduledwhile being executed? If this is something critical, I have a draftimplementation which avoids reentrance. I was gonna apply it for allworks but it would cause too much cross CPU access when the wq userscan already handle reentrance but it can be implemented as optionalbehavior along with SINGLE_CPU.

> Note that it would still be useful to know whether an object was queued for> work or being executed.

Adding wouldn't be difficult but would it justify having a dedicatedfunction for that in workqueue where fscache would be the only user?Also please note that such information is only useful for debugging oras hints due to lack of synchronization.