linux-kernel - [RFC 03/10] lib/idr.c: only fill ida->idr when needed

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu,  8 Dec 2016 02:22:58 +0100
From:   Rasmus Villemoes <linux@...musvillemoes.dk>
To:     Tejun Heo <tj@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>
Cc:     linux-kernel@...r.kernel.org,
        Lai Jiangshan <jiangshanlai@...il.com>,
        Jens Axboe <axboe@...nel.dk>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Rasmus Villemoes <linux@...musvillemoes.dk>
Subject: [RFC 03/10] lib/idr.c: only fill ida->idr when needed

If I'm reading the code correctly, every call of ida_simple_get (and
almost all users of ida use that) starts by calling

  ida_pre_get -> __idr_pre_get

which fills up the free list of &ida->idr with MAX_IDR_FREE (aka 8)
struct idr_layers. After a successful id allocation, ida_get_new_above
gets rid of up to one of these (maybe some were used during the
allocation, but in the common case all 8 will still be there). On the
next call of ida_simple_get, we again fill up &ida->idr, and
deallocate at most one, which at least contradicts the comment at the
bottom of ida_get_new_above.

So rather than unconditionally calling ida_pre_get in ida_simple_get,
only do that if we get -EAGAIN.

But I'm not really convinced that this is a good way of doing
things. Many users of ida probably only ever allocates one or two ids,
so those would not hit the "free the excess idr_layers" often enough
to actually get rid of the extra memory. In practice, those two bits
could cost around 16KB of memory. But maybe the users shouldn't be
using ida in the first place, then. (For example, the watchdog code
always passes max=MAX_DOGS==32, so they'd be much better served with a
small fixed bitmap).

Signed-off-by: Rasmus Villemoes <linux@...musvillemoes.dk>
---
 lib/idr.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/idr.c b/lib/idr.c
index 1e786f817e66..6393feb4b087 100644
--- a/lib/idr.c
+++ b/lib/idr.c
@@ -1092,9 +1092,6 @@ int ida_simple_get(struct ida *ida, unsigned int start, unsigned int end,
 	}

 again:
-	if (!ida_pre_get(ida, gfp_mask))
-		return -ENOMEM;
-
 	spin_lock_irqsave(&simple_ida_lock, flags);
 	ret = ida_get_new_above(ida, start, &id);
 	if (!ret) {
@@ -1107,8 +1104,11 @@ int ida_simple_get(struct ida *ida, unsigned int start, unsigned int end,
 	}
 	spin_unlock_irqrestore(&simple_ida_lock, flags);

-	if (unlikely(ret == -EAGAIN))
+	if (unlikely(ret == -EAGAIN)) {
+		if (!ida_pre_get(ida, gfp_mask))
+			return -ENOMEM;
 		goto again;
+	}

 	return ret;
 }
-- 
2.1.4