Messages in this thread | | | Subject | Re: [PATCH-v2 1/3] percpu_ida: Make percpu_ida_alloc + callers accept task state bitmask | From | "Nicholas A. Bellinger" <> | Date | Tue, 21 Jan 2014 14:09:38 -0800 |
| |
On Mon, 2014-01-20 at 12:34 +0100, Peter Zijlstra wrote: > On Mon, Jan 20, 2014 at 03:44:44AM +0000, Nicholas A. Bellinger wrote: > > From: Kent Overstreet <kmo@daterainc.com> > > > > This patch changes percpu_ida_alloc() + callers to accept task state > > bitmask for prepare_to_wait() for code like target/iscsi that needs > > it for interruptible sleep, that is provided in a subsequent patch. > > > > It now expects TASK_UNINTERRUPTIBLE when the caller is able to sleep > > waiting for a new tag, or TASK_RUNNING when the caller cannot sleep, > > and is forced to return a negative value when no tags are available. > > > > v2 changes: > > - Include blk-mq + tcm_fc + vhost/scsi + target/iscsi changes > > - Drop signal_pending_state() call > > Urgh, you made me look at percpu_ida... steal_tags() does a > for_each_cpus() with IRQs disabled. This mean you'll disable IRQs for > multiple ticks on SGI class hardware. That is a _very_ long time indeed. >
So given the performance penalties involved in the steal tag slow path, consumers should typically be pre-allocating a larger number of percpu_ida tags than necessary to (ideally) avoid this logic completely.
> Then there's alloc_global_tags() vs alloc_local_tags(), one gets an > actual tag, while the other only moves tags about -- semantic mismatch. >
How about just in-lining alloc_global_tags() into percpu_ida_alloc()..?
> I do not get the comment near prepare to wait -- why does it matter if > percpu_ida_free() flips a cpus_have_tags bit? >
Mmm, not sure on that one.
> Given I don't understand this comment, its hard for me to properly > review the proposed patch series. >
Kent..?
--nab
| |