| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20151119124644.GA3282@yuval-lab>
Date: Thu, 19 Nov 2015 14:46:45 +0200
From: Yuval Shaia <yuval.shaia@...cle.com>
To: Petr Mladek <pmladek@...e.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
Oleg Nesterov <oleg@...hat.com>, Tejun Heo <tj@...nel.org>,
Ingo Molnar <mingo@...hat.com>,
Peter Zijlstra <peterz@...radead.org>,
Steven Rostedt <rostedt@...dmis.org>,
"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
Josh Triplett <josh@...htriplett.org>,
Thomas Gleixner <tglx@...utronix.de>,
Linus Torvalds <torvalds@...ux-foundation.org>,
Jiri Kosina <jkosina@...e.cz>, Borislav Petkov <bp@...e.de>,
Michal Hocko <mhocko@...e.cz>, linux-mm@...ck.org,
Vlastimil Babka <vbabka@...e.cz>, linux-api@...r.kernel.org,
linux-kernel@...r.kernel.org, Doug Ledford <dledford@...hat.com>,
Sean Hefty <sean.hefty@...el.com>,
Hal Rosenstock <hal.rosenstock@...il.com>,
linux-rdma@...r.kernel.org
Subject: Re: [PATCH v3 18/22] IB/fmr_pool: Convert the cleanup thread into
kthread worker API
On Wed, Nov 18, 2015 at 02:25:23PM +0100, Petr Mladek wrote:
> Kthreads are currently implemented as an infinite loop. Each
> has its own variant of checks for terminating, freezing,
> awakening. In many cases it is unclear to say in which state
> it is and sometimes it is done a wrong way.
>
> The plan is to convert kthreads into kthread_worker or workqueues
> API. It allows to split the functionality into separate operations.
> It helps to make a better structure. Also it defines a clean state
> where no locks are taken, IRQs blocked, the kthread might sleep
> or even be safely migrated.
>
> The kthread worker API is useful when we want to have a dedicated
> single thread for the work. It helps to make sure that it is
> available when needed. Also it allows a better control, e.g.
> define a scheduling priority.
>
> This patch converts the frm_pool kthread into the kthread worker
s/frm/fmr
> API because I am not sure how busy the thread is. It is well
> possible that it does not need a dedicated kthread and workqueues
> would be perfectly fine. Well, the conversion between kthread
> worker API and workqueues is pretty trivial.
>
> The patch moves one iteration from the kthread into the work function.
> It preserves the check for a spurious queuing (wake up). Then it
> processes one request. Finally, it re-queues itself if more requests
> are pending.
>
> Otherwise, wake_up_process() is replaced by queuing the work.
>
> Important: The change is only compile tested. I did not find an easy
> way how to check it in a real life.
What are the expectations?
>
> Signed-off-by: Petr Mladek <pmladek@...e.com>
> CC: Doug Ledford <dledford@...hat.com>
> CC: Sean Hefty <sean.hefty@...el.com>
> CC: Hal Rosenstock <hal.rosenstock@...il.com>
> CC: linux-rdma@...r.kernel.org
> ---
> drivers/infiniband/core/fmr_pool.c | 54 ++++++++++++++++++--------------------
> 1 file changed, 25 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/infiniband/core/fmr_pool.c b/drivers/infiniband/core/fmr_pool.c
> index 9f5ad7cc33c8..5f2b06bd14da 100644
> --- a/drivers/infiniband/core/fmr_pool.c
> +++ b/drivers/infiniband/core/fmr_pool.c
> @@ -96,7 +96,8 @@ struct ib_fmr_pool {
> void * arg);
> void *flush_arg;
>
> - struct task_struct *thread;
> + struct kthread_worker *worker;
> + struct kthread_work work;
>
> atomic_t req_ser;
> atomic_t flush_ser;
> @@ -174,29 +175,26 @@ static void ib_fmr_batch_release(struct ib_fmr_pool *pool)
> spin_unlock_irq(&pool->pool_lock);
> }
>
> -static int ib_fmr_cleanup_thread(void *pool_ptr)
> +static void ib_fmr_cleanup_func(struct kthread_work *work)
> {
> - struct ib_fmr_pool *pool = pool_ptr;
> + struct ib_fmr_pool *pool = container_of(work, struct ib_fmr_pool, work);
>
> - do {
> - if (atomic_read(&pool->flush_ser) - atomic_read(&pool->req_ser) < 0) {
> - ib_fmr_batch_release(pool);
> -
> - atomic_inc(&pool->flush_ser);
> - wake_up_interruptible(&pool->force_wait);
> + /*
> + * The same request might be queued twice when it appears and
> + * by re-queuing from this work.
> + */
> + if (atomic_read(&pool->flush_ser) - atomic_read(&pool->req_ser) >= 0)
> + return;
>
> - if (pool->flush_function)
> - pool->flush_function(pool, pool->flush_arg);
> - }
> + ib_fmr_batch_release(pool);
> + atomic_inc(&pool->flush_ser);
> + wake_up_interruptible(&pool->force_wait);
>
> - set_current_state(TASK_INTERRUPTIBLE);
> - if (atomic_read(&pool->flush_ser) - atomic_read(&pool->req_ser) >= 0 &&
> - !kthread_should_stop())
> - schedule();
> - __set_current_state(TASK_RUNNING);
> - } while (!kthread_should_stop());
> + if (pool->flush_function)
> + pool->flush_function(pool, pool->flush_arg);
>
> - return 0;
> + if (atomic_read(&pool->flush_ser) - atomic_read(&pool->req_ser) < 0)
> + queue_kthread_work(pool->worker, &pool->work);
> }
>
> /**
> @@ -286,15 +284,13 @@ struct ib_fmr_pool *ib_create_fmr_pool(struct ib_pd *pd,
> atomic_set(&pool->flush_ser, 0);
> init_waitqueue_head(&pool->force_wait);
>
> - pool->thread = kthread_run(ib_fmr_cleanup_thread,
> - pool,
> - "ib_fmr(%s)",
> - device->name);
> - if (IS_ERR(pool->thread)) {
> - printk(KERN_WARNING PFX "couldn't start cleanup thread\n");
> - ret = PTR_ERR(pool->thread);
> + pool->worker = create_kthread_worker(0, "ib_fmr(%s)", device->name);
Is this patch depends on some other patch?
> + if (IS_ERR(pool->worker)) {
> + pr_warn(PFX "couldn't start cleanup kthread worker\n");
> + ret = PTR_ERR(pool->worker);
> goto out_free_pool;
> }
> + init_kthread_work(&pool->work, ib_fmr_cleanup_func);
>
> {
> struct ib_pool_fmr *fmr;
> @@ -362,7 +358,7 @@ void ib_destroy_fmr_pool(struct ib_fmr_pool *pool)
> LIST_HEAD(fmr_list);
> int i;
>
> - kthread_stop(pool->thread);
> + destroy_kthread_worker(pool->worker);
> ib_fmr_batch_release(pool);
>
> i = 0;
> @@ -412,7 +408,7 @@ int ib_flush_fmr_pool(struct ib_fmr_pool *pool)
> spin_unlock_irq(&pool->pool_lock);
>
> serial = atomic_inc_return(&pool->req_ser);
> - wake_up_process(pool->thread);
> + queue_kthread_work(pool->worker, &pool->work);
>
> if (wait_event_interruptible(pool->force_wait,
> atomic_read(&pool->flush_ser) - serial >= 0))
> @@ -526,7 +522,7 @@ int ib_fmr_pool_unmap(struct ib_pool_fmr *fmr)
> list_add_tail(&fmr->list, &pool->dirty_list);
> if (++pool->dirty_len >= pool->dirty_watermark) {
> atomic_inc(&pool->req_ser);
> - wake_up_process(pool->thread);
> + queue_kthread_work(pool->worker, &pool->work);
> }
> }
> }
> --
> 1.8.5.6
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists