Skip to contentSkip to conversation

idr: Rewrite ida

PATCH In review Kent Overstreet 2 participants 4 messages Jun 18, 2013 1 message not in the archive

Subsystems (2)
lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

#1 lore raw

PATCH 3/4 idr: Rewrite ida

This is a new, from scratch implementation of ida that should be simpler, faster and more space efficient.

Two primary reasons for the rewrite:
* A future patch will reimplement idr on top of this ida implementation + radix trees. Once that's done, the end result will be ~1k fewer lines of code, much simpler and easier to understand and it should be quite a bit faster.

* The performance improvements and addition of ganged allocation should make ida more suitable for use by a percpu id/tag allocator, which would then act as a frontend to this allocator.

The old ida implementation was done with the idr data structures - this was IMO backwards. I'll soon be reimplementing idr on top of this new ida implementation and radix trees - using a separate dedicated data structure for the free ID bitmap should actually make idr faster, and the end result is _significantly_ less code.

This implementation conceptually isn't that different from the old one - it's a tree of bitmaps, where one bit in a given node indicates whether or not there are free bits in a child node.

The main difference (and advantage) over the old version is that the tree isn't implemented with pointers - it's implemented in an array, like how heaps are implemented, which both better space efficiency and it'll be faster since there's no pointer chasing.

This does mean that the entire bitmap is stored in one contiguous memory allocation - and as currently implemented we won't be able to allocate _quite_ as many ids as with the previous implementation.

I don't expect this to be an issue in practice since anywhere this is used, an id corresponds to a struct allocation somewher else - we can't allocate an unbounded number of ids, we'll run out of memory somewhere else eventually, and I expect that to be the limiting factor in practice.

If a user/use case does come up where this matters I can add some sharding (or perhaps add a separate big_ida implementation) - but the extra complexity would adversely affect performance for the users that don't need > millions of ids, so I intend to leave the implementation as is until if and when this becomes an issue.

Cc Andrew Morton <[email protected]>
Cc Tejun Heo <[email protected]>
Cc Stephen Rothwell <[email protected]>
Cc Fengguang Wu <[email protected]>
---
 include/linux/idr.h | 118 +++++---
 lib/idr.c           | 776 +++++++++++++++++++++++++++++++++-------------------
 2 files changed, 573 insertions(+), 321 deletions(-)
include/linux/idr.h +82 -36
index c0e0c54..9169e18 100644
@@ -17,6 +17,88 @@
 #include <linux/init.h>
 #include <linux/rcupdate.h>
 
+/* IDA */
+
+#define IDA_INLINE_NODES	4
+
+struct ida {
+	spinlock_t		lock;
+
+	/*
+	 * cur_id and allocated_ids are for ida_alloc_cyclic. For cyclic
+	 * allocations we search for new ids to allocate starting from the last
+	 * id allocated - cur_id is the next id to try allocating.
+	 *
+	 * But we also don't want the allocated ids to be arbitrarily sparse -
+	 * the memory usage for the bitmap could be arbitrarily bad, and if
+	 * they're used as keys in a radix tree the memory overhead of the radix
+	 * tree could be quite bad as well. So we use allocated_ids to decide
+	 * when to restart cur_id from 0, and bound how sparse the bitmap can
+	 * be.
+	 */
+	unsigned		cur_id;
+	unsigned		allocated_ids;
+
+	/* size of ida->tree */
+	unsigned		nodes;
+
+	/*
+	 * Index of first leaf node in ida->tree; equal to the number of non
+	 * leaf nodes, ida->nodes - ida->first_leaf == number of leaf nodes
+	 */
+	unsigned		first_leaf;
+
+	unsigned long		*tree;
+	unsigned long		inline_nodes[IDA_INLINE_NODES];
+};
+
+#define IDA_INIT(name)						\
+{								\
+	.lock		= __SPIN_LOCK_UNLOCKED(name.lock),	\
+	.nodes		= IDA_INLINE_NODES,			\
+	.first_leaf	= 1,					\
+	.tree		= name.inline_nodes,			\
+}
+#define DEFINE_IDA(name)	struct ida name = IDA_INIT(name)
+
+void ida_remove(struct ida *ida, unsigned id);
+int ida_alloc_range(struct ida *ida, unsigned int start,
+		  unsigned int end, gfp_t gfp);
+int ida_alloc_cyclic(struct ida *ida, unsigned start, unsigned end, gfp_t gfp);
+void ida_destroy(struct ida *ida);
+int ida_init_prealloc(struct ida *ida, unsigned prealloc);
+
+/**
+ * ida_alloc_range - allocate a new id.
+ * @ida: the (initialized) ida.
+ * @gfp_mask: memory allocation flags
+ *
+ * Allocates an id in the range [0, INT_MAX]. Returns -ENOSPC if no ids are
+ * available, or -ENOMEM on memory allocation failure.
+ *
+ * Returns the smallest available id
+ *
+ * Use ida_remove() to get rid of an id.
+ */
+static inline int ida_alloc(struct ida *ida, gfp_t gfp_mask)
+{
+	return ida_alloc_range(ida, 0, 0, gfp_mask);
+}
+
+/**
+ * ida_init - initialize ida handle
+ * @ida:	ida handle
+ *
+ * This function is use to set up the handle (@ida) that you will pass
+ * to the rest of the functions.
+ */
+static inline void ida_init(struct ida *ida)
+{
+	ida_init_prealloc(ida, 0);
+}
+
+/* IDR */
+
 /*
  * We want shallower trees and thus more bits covered at each layer.  8
  * bits gives us large enough first layer for most use cases and maximum
@@ -195,42 +277,6 @@ static inline void __deprecated idr_remove_all(struct idr *idp)
 	__idr_remove_all(idp);
 }
 
-/*
- * IDA - IDR based id allocator, use when translation from id to
- * pointer isn't necessary.
- *
- * IDA_BITMAP_LONGS is calculated to be one less to accommodate
- * ida_bitmap->nr_busy so that the whole struct fits in 128 bytes.
- */
-#define IDA_CHUNK_SIZE		128	/* 128 bytes per chunk */
-#define IDA_BITMAP_LONGS	(IDA_CHUNK_SIZE / sizeof(long) - 1)
-#define IDA_BITMAP_BITS 	(IDA_BITMAP_LONGS * sizeof(long) * 8)
-
-struct ida_bitmap {
-	long			nr_busy;
-	unsigned long		bitmap[IDA_BITMAP_LONGS];
-};
-
-struct ida {
-	struct idr		idr;
-	struct ida_bitmap	*free_bitmap;
-};
-
-#define IDA_INIT(name)		{ .idr = IDR_INIT((name).idr), .free_bitmap = NULL, }
-#define DEFINE_IDA(name)	struct ida name = IDA_INIT(name)
-
-void ida_destroy(struct ida *ida);
-void ida_init(struct ida *ida);
-
-int ida_alloc_range(struct ida *ida, unsigned int start, unsigned int end,
-		   gfp_t gfp_mask);
-void ida_remove(struct ida *ida, unsigned int id);
-
-static inline int ida_alloc(struct ida *ida, gfp_t gfp_mask)
-{
-	return ida_alloc_range(ida, 0, 0, gfp_mask);
-}
-
 void __init idr_init_cache(void);
 
 #endif /* __IDR_H__ */
lib/idr.c +491 -285

797 lines not on this page · raw message

Signature · 1 line
1.8.3.1
#2 lore raw

PATCH 4/4 idr: Percpu ida

In reply to a message not in the archive

Percpu frontend for allocating ids. With percpu allocation (that works), it's impossible to guarantee it will always be possible to allocate all nr_tags - typically, some will be stuck on a remote percpu freelist where the current job can't get to them.

We do guarantee that it will always be possible to allocate at least (nr_tags / 2) tags - this is done by keeping track of which and how many cpus have tags on their percpu freelists. On allocation failure if enough cpus have tags that there could potentially be (nr_tags / 2) tags stuck on remote percpu freelists, we then pick a remote cpu at random to steal from.

Note that the synchronization is _definitely_ tricky - we're using xchg()/cmpxchg() on the percpu lists, to synchronize between steal_tags().

The alternative would've been adding a spinlock to protect the percpu freelists, but that would've required some different tricky code to avoid deadlock because of the lock ordering.

Note that there's no cpu hotplug notifier - we don't care, because steal_tags() will eventually get the down cpu's tags. We _could_ satisfy more allocations if we had a notifier - but we'll still meet our guarantees and it's absolutely not a correctness issue, so I don't think it's worth the extra code.

Cc Tejun Heo <[email protected]>
Cc Oleg Nesterov <[email protected]>
Cc Christoph Lameter <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Andi Kleen <[email protected]>
Cc Jens Axboe <[email protected]>
Cc "Nicholas A. Bellinger" <[email protected]>
---
 include/linux/idr.h |  48 ++++++++
 lib/idr.c           | 312 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 352 insertions(+), 8 deletions(-)
include/linux/idr.h +48 -0
index 9169e18..afaa071 100644
@@ -16,6 +16,8 @@
 #include <linux/bitops.h>
 #include <linux/init.h>
 #include <linux/rcupdate.h>
+#include <linux/spinlock_types.h>
+#include <linux/wait.h>
 
 /* IDA */
 
@@ -97,6 +99,52 @@ static inline void ida_init(struct ida *ida)
 	ida_init_prealloc(ida, 0);
 }
 
+/* Percpu IDA/tag allocator */
+
+struct percpu_ida_cpu;
+
+struct percpu_ida {
+	/*
+	 * number of tags available to be allocated, as passed to
+	 * percpu_ida_init()
+	 */
+	unsigned			nr_tags;
+
+	struct percpu_ida_cpu __percpu	*tag_cpu;
+
+	/*
+	 * Bitmap of cpus that (may) have tags on their percpu freelists:
+	 * steal_tags() uses this to decide when to steal tags, and which cpus
+	 * to try stealing from.
+	 *
+	 * It's ok for a freelist to be empty when its bit is set - steal_tags()
+	 * will just keep looking - but the bitmap _must_ be set whenever a
+	 * percpu freelist does have tags.
+	 */
+	unsigned long			*cpus_have_tags;
+
+	struct {
+		/*
+		 * When we go to steal tags from another cpu (see steal_tags()),
+		 * we want to pick a cpu at random. Cycling through them every
+		 * time we steal is a bit easier and more or less equivalent:
+		 */
+		unsigned		cpu_last_stolen;
+
+		/* For sleeping on allocation failure */
+		wait_queue_head_t	wait;
+
+		/* Global freelist */
+		struct ida		ida;
+	} ____cacheline_aligned_in_smp;
+};
+
+int percpu_ida_alloc(struct percpu_ida *pool, gfp_t gfp);
+void percpu_ida_free(struct percpu_ida *pool, unsigned tag);
+
+void percpu_ida_destroy(struct percpu_ida *pool);
+int percpu_ida_init(struct percpu_ida *pool, unsigned long nr_tags);
+
 /* IDR */
 
 /*
lib/idr.c +304 -8

327 lines not on this page · raw message

Signature · 1 line
1.8.3.1
#3 lore raw

Re: [PATCH 4/4] idr: Percpu ida

lib/idr.c int ida_init_prealloc(struct ida *ida, unsigned prealloc) · line 635

Quoted 12 lines · Kent Overstreet
lib/idr.c +12 -0
+static inline unsigned alloc_local_tag(struct percpu_ida *pool,
+				       struct percpu_ida_cpu *tags)
+{
+	int tag = -ENOSPC;
+
+	spin_lock(&tags->lock);
+	if (tags->nr_free)
+			tag = tags->freelist[--tags->nr_free];
+	spin_unlock(&tags->lock);
+
+	return tag;
+}

This could be made much faster by avoiding real atomics (coming with spinlocks) and using per cpu atomics instead. Slub f.e. uses a single linked per cpu list managed via this_cpu_cmpxchg.

In order to avoid locking completely the state of the percpu tag list must fit into one word that can be handled via the local cmpxchg (or two with the cmpxchg_double). May require some changes to the data structure.

#4 lore raw

Re: [PATCH 4/4] idr: Percpu ida

Quoted 18 lines · Christoph Lameter

lib/idr.c int ida_init_prealloc(struct ida *ida, unsigned prealloc) · line 635

Quoted 12 lines · Kent Overstreet
lib/idr.c +12 -0
+static inline unsigned alloc_local_tag(struct percpu_ida *pool,
+				       struct percpu_ida_cpu *tags)
+{
+	int tag = -ENOSPC;
+
+	spin_lock(&tags->lock);
+	if (tags->nr_free)
+			tag = tags->freelist[--tags->nr_free];
+	spin_unlock(&tags->lock);
+
+	return tag;
+}

This could be made much faster by avoiding real atomics (coming with spinlocks) and using per cpu atomics instead. Slub f.e. uses a single linked per cpu list managed via this_cpu_cmpxchg.

Actually, we do need the atomic ops - they're protecting against a different cpu grabbing our freelist with steal_tags().

The alternative (which Andrew Morton suggested and Jens implemented in his fork of an earlier version of this code) would be to use IPIs, but there are reasonable use cases (high ratio of cpus:nr_tags) where tag stealing is going to be reasonably common so we don't want it to suck too much. Plus, steal_tags() needs to try different cpu's freelists until it finds one with tags, IPIs would make that loop... problematic.

I actually originally used cmpxchg(), but then later in review I realized I had an ABA and couldn't figure out how to solve it, so that's when I switched to spinlocks (which Andrew Morton preferred anyways).

But I just realized a few minutes ago I've seen your solution to ABA with a stack, in the slub code - double word cmpxchg() with a transaction id :) I may try that again just to see how the code looks, but I'm not sure the difference in performance will be big enough to matter, give that this lock should basically never be contended.