Date:	Mon, 4 Feb 2013 13:04:29 -0800
From:	Tejun Heo <tj@...nel.org>
To:	Lai Jiangshan <laijs@...fujitsu.com>
Cc:	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 00/13] workqueue: enhance locking and record global
 worker id for work data

Hello, Lai.

Generally, I *really* like where you're headed but like before it's a
bit difficult for me to apply the patches as-is.  Please read on.

On Fri, Feb 01, 2013 at 02:41:23AM +0800, Lai Jiangshan wrote:
> Better Locking:
> mainly based on *mb() which is the most dangerous code and bad for readability.
> This series change the usage of CWQ bit and makes these code simpler.
> 	--PATCH 3,4,5

Yeah, that's one ugly piece of memory barrier magic which has been
around forever.  I never bothered with it as it was fairly localized
and not broken.  I *do* like removing it.  A bit on the fence about
adding another field to delayed_work tho.  The @cpu addition was about
correctness but this one doesn't really buy us anything other than
cleaner code.  Folding the wq field into work_struct would be ugly,
right?  Hmmm....

> We have get_work_pool(), but it requires the caller do the later check and locking,
> we replace it which 3 better internal locking API. 1) More proper API and
> 2) merge the duplicated code and 3) simplify the caller.
> 	--PATCH 8,9,10

This mostly leads up to gwid change, right?

> get_work_pool()/get_work_pool_id() are called everywhere, something they are
> overkill(called idr_find() unneeded) and indirectly(caller knows it is onq or not),
> we replace them with get_work_cwq()/offq_work_pool_id()/locking APIs.
> 	--PATCH 3,4,5,6,8,9,10

Can't we just make get_work_pool_id() do a fast path if OFFQ than
requiring the user to distinguish off and on queue cases?

> Safely/one-step searching and worker id:
> ----------------------------------------
> 
> We are planing to add non-std worker_pool, but old get_work_pool() or new
> lock_pool_executing_work() was not prepared for this plan, idr_find(pool_id)
> is unsafe when we introduce free-able non-std worker_pool. Although we can
> fix it by adding rcu to worker_pool. but "recording global worker id for
> work data and adding rcu to worker" is another way and more straight forward.
> We implement the later one,  Now, lock_pool_executing_work() is ready for this plan.
> 	--PATCH 12,13
>
> When every time we need to find out the running worker from a work,
> we need two searches: search work_pool from work's data, and search worker
> from hash. We record global worker id for work data and we only need one search.
> 	--PATCH 13

While I'm a bit worried about capping total number of workers by the
amount bits left in work->data, if that doesn't cause any practical
issue (how many do we have available on 32bit?), I think this is the
better approach.  We couldn't do this before because work -> worker
relationship could be 1:N but it should now be doable.  Note that we
need RCU no matter what we index (pool or worker) to avoid locking on
each lookup.

So, I like both major changes made by the patchset and most changes
seem correct, well at least on casual review that is.

The problem is that I'm not very happy with the descriptions and
comments (what's up with the weird /** formatting?).  At least for me,
the patchset is quite difficult to follow.  I'm not sure whether it
has actual organizational issues or the descriptions aren't detailed /
clear enough yet.

>From past experience, I *think* it's gonna be a bit of struggle for
both of us to get the series in a shape that I would find acceptable
by reviewing and iterating, so I might just swallow it and regurgitate
into a form that I like.  Hmm.... dunno.  Will think about it.

Anyways, nice work.

Thanks.

-- 
tejun
--
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