Skip to content
This repository has been archived by the owner on Feb 26, 2020. It is now read-only.

Implement taskq_dispatch_prealloc() interface #65

Closed
wants to merge 6 commits into from
Closed

Implement taskq_dispatch_prealloc() interface #65

wants to merge 6 commits into from

Conversation

prakashsurya
Copy link
Member

No description provided.

Prakash Surya added 6 commits December 6, 2011 09:56
This change adds the neglected SPLAT_TEST_FINI call for the
SPLAT_TASKQ_TEST6_ID, just as is done for the other 5 SPLAT_TASKQ_*
tests.

Signed-off-by: Prakash Surya <surya1@llnl.gov>
The spl_task structure was renamed to taskq_ent, and all of
its fields were renamed to have a prefix of 'tqent' rather
than 't'. This was to align with the naming convention which
the ZFS code assumes.  Previously these fields were private
so the name never mattered.

Signed-off-by: Prakash Surya <surya1@llnl.gov>
To lay the ground work for introducing the taskq_dispatch_prealloc()
interface, the tq_work_list and tq_threads fields had to be replaced
with new alternatives in the taskq_t structure.

The tq_threads field was replaced with tq_thread_list. Rather than
storing the pointers to the taskq's kernel threads in an array, they are
now stored as a list. In addition to laying the ground work for the
taskq_dispatch_prealloc() interface, this change could also enable taskq
threads to be dynamically created and destroyed as threads can now be
added and removed to this list relatively easily.

The tq_work_list field was replaced with tq_active_list. Instead of
keeping a list of taskq_ent_t's which are currently being serviced, a
list of taskq_threads currently servicing a taskq_ent_t is kept. This
frees up the taskq_ent_t's tqent_list field when it is being serviced
(i.e. now when a taskq_ent_t is being serviced, it's tqent_list field
will be empty).

Signed-off-by: Prakash Surya <surya1@llnl.gov>
Added another splat taskq test to ensure tasks can be recursively
submitted to a single task queue without issues. When the
taskq_dispatch_prealloc() interface is introduced, this use case can
potentially cause a deadlock if a taskq_ent_t is dispatched while it's
tqent_list field is not empty. This _should_ never be a problem with the
existing taskq_dispatch() interface.

Signed-off-by: Prakash Surya <surya1@llnl.gov>
This patch implements the taskq_dispatch_prealloc() interface which
was introduced by the following illumos-gate commit.  It allows for
a preallocated taskq_ent_t to be used when dispatching items to a
taskq.  This eliminates a memory allocation which helps minimize
lock contention in the taskq when dispatching functions.

    commit 5aeb94743e3be0c51e86f73096334611ae3a058e
    Author: Garrett D'Amore <garrett@nexenta.com>
    Date:   Wed Jul 27 07:13:44 2011 -0700

    734 taskq_dispatch_prealloc() desired
    943 zio_interrupt ends up calling taskq_dispatch with TQ_SLEEP

Signed-off-by: Prakash Surya <surya1@llnl.gov>
The splat-taskq test functions were slightly modified to exercise
the new taskq interface in addition to the old interface.  If the
old interface passes each of its tests, the new interface is
exercised.  Both sub tests (old interface and new interface) must
pass for each test as a whole to pass.

Signed-off-by: Prakash Surya <surya1@llnl.gov>
@behlendorf
Copy link
Contributor

Close, but the buildbot turned up a race which caused a crash on some of the systems.

RIP: 0010:[]  [] __taskq_destroy+0x91/0x110 [spl]
Process zpool (pid: 1377, threadinfo ffff880100c48000, task ffff88003f850b40)
Stack:
 ffff880106fa0000 ffff880106fa0160 0000000000000000 0000000000000000
<0> ffff880100c49d88 ffffffffa04f2500 0000000000000000 0000000000000002
<0> ffff880106fa0000 0000000000000000 ffff880100c49de8 ffffffffa04f999d
Call Trace:
 [] spa_deactivate+0x60/0x190 [zfs]
 [] spa_export_common+0xfd/0x310 [zfs]
 [] spa_destroy+0x1a/0x20 [zfs]
 [] zfs_ioc_pool_destroy+0x1e/0x40 [zfs]
 [] zfsdev_ioctl+0xfd/0x1d0 [zfs]
 [] vfs_ioctl+0x22/0xa0
 [] do_vfs_ioctl+0x84/0x580
 [] sys_ioctl+0x81/0xa0
 [] system_call_fastpath+0x16/0x1b

This looks like it was caused be accessing the tqt_thread_list while the kthreads were still terminating. It would probably be a good idea to add code to __taskq_destroy() after kthread_stop() signals all the threads to exit to wait until they do. This could be done by checking tq->tq_nthreads == 0. If I'd been running with debugging enabled I suspect I'd have hit this.

@behlendorf
Copy link
Contributor

Or something else... it looks like kthread_stop() blocks until the thread exits

behlendorf pushed a commit to behlendorf/spl that referenced this pull request Dec 14, 2011
The spl_task structure was renamed to taskq_ent, and all of
its fields were renamed to have a prefix of 'tqent' rather
than 't'. This was to align with the naming convention which
the ZFS code assumes.  Previously these fields were private
so the name never mattered.

Signed-off-by: Prakash Surya <surya1@llnl.gov>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Issue openzfs#65
behlendorf pushed a commit to behlendorf/spl that referenced this pull request Dec 14, 2011
To lay the ground work for introducing the taskq_dispatch_prealloc()
interface, the tq_work_list and tq_threads fields had to be replaced
with new alternatives in the taskq_t structure.

The tq_threads field was replaced with tq_thread_list. Rather than
storing the pointers to the taskq's kernel threads in an array, they are
now stored as a list. In addition to laying the ground work for the
taskq_dispatch_prealloc() interface, this change could also enable taskq
threads to be dynamically created and destroyed as threads can now be
added and removed to this list relatively easily.

The tq_work_list field was replaced with tq_active_list. Instead of
keeping a list of taskq_ent_t's which are currently being serviced, a
list of taskq_threads currently servicing a taskq_ent_t is kept. This
frees up the taskq_ent_t's tqent_list field when it is being serviced
(i.e. now when a taskq_ent_t is being serviced, it's tqent_list field
will be empty).

Signed-off-by: Prakash Surya <surya1@llnl.gov>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Issue openzfs#65
behlendorf pushed a commit to behlendorf/spl that referenced this pull request Dec 14, 2011
Added another splat taskq test to ensure tasks can be recursively
submitted to a single task queue without issue. When the
taskq_dispatch_prealloc() interface is introduced, this use case
can potentially cause a deadlock if a taskq_ent_t is dispatched
while its tqent_list field is not empty. This _should_ never be
a problem with the existing taskq_dispatch() interface.

Signed-off-by: Prakash Surya <surya1@llnl.gov>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Issue openzfs#65
behlendorf pushed a commit to behlendorf/spl that referenced this pull request Dec 14, 2011
This patch implements the taskq_dispatch_prealloc() interface which
was introduced by the following illumos-gate commit.  It allows for
a preallocated taskq_ent_t to be used when dispatching items to a
taskq.  This eliminates a memory allocation which helps minimize
lock contention in the taskq when dispatching functions.

    commit 5aeb94743e3be0c51e86f73096334611ae3a058e
    Author: Garrett D'Amore <garrett@nexenta.com>
    Date:   Wed Jul 27 07:13:44 2011 -0700

    734 taskq_dispatch_prealloc() desired
    943 zio_interrupt ends up calling taskq_dispatch with TQ_SLEEP

Signed-off-by: Prakash Surya <surya1@llnl.gov>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Issue openzfs#65
@behlendorf behlendorf mentioned this pull request Dec 14, 2011
behlendorf pushed a commit to behlendorf/spl that referenced this pull request Apr 30, 2012
As of the removal of the taskq work list made in commit:

    commit 2c02b71
    Author: Prakash Surya <surya1@llnl.gov>
    Date:   Mon Dec 5 17:32:48 2011 -0800

        Replace tq_work_list and tq_threads in taskq_t

        To lay the ground work for introducing the taskq_dispatch_prealloc()
        interface, the tq_work_list and tq_threads fields had to be replaced
        with new alternatives in the taskq_t structure.

the comment above taskq_wait_check has been incorrect. This change is an
attempt at bringing that description more in line with the current
implementation. Essentially, references to the old task work list had to
be updated to reference the new taskq thread active list.

Signed-off-by: Prakash Surya <surya1@llnl.gov>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Issue openzfs#65
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants