All of lore.kernel.org
 help / color / mirror / Atom feed
From: Tejun Heo <tj@kernel.org>
To: cluster-devel.redhat.com
Subject: [Cluster-devel] [PATCH 10/10] idr: Rework idr_preload()
Date: Wed, 19 Jun 2013 01:18:32 -0700	[thread overview]
Message-ID: <20130619081832.GC30681@mtj.dyndns.org> (raw)
In-Reply-To: <1371600150-23557-11-git-send-email-koverstreet@google.com>

Hello, Kent.

On Tue, Jun 18, 2013 at 05:02:30PM -0700, Kent Overstreet wrote:
> With the new ida implementation, that doesn't work anymore - the new ida
> code grows its bitmap tree by reallocating the entire thing in power of
> two increments. Doh.

So, I'm not sure about the single contiguous array implementation.  It
introduces some nasty characteristics such as possibly too large
contiguous allocation, bursty CPU usages, and loss of the ability to
handle ID allocations clustered in high areas - sure, the old ida
wasn't great on that part either but this can be much worse.

In addition, I'm not sure what this single contiguous allocation buys
us.  Let's say each leaf node size is 4k.  Even with in-array tree
implementation, it should be able to serve 2k IDs per-page, which
would be enough for most use cases and with a bit of caching it can
easily achieve both the performance benefits of array implementation
and the flexibility of allocating each leaf separately.  Even without
caching, the tree depth would normally be much lower than the current
implementation and the performance should be better.  If you're
bothered by having to implement another radix tree for it, it should
be possible to just implement the leaf node logic and use the existing
radix tree for internal nodes, right?

> So we need a slightly different trick. Note that if all allocations from
> an idr start by calling idr_prealloc() and disabling premption, at
> most nr_cpus() allocations can happen before someone calls
> idr_prealloc() again.

But we allow mixing preloaded and normal allocations and users are
allowed to allocate as many IDs they want after preloading.  It should
guarantee that the first allocation after preloading follows the
stronger allocation flag, and the above approach can't guarantee that.
You can declare that if preload is to work, all allocators of the ida
should preload and enforce it but that restriction arises only because
you're using this single array implementation.  It's another drawback
of this particular approach.  There seem to be a lot of cons and I
can't really see what the pros are.

Thanks.

-- 
tejun



WARNING: multiple messages have this Message-ID (diff)
From: Tejun Heo <tj@kernel.org>
To: Kent Overstreet <koverstreet@google.com>
Cc: Dmitry Torokhov <dtor@vmware.com>,
	David Airlie <airlied@linux.ie>,
	Davidlohr Bueso <davidlohr.bueso@hp.com>,
	Trond Myklebust <Trond.Myklebust@netapp.com>,
	nab@linux-iscsi.org, Sean Hefty <sean.hefty@intel.com>,
	Michel Lespinasse <walken@google.com>,
	John McCutchan <john@johnmccutchan.com>,
	Roland Dreier <roland@kernel.org>,
	Thomas Hellstrom <thellstrom@vmware.com>,
	linux1394-devel@lists.sourceforge.net,
	linux-scsi@vger.kernel.org, Robert Love <rlove@rlove.org>,
	linux-rdma@vger.kernel.org, cluster-devel@redhat.com,
	Stefan Richter <stefanr@s5r6.in-berlin.de>,
	Brian Paul <brianp@vmware.com>,
	Doug Gilbert <dgilbert@interlog.com>,
	Dave Airlie <airlied@redhat.com>,
	Hal Rosenstock <hal.rosenstock@gmail.com>,
	Rik van Riel <riel@redhat.com>,
	Erez Shitrit <erezsh@mellanox.co.il>
Subject: Re: [PATCH 10/10] idr: Rework idr_preload()
Date: Wed, 19 Jun 2013 01:18:32 -0700	[thread overview]
Message-ID: <20130619081832.GC30681@mtj.dyndns.org> (raw)
In-Reply-To: <1371600150-23557-11-git-send-email-koverstreet@google.com>

Hello, Kent.

On Tue, Jun 18, 2013 at 05:02:30PM -0700, Kent Overstreet wrote:
> With the new ida implementation, that doesn't work anymore - the new ida
> code grows its bitmap tree by reallocating the entire thing in power of
> two increments. Doh.

So, I'm not sure about the single contiguous array implementation.  It
introduces some nasty characteristics such as possibly too large
contiguous allocation, bursty CPU usages, and loss of the ability to
handle ID allocations clustered in high areas - sure, the old ida
wasn't great on that part either but this can be much worse.

In addition, I'm not sure what this single contiguous allocation buys
us.  Let's say each leaf node size is 4k.  Even with in-array tree
implementation, it should be able to serve 2k IDs per-page, which
would be enough for most use cases and with a bit of caching it can
easily achieve both the performance benefits of array implementation
and the flexibility of allocating each leaf separately.  Even without
caching, the tree depth would normally be much lower than the current
implementation and the performance should be better.  If you're
bothered by having to implement another radix tree for it, it should
be possible to just implement the leaf node logic and use the existing
radix tree for internal nodes, right?

> So we need a slightly different trick. Note that if all allocations from
> an idr start by calling idr_prealloc() and disabling premption, at
> most nr_cpus() allocations can happen before someone calls
> idr_prealloc() again.

But we allow mixing preloaded and normal allocations and users are
allowed to allocate as many IDs they want after preloading.  It should
guarantee that the first allocation after preloading follows the
stronger allocation flag, and the above approach can't guarantee that.
You can declare that if preload is to work, all allocators of the ida
should preload and enforce it but that restriction arises only because
you're using this single array implementation.  It's another drawback
of this particular approach.  There seem to be a lot of cons and I
can't really see what the pros are.

Thanks.

-- 
tejun

  reply	other threads:[~2013-06-19  8:18 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-06-19  0:02 [PATCH v2] lib/idr.c rewrite, percpu ida/tag allocator Kent Overstreet
2013-06-19  0:02 ` [PATCH 03/10] idr: Rewrite ida Kent Overstreet
2013-06-19  9:40   ` Steven Whitehouse
2013-06-19 23:38     ` Kent Overstreet
2013-06-21 16:33       ` David Teigland
2013-06-19  0:02 ` [PATCH 04/10] idr: Percpu ida Kent Overstreet
2013-06-19  0:02 ` [PATCH 05/10] idr: Kill old deprecated idr interfaces Kent Overstreet
2013-06-19  0:02 ` [PATCH 06/10] idr: Rename idr_get_next() -> idr_find_next() Kent Overstreet
2013-06-19  0:02 ` [PATCH 08/10] idr: Reimplement idr on top of ida/radix trees Kent Overstreet
     [not found] ` <1371600150-23557-1-git-send-email-koverstreet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
2013-06-19  0:02   ` [PATCH 07/10] idr: Rename idr_alloc() -> idr_alloc_range() Kent Overstreet
2013-06-19  0:02     ` [Drbd-dev] " Kent Overstreet
2013-06-19 16:44     ` Alex Williamson
2013-06-19 17:28     ` Davidlohr Bueso
2013-06-21  3:13     ` Vinod Koul
2013-06-19  0:02   ` [PATCH 09/10] idr: Remove unneeded idr locking, idr_preload() usage Kent Overstreet
2013-06-19  0:02     ` Kent Overstreet
2013-06-19  0:02 ` [Cluster-devel] [PATCH 10/10] idr: Rework idr_preload() Kent Overstreet
2013-06-19  0:02   ` Kent Overstreet
2013-06-19  8:18   ` Tejun Heo [this message]
2013-06-19  8:18     ` Tejun Heo
2013-06-19 23:33     ` [Cluster-devel] " Kent Overstreet
2013-06-19 23:33       ` Kent Overstreet
2013-06-19 23:46       ` [Cluster-devel] " Tejun Heo
2013-06-19 23:46         ` Tejun Heo
  -- strict thread matches above, loose matches on Subject: below --
2013-07-06  5:08 [PATCH v3] lib/idr.c rewrite, percpu ida/tag allocator Kent Overstreet
2013-07-06  5:08 ` [Cluster-devel] [PATCH 10/10] idr: Rework idr_preload() Kent Overstreet
2013-08-07 17:34 IDA/IDR rewrite, percpu ida Kent Overstreet
2013-08-07 17:46 ` [Cluster-devel] [PATCH 10/10] idr: Rework idr_preload() Kent Overstreet

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=20130619081832.GC30681@mtj.dyndns.org \
    --to=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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.