[PATCH] fio: increase max smalloc pools

[PATCH] fio: increase max smalloc pools

[Christian Ehrhardt]

From: Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>

For our tests with about 250k files we found the smalloc pool being depleated.
Now for us values of 3-4 would be enough, but since it is a compile time switch
I'd like to make it safe for everybody and set 8.

Since it is a dynamic sizing anyway that should hopefully be ok for everybody.

Signed-off-by: Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>
---

[diffstat]

[diff]

---
 smalloc.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/smalloc.c
+++ b/smalloc.c
@@ -26,7 +26,7 @@
 #define SMALLOC_BPL	(SMALLOC_BPB * SMALLOC_BPI)
 
 #define INITIAL_SIZE	16*1024*1024	/* new pool size */
-#define MAX_POOLS	1		/* maximum number of pools to setup */
+#define MAX_POOLS	8		/* maximum number of pools to setup */
 
 #define SMALLOC_PRE_RED		0xdeadbeefU
 #define SMALLOC_POST_RED	0x5aa55aa5U

Re: [PATCH] fio: increase max smalloc pools

[Jens Axboe]

On 03/03/2015 04:44 AM, Christian Ehrhardt wrote:
> From: Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>
>
> For our tests with about 250k files we found the smalloc pool being depleated.
> Now for us values of 3-4 would be enough, but since it is a compile time switch
> I'd like to make it safe for everybody and set 8.
>
> Since it is a dynamic sizing anyway that should hopefully be ok for everybody.

The reason it was scaled down to 1 pool is because we could run into 
situations where one of the forked processes (or threads) would cause 
the expansion of pools, and smalloc() could then return memory that 
wasn't properly shared (or valid) between all jobs. This was recently 
found and fixed, and the smalloc code should probably just be updated to 
reflect that. We can't runtime add pools safely.

Right now it's 1 pool at 16MB - how about we just bump it to 64MB for 
that one pool? Or, alternatively, pre-add 4 pools initially when smalloc 
is setup?

-- 
Jens Axboe

Re: [PATCH] fio: increase max smalloc pools

[Jens Axboe]

[-- Attachment #1: Type: text/plain, Size: 1321 bytes --]

On 03/03/2015 03:03 PM, Jens Axboe wrote:
> On 03/03/2015 04:44 AM, Christian Ehrhardt wrote:
>> From: Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>
>>
>> For our tests with about 250k files we found the smalloc pool being
>> depleated.
>> Now for us values of 3-4 would be enough, but since it is a compile
>> time switch
>> I'd like to make it safe for everybody and set 8.
>>
>> Since it is a dynamic sizing anyway that should hopefully be ok for
>> everybody.
>
> The reason it was scaled down to 1 pool is because we could run into
> situations where one of the forked processes (or threads) would cause
> the expansion of pools, and smalloc() could then return memory that
> wasn't properly shared (or valid) between all jobs. This was recently
> found and fixed, and the smalloc code should probably just be updated to
> reflect that. We can't runtime add pools safely.
>
> Right now it's 1 pool at 16MB - how about we just bump it to 64MB for
> that one pool? Or, alternatively, pre-add 4 pools initially when smalloc
> is setup?

Something like the attached, does that work for you? That's 4 pools of 
16MB added. I think that's more flexible than (the more ideal) 1 pool of 
64MB, since fio can survive if later pool additions fail. Or we can bump 
it to 8x16 just to be on the safe side...

-- 
Jens Axboe


[-- Attachment #2: smalloc-pools.patch --]
[-- Type: text/x-patch, Size: 1637 bytes --]

diff --git a/smalloc.c b/smalloc.c
index 66f9ec0dd5c1..378881bb4560 100644
--- a/smalloc.c
+++ b/smalloc.c
@@ -26,7 +26,7 @@
 #define SMALLOC_BPL	(SMALLOC_BPB * SMALLOC_BPI)
 
 #define INITIAL_SIZE	16*1024*1024	/* new pool size */
-#define MAX_POOLS	1		/* maximum number of pools to setup */
+#define MAX_POOLS	4		/* maximum number of pools to setup */
 
 #define SMALLOC_PRE_RED		0xdeadbeefU
 #define SMALLOC_POST_RED	0x5aa55aa5U
@@ -230,11 +230,21 @@ out_fail:
 
 void sinit(void)
 {
-	int ret;
+	int i, ret;
 
 	lock = fio_rwlock_init();
-	ret = add_pool(&mp[0], INITIAL_SIZE);
-	assert(!ret);
+
+	for (i = 0; i < MAX_POOLS; i++) {
+		ret = add_pool(&mp[i], INITIAL_SIZE);
+		if (ret)
+			break;
+	}
+
+	/*
+	 * If we added at least one pool, we should be OK for most
+	 * cases.
+	 */
+	assert(i);
 }
 
 static void cleanup_pool(struct pool *pool)
@@ -442,16 +452,17 @@ static void *smalloc_pool(struct pool *pool, size_t size)
 
 void *smalloc(size_t size)
 {
-	unsigned int i;
+	unsigned int i, end_pool;
 
 	if (size != (unsigned int) size)
 		return NULL;
 
 	global_write_lock();
 	i = last_pool;
+	end_pool = nr_pools;
 
 	do {
-		for (; i < nr_pools; i++) {
+		for (; i < end_pool; i++) {
 			void *ptr = smalloc_pool(&mp[i], size);
 
 			if (ptr) {
@@ -461,20 +472,14 @@ void *smalloc(size_t size)
 			}
 		}
 		if (last_pool) {
-			last_pool = 0;
+			end_pool = last_pool;
+			last_pool = i = 0;
 			continue;
 		}
 
-		if (nr_pools + 1 > MAX_POOLS)
-			break;
-		else {
-			i = nr_pools;
-			if (add_pool(&mp[nr_pools], size))
-				goto out;
-		}
+		break;
 	} while (1);
 
-out:
 	global_write_unlock();
 	return NULL;
 }

Re: [PATCH] fio: increase max smalloc pools

[Christian Ehrhardt]

On 03/03/15 23:10, Jens Axboe wrote:
> On 03/03/2015 03:03 PM, Jens Axboe wrote:
> > On 03/03/2015 04:44 AM, Christian Ehrhardt wrote:
> >> From: Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>
> >>
> >> For our tests with about 250k files we found the smalloc pool being
> >> depleated.
> >> Now for us values of 3-4 would be enough, but since it is a compile
> >> time switch
> >> I'd like to make it safe for everybody and set 8.
> >>
> >> Since it is a dynamic sizing anyway that should hopefully be ok for
> >> everybody.
> >
> > The reason it was scaled down to 1 pool is because we could run into
> > situations where one of the forked processes (or threads) would cause
> > the expansion of pools, and smalloc() could then return memory that
> > wasn't properly shared (or valid) between all jobs. This was recently
> > found and fixed, and the smalloc code should probably just be updated to
> > reflect that. We can't runtime add pools safely.
> >
> > Right now it's 1 pool at 16MB - how about we just bump it to 64MB for
> > that one pool? Or, alternatively, pre-add 4 pools initially when smalloc
> > is setup?
>
> Something like the attached, does that work for you? That's 4 pools of
> 16MB added. I think that's more flexible than (the more ideal) 1 pool of
> 64MB, since fio can survive if later pool additions fail. Or we can bump
> it to 8x16 just to be on the safe side...
>
Hi,
I saw you already checked it in with 8 max pools and your new code to initialize on sinit.
We tested that and it works like a charm for our case now.

Thank you.

Re: [PATCH] fio: increase max smalloc pools

[Jens Axboe]

On 03/04/2015 05:16 AM, Christian Ehrhardt wrote:
> On 03/03/15 23:10, Jens Axboe wrote:
>> On 03/03/2015 03:03 PM, Jens Axboe wrote:
>> > On 03/03/2015 04:44 AM, Christian Ehrhardt wrote:
>> >> From: Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>
>> >>
>> >> For our tests with about 250k files we found the smalloc pool being
>> >> depleated.
>> >> Now for us values of 3-4 would be enough, but since it is a compile
>> >> time switch
>> >> I'd like to make it safe for everybody and set 8.
>> >>
>> >> Since it is a dynamic sizing anyway that should hopefully be ok for
>> >> everybody.
>> >
>> > The reason it was scaled down to 1 pool is because we could run into
>> > situations where one of the forked processes (or threads) would cause
>> > the expansion of pools, and smalloc() could then return memory that
>> > wasn't properly shared (or valid) between all jobs. This was recently
>> > found and fixed, and the smalloc code should probably just be
>> updated to
>> > reflect that. We can't runtime add pools safely.
>> >
>> > Right now it's 1 pool at 16MB - how about we just bump it to 64MB for
>> > that one pool? Or, alternatively, pre-add 4 pools initially when
>> smalloc
>> > is setup?
>>
>> Something like the attached, does that work for you? That's 4 pools of
>> 16MB added. I think that's more flexible than (the more ideal) 1 pool of
>> 64MB, since fio can survive if later pool additions fail. Or we can bump
>> it to 8x16 just to be on the safe side...
>>
> Hi,
> I saw you already checked it in with 8 max pools and your new code to
> initialize on sinit.
> We tested that and it works like a charm for our case now.

Yep, I ended up feeling fine about that patch. It also fixes a bug in 
the iteration of pools, where smalloc() would sometimes have failed too 
early.

-- 
Jens Axboe