From: Lai Jiangshan <eag0628@gmail.com>
To: Tejun Heo <tj@kernel.org>
Cc: Lai Jiangshan <laijs@cn.fujitsu.com>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 00/13] workqueue: enhance locking and record global worker id for work data
Date: Wed, 6 Feb 2013 00:19:26 +0800 [thread overview]
Message-ID: <CACvQF52Lqi2TuXc9oCdGkAzxcpE12YfuX=9-jp5tePsOqPBe6w@mail.gmail.com> (raw)
In-Reply-To: <20130204210429.GB27963@mtj.dyndns.org>
On Tue, Feb 5, 2013 at 5:04 AM, Tejun Heo <tj@kernel.org> wrote:
> 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?
old code, get_work_pool_id() is only called when offq.
after series applied, offq_work_worker_id() *must* be called only when offq,
and we can't offer get_work_worker_id().
so removing get_work_pool_id() and using offq_work_pool_id() instead
are preparing.
>
>> 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.
BUILD_BUG_ON((BITS_PER_LONG != 64) && (WORK_OFFQ_WORKER_SHIFT > 12));
Every worker needs at least 4k memory for its stack, the bits are enough if
WORK_OFFQ_WORKER_SHIFT <= 12.
>
> 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.
>
It is not nightmare for me! the work and discusses will consume most
time of my night, no night time for nightmare.
> Anyways, nice work.
>
I'm glad you like it. My daughter was born about 3month ago and I left
workqueue work then. I think it is time to pick up old pending
patches.
Thanks,
Lai
>
> --
> tejun
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
next prev parent reply other threads:[~2013-02-05 16:19 UTC|newest]
Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-01-31 18:41 [PATCH 00/13] workqueue: enhance locking and record global worker id for work data Lai Jiangshan
2013-01-31 18:41 ` [PATCH 01/13] workqueue: remove WORK_CPU_NONE Lai Jiangshan
2013-02-04 19:49 ` Tejun Heo
2013-02-05 12:03 ` Lai Jiangshan
2013-02-06 22:34 ` [PATCH wq/for-3.9] workqueue: replace WORK_CPU_NONE/LAST with WORK_CPU_END Tejun Heo
2013-01-31 18:41 ` [PATCH 02/13] workqueue: fix work_busy() Lai Jiangshan
2013-02-04 19:54 ` Tejun Heo
2013-02-05 12:06 ` Lai Jiangshan
2013-02-05 18:53 ` Tejun Heo
2013-02-06 11:42 ` Lai Jiangshan
2013-02-06 14:05 ` Tejun Heo
2013-02-06 23:02 ` [PATCH wq/for-3.9] workqueue: make work_busy() test WORK_STRUCT_PENDING first Tejun Heo
2013-01-31 18:41 ` [PATCH 03/13] workqueue: don't set work cwq until we queued it on pool Lai Jiangshan
2013-02-04 21:28 ` Tejun Heo
2013-02-05 15:00 ` Lai Jiangshan
2013-02-05 16:45 ` Tejun Heo
2013-02-06 11:35 ` Lai Jiangshan
2013-02-06 14:18 ` Tejun Heo
2013-02-05 15:06 ` Lai Jiangshan
2013-02-07 0:28 ` [PATCH wq/for-3.9 workqueue]: add delayed_work->wq to simplify reentrancy handling Tejun Heo
2013-01-31 18:41 ` [PATCH 04/13] workqueue: clear cwq when cancel the work Lai Jiangshan
2013-02-07 0:53 ` [PATCH wq/for-3.9] workqueue: make work->data point to pool after try_to_grab_pending() Tejun Heo
2013-01-31 18:41 ` [PATCH 05/13] workqueue: change queued detection and remove *mb()s Lai Jiangshan
2013-02-07 1:52 ` [PATCH wq/for-3.9] workqueue: simplify is-work-item-queued-here test Tejun Heo
2013-02-07 2:03 ` [PATCH wq/for-3.9] workqueue: cosmetic update in try_to_grab_pending() Tejun Heo
2013-01-31 18:41 ` [PATCH 06/13] workqueue: get pool id from work->data directly if it is offq Lai Jiangshan
2013-02-07 20:16 ` [PATCH wq/for-3.9] workqueue: make get_work_pool_id() cheaper Tejun Heo
2013-01-31 18:41 ` [PATCH 07/13] workqueue: get pool from wq/cwq Lai Jiangshan
2013-02-07 21:13 ` [PATCH wq/for-3.9] workqueue: pick cwq instead of pool in __queue_work() Tejun Heo
2013-01-31 18:41 ` [PATCH 08/13] workqueue: add lock_pool_executing_work() Lai Jiangshan
2013-02-04 21:34 ` Tejun Heo
2013-02-05 12:15 ` Lai Jiangshan
2013-02-05 19:09 ` Tejun Heo
2013-01-31 18:41 ` [PATCH 09/13] workqueue: add lock_pool_queued_work() Lai Jiangshan
2013-01-31 18:41 ` [PATCH 10/13] workqueue: add lock_pool_own_work() and remove get_work_pool() Lai Jiangshan
2013-02-04 21:38 ` Tejun Heo
2013-01-31 18:41 ` [PATCH 11/13] workqueue: allow more work_pool id space Lai Jiangshan
2013-01-31 18:41 ` [PATCH 12/13] workqueue: add worker's global worker ID Lai Jiangshan
2013-02-04 21:39 ` Tejun Heo
2013-01-31 18:41 ` [PATCH 13/13] workqueue: record global worker ID instead of pool ID in work->data when off-queue Lai Jiangshan
2013-02-04 21:40 ` Tejun Heo
2013-02-04 22:12 ` Tejun Heo
2013-02-05 15:18 ` Lai Jiangshan
2013-02-07 22:02 ` Tejun Heo
2013-02-13 22:23 ` Tejun Heo
2013-02-04 21:04 ` [PATCH 00/13] workqueue: enhance locking and record global worker id for work data Tejun Heo
2013-02-05 16:19 ` Lai Jiangshan [this message]
2013-02-05 16:50 ` Tejun Heo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CACvQF52Lqi2TuXc9oCdGkAzxcpE12YfuX=9-jp5tePsOqPBe6w@mail.gmail.com' \
--to=eag0628@gmail.com \
--cc=laijs@cn.fujitsu.com \
--cc=linux-kernel@vger.kernel.org \
--cc=tj@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).