Re: [PATCH] Break up the single NBD lock into one per NBD device

Re: [PATCH] Break up the single NBD lock into one per NBD device

[Nicholas Thomas]

Hi,

I was wondering if there was any chance of resurrecting this patch: 

http://comments.gmane.org/gmane.linux.network/207233

My use case is a Linux kernel connecting to up to 6,144 NBD devices (in
practice, probably < 1,000 almost all the time) simultaneously - our
current deployment has a userspace implementation of NBD, and I came
across this patch while investigating potential scalability issues.

I've not performed any benchmarks yet, but I assume the issues Jerry Chu
mentions would affect my case too, and the asked-for changes to get it
accepted seemed minimal.

Regards,
-- 
Nick Thomas
Bytemark Computing

Re: [PATCH] Break up the single NBD lock into one per NBD device

[Jerry Chu]

The change was straightforward but according to David Miller it should go to
"Jens Axboe's block layer tree". You are welcome to make and submit the change
there. (I did not follow nor do I know where that tree is.)

Jerry

On Mon, Feb 4, 2013 at 11:55 PM, Nicholas Thomas <nick@bytemark.co.uk> wrote:
> Hi,
>
> I was wondering if there was any chance of resurrecting this patch:
>
> http://comments.gmane.org/gmane.linux.network/207233
>
> My use case is a Linux kernel connecting to up to 6,144 NBD devices (in
> practice, probably < 1,000 almost all the time) simultaneously - our
> current deployment has a userspace implementation of NBD, and I came
> across this patch while investigating potential scalability issues.
>
> I've not performed any benchmarks yet, but I assume the issues Jerry Chu
> mentions would affect my case too, and the asked-for changes to get it
> accepted seemed minimal.
>
> Regards,
> --
> Nick Thomas
> Bytemark Computing
>
>
>
>
>

[PATCH] NBD: Move from a global spinlock to one lock per NBD device

[Nicholas Thomas]

This patch is entirely based on a submission by Jerry Chu, incorporating
suggestions from Eric Dumazet. Modern, faster NICs make the original comment
on why a single lock is preferable incorrect; moving to one lock per NBD
device removes a performance bottleneck.

Original patch: http://permalink.gmane.org/gmane.linux.network/207233
    
Signed-off-by: Nick Thomas <nick@bytemark.co.uk>

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 043ddcc..d285b64 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -57,20 +57,9 @@ static unsigned int debugflags;
 
 static unsigned int nbds_max = 16;
 static struct nbd_device *nbd_dev;
+static spinlock_t *nbd_locks __read_mostly;
 static int max_part;
 
-/*
- * Use just one lock (or at most 1 per NIC). Two arguments for this:
- * 1. Each NIC is essentially a synchronization point for all servers
- *    accessed through that NIC so there's no need to have more locks
- *    than NICs anyway.
- * 2. More locks lead to more "Dirty cache line bouncing" which will slow
- *    down each lock to the point where they're actually slower than just
- *    a single lock.
- * Thanks go to Jens Axboe and Al Viro for their LKML emails explaining this!
- */
-static DEFINE_SPINLOCK(nbd_lock);
-
 #ifndef NDEBUG
 static const char *ioctl_cmd_to_ascii(int cmd)
 {
@@ -781,6 +770,15 @@ static int __init nbd_init(void)
 	if (!nbd_dev)
 		return -ENOMEM;
 
+	nbd_locks = kcalloc(nbds_max, sizeof(*nbd_locks), GFP_KERNEL);
+	if (!nbd_locks) {
+		kfree(nbd_dev);
+		return -ENOMEM;
+	}
+
+	for (i = 0; i < nbds_max; i++)
+		spin_lock_init(&nbd_locks[i]);
+
 	part_shift = 0;
 	if (max_part > 0) {
 		part_shift = fls(max_part);
@@ -812,7 +810,7 @@ static int __init nbd_init(void)
 		 * every gendisk to have its very own request_queue struct.
 		 * These structs are big so we dynamically allocate them.
 		 */
-		disk->queue = blk_init_queue(do_nbd_request, &nbd_lock);
+		disk->queue = blk_init_queue(do_nbd_request, &nbd_locks[i]);
 		if (!disk->queue) {
 			put_disk(disk);
 			goto out;
@@ -863,6 +861,7 @@ out:
 		put_disk(nbd_dev[i].disk);
 	}
 	kfree(nbd_dev);
+	kfree(nbd_locks);
 	return err;
 }
 
@@ -880,6 +879,7 @@ static void __exit nbd_cleanup(void)
 	}
 	unregister_blkdev(NBD_MAJOR, "nbd");
 	kfree(nbd_dev);
+	kfree(nbd_locks);
 	printk(KERN_INFO "nbd: unregistered device at major %d\n", NBD_MAJOR);
 }

Re: [PATCH] NBD: Move from a global spinlock to one lock per NBD device

[Benjamin LaHaise]

On Tue, Feb 05, 2013 at 04:14:27PM +0000, Nicholas Thomas wrote:
> This patch is entirely based on a submission by Jerry Chu, incorporating
> suggestions from Eric Dumazet. Modern, faster NICs make the original comment
> on why a single lock is preferable incorrect; moving to one lock per NBD
> device removes a performance bottleneck.
> 
> Original patch: http://permalink.gmane.org/gmane.linux.network/207233
...
> +static spinlock_t *nbd_locks __read_mostly;
...

This is about the worst way to split up a lock possible.  Most (all?) of the 
spinlocks across nbd devices are on the same cacheline, so performance will 
be limited by the rate of cacheline bounces for the lock.  It would be far 
better to embed the spinlock in the data structures that it will be 
protecting to avoid this expensive false-sharing.

		-ben

Re: [PATCH] NBD: Move from a global spinlock to one lock per NBD device

[Paul Clements]

On Tue, Feb 5, 2013 at 11:22 AM, Benjamin LaHaise <bcrl@kvack.org> wrote:
> On Tue, Feb 05, 2013 at 04:14:27PM +0000, Nicholas Thomas wrote:
>> This patch is entirely based on a submission by Jerry Chu, incorporating
>> suggestions from Eric Dumazet. Modern, faster NICs make the original comment
>> on why a single lock is preferable incorrect; moving to one lock per NBD
>> device removes a performance bottleneck.
>>
>> Original patch: http://permalink.gmane.org/gmane.linux.network/207233
> ...
>> +static spinlock_t *nbd_locks __read_mostly;
> ...
>
> This is about the worst way to split up a lock possible.  Most (all?) of the
> spinlocks across nbd devices are on the same cacheline, so performance will
> be limited by the rate of cacheline bounces for the lock.  It would be far
> better to embed the spinlock in the data structures that it will be
> protecting to avoid this expensive false-sharing.

Yes, embedding this in the nbd_device would be much better, no?

--
Paul