-
Notifications
You must be signed in to change notification settings - Fork 178
Conversation
tuxoko
commented
May 21, 2016
- Restore CALLOUT_FLAG_ABSOLUTE in cv_timedwait_hires
- Fix splat-condvar on Linux 4.7
- Fix race in taskq_destroy and dynamic spawing
In 39cd90e, I mistakenly disabled the ability of using absolute expire time in cv_timedwait_hires. I don't quite sure why I did that, so let's restore it. Signed-off-by: Chunwei Chen <[email protected]>
Use schedule_timeout_interruptible instead of schedule for busy waiting. Otherwise, it will cause "BUG: workqueue lockup" on Linux 4.7 pre-rc. This should also reduce CPU usage in case we need to wait for a long time. Signed-off-by: Chunwei Chen <[email protected]> Fix openzfs#552
Looking at the taskq_destroy/spawning issue only: I'm wondering whether you really need tqt_is_dynamic. It would seem that taskq_destroy could only do the wait-for-nspawn==0 loop if TASKQ_DYNAMIC were set. Similarly the taskq_thread could only decrement nspawn if TASKQ_DYNAMIC is set although it ought to be harmless to decrement it no matter what since nspawn should only be looked at for a TASKQ_DYNAMIC taskq. |
Well, taskq_create will create one thread even if it's TASKQ_DYNAMIC. |
@tuxoko Interesting you should mention the single thread created for dynamic taskqs. There doesn't seem to be much of a need them to have a thread at all. What are your thoughts on 345f9be which changes things a few things in your patch? I've run the splat tests, ztest and a few hours (so far) of the test suite with it. |
@dweeezil |
@dweeezil |
tq->tq_nspawn--; | ||
spin_unlock_irqrestore(&tq->tq_lock, flags); | ||
if (taskq_thread_create(tq) == NULL) { | ||
/* restore spawning count if failed */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wondered at first the context of this comment (when looking at the original patch) and thought "restore to what"? How about:
/* cancel the request to spawn if thread creation fails */
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see how that makes it more understandable. The creation is already failed, and we are not actively cancel anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original comment was just fine once I thought about it for a moment.
@tuxoko LGTM. |
While taskq_destroy would wait for dynamic_taskq to finish its tasks, but it does not implies the thread being spawned is up and running. This will cause taskq to be freed before the thread can exit. We fix this by using tq_nspawn to indicate how many threads are being spawned before they are inserted to the thread list. And have taskq_destroy to wait for it to drop to zero. Signed-off-by: Chunwei Chen <[email protected]> Fix openzfs#550
wait_event is a macro, so the current implementation will cause re-evaluation of tq_next_id every time it wakes up. This would cause taskq_wait_outstanding(tq, 0) to be equivalent to taskq_wait(tq) Signed-off-by: Chunwei Chen <[email protected]>
/* get abs expire time */ | ||
tim += gethrtime(); | ||
if (!(flag & CALLOUT_FLAG_ABSOLUTE)) | ||
tim += gethrtime(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how I missed this when I looked over 39cd90e but yes we definitely need to restore this.
@tuxoko these three LGTM. e726590 Fix taskq_wait_outstanding re-evaluate tq_next_id However, I'd like to hold off on this one until things settle down in the mainline kernel. fc6269d Use schedule_timeout_interruptible for busy wait in splat-condvar.c |
I'm fine with that. |
In 39cd90e, I mistakenly disabled the ability of using absolute expire time in cv_timedwait_hires. I don't quite sure why I did that, so let's restore it. Signed-off-by: Chunwei Chen <[email protected]> Signed-off-by: Brian Behlendorf <[email protected]> Signed-off-by: Tim Chase <[email protected]> Issue #553
While taskq_destroy would wait for dynamic_taskq to finish its tasks, but it does not implies the thread being spawned is up and running. This will cause taskq to be freed before the thread can exit. We fix this by using tq_nspawn to indicate how many threads are being spawned before they are inserted to the thread list. And have taskq_destroy to wait for it to drop to zero. Signed-off-by: Chunwei Chen <[email protected]> Signed-off-by: Brian Behlendorf <[email protected]> Signed-off-by: Tim Chase <[email protected]> Issue #553 Closes #550
wait_event is a macro, so the current implementation will cause re- evaluation of tq_next_id every time it wakes up. This would cause taskq_wait_outstanding(tq, 0) to be equivalent to taskq_wait(tq) Signed-off-by: Chunwei Chen <[email protected]> Signed-off-by: Brian Behlendorf <[email protected]> Signed-off-by: Tim Chase <[email protected]> Issue #553
wait_event is a macro, so the current implementation will cause re- evaluation of tq_next_id every time it wakes up. This would cause taskq_wait_outstanding(tq, 0) to be equivalent to taskq_wait(tq) Signed-off-by: Chunwei Chen <[email protected]> Signed-off-by: Brian Behlendorf <[email protected]> Signed-off-by: Tim Chase <[email protected]> Issue openzfs#553
While taskq_destroy would wait for dynamic_taskq to finish its tasks, but it does not implies the thread being spawned is up and running. This will cause taskq to be freed before the thread can exit. We fix this by using tq_nspawn to indicate how many threads are being spawned before they are inserted to the thread list. And have taskq_destroy to wait for it to drop to zero. Signed-off-by: Chunwei Chen <[email protected]> Signed-off-by: Brian Behlendorf <[email protected]> Signed-off-by: Tim Chase <[email protected]> Issue openzfs#553 Closes openzfs#550
wait_event is a macro, so the current implementation will cause re- evaluation of tq_next_id every time it wakes up. This would cause taskq_wait_outstanding(tq, 0) to be equivalent to taskq_wait(tq) Signed-off-by: Chunwei Chen <[email protected]> Signed-off-by: Brian Behlendorf <[email protected]> Signed-off-by: Tim Chase <[email protected]> Issue openzfs#553
While taskq_destroy would wait for dynamic_taskq to finish its tasks, but it does not implies the thread being spawned is up and running. This will cause taskq to be freed before the thread can exit. We fix this by using tq_nspawn to indicate how many threads are being spawned before they are inserted to the thread list. And have taskq_destroy to wait for it to drop to zero. Signed-off-by: Chunwei Chen <[email protected]> Signed-off-by: Brian Behlendorf <[email protected]> Signed-off-by: Tim Chase <[email protected]> Issue openzfs#553 Closes openzfs#550