[PATCH] dm-crypt: remove per-cpu structure

[PATCH] dm-crypt: remove per-cpu structure

[Mikulas Patocka]

Dm-crypt used per-cpu structures to hold pointers to ablkcipher_request.
The code assumed that the work item keeps executing on a single CPU, so it
used no synchronization when accessing this structure.

When we disable a CPU by writing zero to
/sys/devices/system/cpu/cpu*/online, the work item could be moved to
another CPU. This causes crashes in dm-crypt because the code starts using
a wrong ablkcipher_request.

This patch fixes this bug by removing the percpu definition. The structure
ablkcipher_request is accessed via a pointer from convert_context.
Consequently, if the work item is rescheduled to a different CPU, the
thread still uses the same ablkcipher_request.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Cc: stable@vger.kernel.org

---
 drivers/md/dm-crypt.c |   61 +++++++++-----------------------------------------
 1 file changed, 12 insertions(+), 49 deletions(-)

Index: linux-3.14-rc1/drivers/md/dm-crypt.c
===================================================================
--- linux-3.14-rc1.orig/drivers/md/dm-crypt.c	2014-02-03 19:18:23.000000000 +0100
+++ linux-3.14-rc1/drivers/md/dm-crypt.c	2014-02-03 19:21:35.000000000 +0100
@@ -19,7 +19,6 @@
 #include <linux/crypto.h>
 #include <linux/workqueue.h>
 #include <linux/backing-dev.h>
-#include <linux/percpu.h>
 #include <linux/atomic.h>
 #include <linux/scatterlist.h>
 #include <asm/page.h>
@@ -43,6 +42,7 @@ struct convert_context {
 	struct bvec_iter iter_out;
 	sector_t cc_sector;
 	atomic_t cc_pending;
+	struct ablkcipher_request *req;
 };
 
 /*
@@ -111,15 +111,7 @@ struct iv_tcw_private {
 enum flags { DM_CRYPT_SUSPENDED, DM_CRYPT_KEY_VALID };
 
 /*
- * Duplicated per-CPU state for cipher.
- */
-struct crypt_cpu {
-	struct ablkcipher_request *req;
-};
-
-/*
- * The fields in here must be read only after initialization,
- * changing state should be in crypt_cpu.
+ * The fields in here must be read only after initialization.
  */
 struct crypt_config {
 	struct dm_dev *dev;
@@ -150,12 +142,6 @@ struct crypt_config {
 	sector_t iv_offset;
 	unsigned int iv_size;
 
-	/*
-	 * Duplicated per cpu state. Access through
-	 * per_cpu_ptr() only.
-	 */
-	struct crypt_cpu __percpu *cpu;
-
 	/* ESSIV: struct crypto_cipher *essiv_tfm */
 	void *iv_private;
 	struct crypto_ablkcipher **tfms;
@@ -192,11 +178,6 @@ static void clone_init(struct dm_crypt_i
 static void kcryptd_queue_crypt(struct dm_crypt_io *io);
 static u8 *iv_of_dmreq(struct crypt_config *cc, struct dm_crypt_request *dmreq);
 
-static struct crypt_cpu *this_crypt_config(struct crypt_config *cc)
-{
-	return this_cpu_ptr(cc->cpu);
-}
-
 /*
  * Use this to access cipher attributes that are the same for each CPU.
  */
@@ -903,16 +884,15 @@ static void kcryptd_async_done(struct cr
 static void crypt_alloc_req(struct crypt_config *cc,
 			    struct convert_context *ctx)
 {
-	struct crypt_cpu *this_cc = this_crypt_config(cc);
 	unsigned key_index = ctx->cc_sector & (cc->tfms_count - 1);
 
-	if (!this_cc->req)
-		this_cc->req = mempool_alloc(cc->req_pool, GFP_NOIO);
+	if (!ctx->req)
+		ctx->req = mempool_alloc(cc->req_pool, GFP_NOIO);
 
-	ablkcipher_request_set_tfm(this_cc->req, cc->tfms[key_index]);
-	ablkcipher_request_set_callback(this_cc->req,
+	ablkcipher_request_set_tfm(ctx->req, cc->tfms[key_index]);
+	ablkcipher_request_set_callback(ctx->req,
 	    CRYPTO_TFM_REQ_MAY_BACKLOG | CRYPTO_TFM_REQ_MAY_SLEEP,
-	    kcryptd_async_done, dmreq_of_req(cc, this_cc->req));
+	    kcryptd_async_done, dmreq_of_req(cc, ctx->req));
 }
 
 /*
@@ -921,7 +901,6 @@ static void crypt_alloc_req(struct crypt
 static int crypt_convert(struct crypt_config *cc,
 			 struct convert_context *ctx)
 {
-	struct crypt_cpu *this_cc = this_crypt_config(cc);
 	int r;
 
 	atomic_set(&ctx->cc_pending, 1);
@@ -932,7 +911,7 @@ static int crypt_convert(struct crypt_co
 
 		atomic_inc(&ctx->cc_pending);
 
-		r = crypt_convert_block(cc, ctx, this_cc->req);
+		r = crypt_convert_block(cc, ctx, ctx->req);
 
 		switch (r) {
 		/* async */
@@ -941,7 +920,7 @@ static int crypt_convert(struct crypt_co
 			reinit_completion(&ctx->restart);
 			/* fall through*/
 		case -EINPROGRESS:
-			this_cc->req = NULL;
+			ctx->req = NULL;
 			ctx->cc_sector++;
 			continue;
 
@@ -1040,6 +1019,7 @@ static struct dm_crypt_io *crypt_io_allo
 	io->sector = sector;
 	io->error = 0;
 	io->base_io = NULL;
+	io->ctx.req = NULL;
 	atomic_set(&io->io_pending, 0);
 
 	return io;
@@ -1065,6 +1045,8 @@ static void crypt_dec_pending(struct dm_
 	if (!atomic_dec_and_test(&io->io_pending))
 		return;
 
+	if (io->ctx.req)
+		mempool_free(io->ctx.req, cc->req_pool);
 	mempool_free(io, cc->io_pool);
 
 	if (likely(!base_io))
@@ -1492,8 +1474,6 @@ static int crypt_wipe_key(struct crypt_c
 static void crypt_dtr(struct dm_target *ti)
 {
 	struct crypt_config *cc = ti->private;
-	struct crypt_cpu *cpu_cc;
-	int cpu;
 
 	ti->private = NULL;
 
@@ -1505,13 +1485,6 @@ static void crypt_dtr(struct dm_target *
 	if (cc->crypt_queue)
 		destroy_workqueue(cc->crypt_queue);
 
-	if (cc->cpu)
-		for_each_possible_cpu(cpu) {
-			cpu_cc = per_cpu_ptr(cc->cpu, cpu);
-			if (cpu_cc->req)
-				mempool_free(cpu_cc->req, cc->req_pool);
-		}
-
 	crypt_free_tfms(cc);
 
 	if (cc->bs)
@@ -1530,9 +1503,6 @@ static void crypt_dtr(struct dm_target *
 	if (cc->dev)
 		dm_put_device(ti, cc->dev);
 
-	if (cc->cpu)
-		free_percpu(cc->cpu);
-
 	kzfree(cc->cipher);
 	kzfree(cc->cipher_string);
 
@@ -1588,13 +1558,6 @@ static int crypt_ctr_cipher(struct dm_ta
 	if (tmp)
 		DMWARN("Ignoring unexpected additional cipher options");
 
-	cc->cpu = __alloc_percpu(sizeof(*(cc->cpu)),
-				 __alignof__(struct crypt_cpu));
-	if (!cc->cpu) {
-		ti->error = "Cannot allocate per cpu state";
-		goto bad_mem;
-	}
-
 	/*
 	 * For compatibility with the original dm-crypt mapping format, if
 	 * only the cipher name is supplied, use cbc-plain.

Re: dm-crypt: remove per-cpu structure

[Mike Snitzer]

On Thu, Feb 20 2014 at  6:01pm -0500,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> Dm-crypt used per-cpu structures to hold pointers to ablkcipher_request.
> The code assumed that the work item keeps executing on a single CPU, so it
> used no synchronization when accessing this structure.
> 
> When we disable a CPU by writing zero to
> /sys/devices/system/cpu/cpu*/online, the work item could be moved to
> another CPU. This causes crashes in dm-crypt because the code starts using
> a wrong ablkcipher_request.
> 
> This patch fixes this bug by removing the percpu definition. The structure
> ablkcipher_request is accessed via a pointer from convert_context.
> Consequently, if the work item is rescheduled to a different CPU, the
> thread still uses the same ablkcipher_request.

Hi Mikulas,

Obviously avoiding crashes is more important than performance.

But are we losing performance by switching away from using percpu?  Do
we care?  I'd like to see the header to speak to the potential for
slowdown (if there is any).

Re: dm-crypt: remove per-cpu structure

[Mikulas Patocka]

On Thu, 20 Feb 2014, Mike Snitzer wrote:

> On Thu, Feb 20 2014 at  6:01pm -0500,
> Mikulas Patocka <mpatocka@redhat.com> wrote:
> 
> > Dm-crypt used per-cpu structures to hold pointers to ablkcipher_request.
> > The code assumed that the work item keeps executing on a single CPU, so it
> > used no synchronization when accessing this structure.
> > 
> > When we disable a CPU by writing zero to
> > /sys/devices/system/cpu/cpu*/online, the work item could be moved to
> > another CPU. This causes crashes in dm-crypt because the code starts using
> > a wrong ablkcipher_request.
> > 
> > This patch fixes this bug by removing the percpu definition. The structure
> > ablkcipher_request is accessed via a pointer from convert_context.
> > Consequently, if the work item is rescheduled to a different CPU, the
> > thread still uses the same ablkcipher_request.
> 
> Hi Mikulas,
> 
> Obviously avoiding crashes is more important than performance.
> 
> But are we losing performance by switching away from using percpu?  Do
> we care?  I'd like to see the header to speak to the potential for
> slowdown (if there is any).

There is one more allocation per request than before. I don't know how 
much does it cost.

We could also modify the code to use per_bio_data to save one allocation.

Mikulas

Re: dm-crypt: remove per-cpu structure

[Mike Snitzer]

On Thu, Feb 20 2014 at  7:10pm -0500,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> 
> 
> On Thu, 20 Feb 2014, Mike Snitzer wrote:
> 
> > On Thu, Feb 20 2014 at  6:01pm -0500,
> > Mikulas Patocka <mpatocka@redhat.com> wrote:
> > 
> > > Dm-crypt used per-cpu structures to hold pointers to ablkcipher_request.
> > > The code assumed that the work item keeps executing on a single CPU, so it
> > > used no synchronization when accessing this structure.
> > > 
> > > When we disable a CPU by writing zero to
> > > /sys/devices/system/cpu/cpu*/online, the work item could be moved to
> > > another CPU. This causes crashes in dm-crypt because the code starts using
> > > a wrong ablkcipher_request.
> > > 
> > > This patch fixes this bug by removing the percpu definition. The structure
> > > ablkcipher_request is accessed via a pointer from convert_context.
> > > Consequently, if the work item is rescheduled to a different CPU, the
> > > thread still uses the same ablkcipher_request.
> > 
> > Hi Mikulas,
> > 
> > Obviously avoiding crashes is more important than performance.
> > 
> > But are we losing performance by switching away from using percpu?  Do
> > we care?  I'd like to see the header to speak to the potential for
> > slowdown (if there is any).
> 
> There is one more allocation per request than before. I don't know how 
> much does it cost.

OK, any reason you didn't fix this up by using cpu hotplug hooks like
Tejun suggested?  Too complicated?
 
> We could also modify the code to use per_bio_data to save one allocation.

OK, sounds like a good win.  Can you write a separate followup patch
that makes use of per_bio_data?

Re: dm-crypt: remove per-cpu structure

[Mikulas Patocka]

On Thu, 20 Feb 2014, Mike Snitzer wrote:

> On Thu, Feb 20 2014 at  7:10pm -0500,
> Mikulas Patocka <mpatocka@redhat.com> wrote:
> 
> > On Thu, 20 Feb 2014, Mike Snitzer wrote:
> > 
> > > On Thu, Feb 20 2014 at  6:01pm -0500,
> > > Mikulas Patocka <mpatocka@redhat.com> wrote:
> > > 
> > > > Dm-crypt used per-cpu structures to hold pointers to ablkcipher_request.
> > > > The code assumed that the work item keeps executing on a single CPU, so it
> > > > used no synchronization when accessing this structure.
> > > > 
> > > > When we disable a CPU by writing zero to
> > > > /sys/devices/system/cpu/cpu*/online, the work item could be moved to
> > > > another CPU. This causes crashes in dm-crypt because the code starts using
> > > > a wrong ablkcipher_request.
> > > > 
> > > > This patch fixes this bug by removing the percpu definition. The structure
> > > > ablkcipher_request is accessed via a pointer from convert_context.
> > > > Consequently, if the work item is rescheduled to a different CPU, the
> > > > thread still uses the same ablkcipher_request.
> > > 
> > > Hi Mikulas,
> > > 
> > > Obviously avoiding crashes is more important than performance.
> > > 
> > > But are we losing performance by switching away from using percpu?  Do
> > > we care?  I'd like to see the header to speak to the potential for
> > > slowdown (if there is any).
> > 
> > There is one more allocation per request than before. I don't know how 
> > much does it cost.
> 
> OK, any reason you didn't fix this up by using cpu hotplug hooks like
> Tejun suggested?  Too complicated?

Yes, it would complicate the code. The patch that removes percpu pointers 
shortens the file, using cpu hotplug hooks would make it bigger.

> > We could also modify the code to use per_bio_data to save one allocation.
> 
> OK, sounds like a good win.  Can you write a separate followup patch
> that makes use of per_bio_data?

I will try it.

Mikulas

Re: dm-crypt: remove per-cpu structure

[Milan Broz]

On 02/21/2014 08:41 AM, Mikulas Patocka wrote:
>>>> Hi Mikulas,
>>>>
>>>> Obviously avoiding crashes is more important than performance.

Yes, but please keep these changes in linux-next for a while
before submitting this for stable or you will fix one rare bug and
slow down all existing users (with possible another fix to fix later)...

I know that taking cpu offline becomes more used these days
but still the performance is critical attribute for dmcrypt.

(And IIRC this cpu work relocation problem is there for years.)

>>>> But are we losing performance by switching away from using percpu?  Do
>>>> we care?  I'd like to see the header to speak to the potential for
>>>> slowdown (if there is any).
>>>
>>> There is one more allocation per request than before. I don't know how 
>>> much does it cost.

Could you please try it on some test machine in lab? I think Red Hat has
enough usable machines for such tests.

>> OK, any reason you didn't fix this up by using cpu hotplug hooks like
>> Tejun suggested?  Too complicated?
> 
> Yes, it would complicate the code. The patch that removes percpu pointers 
> shortens the file, using cpu hotplug hooks would make it bigger.

Since we prefer source file size to correct solution?

That said, dmcrypt becomes already too big. I already thought about separating
some things (keeping the core functionality in one place), e.g. IV generators
implementation is something what could be logically separated.
(Is it worth to do it?)

>>> We could also modify the code to use per_bio_data to save one allocation.
>>
>> OK, sounds like a good win.  Can you write a separate followup patch
>> that makes use of per_bio_data?
> 
> I will try it.

If you have some performance numbers, post it as well please.
(I would not be surprised that removing percpu struct is sometimes even better
for performance but this is just wild guess.)

And if you have better solution to dmcrypt parallel performance, post it too
but with real hw (with and without crypto acceleration) speed up numbers please. ;-)

Thanks,
Milan

Re: dm-crypt: remove per-cpu structure

[Mike Snitzer]

On Sat, Feb 22 2014 at  5:28am -0500,
Milan Broz <gmazyland@gmail.com> wrote:

> On 02/21/2014 08:41 AM, Mikulas Patocka wrote:
> >>>> Hi Mikulas,
> >>>>
> >>>> Obviously avoiding crashes is more important than performance.
> 
> Yes, but please keep these changes in linux-next for a while
> before submitting this for stable or you will fix one rare bug and
> slow down all existing users (with possible another fix to fix later)...
> 
> I know that taking cpu offline becomes more used these days
> but still the performance is critical attribute for dmcrypt.
> 
> (And IIRC this cpu work relocation problem is there for years.)

Unfortunately, we cannot leave the code in its current state.  Sure it
has been this way for years but taking cpus offline is _much_ more
common.

As I said here:
https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=for-next&id=fba2d94c5e0f3d848792981977b1f62ce96377ac

This change may undermine performance improvements intended by commit
c0297721 ("dm crypt: scale to multiple cpus").  But correctness is more
important than performance.  The use of per-cpu structures may be
revisited later (by using cpu hot-plug notifiers to make them safe).

> >>>> But are we losing performance by switching away from using percpu?  Do
> >>>> we care?  I'd like to see the header to speak to the potential for
> >>>> slowdown (if there is any).
> >>>
> >>> There is one more allocation per request than before. I don't know how 
> >>> much does it cost.
> 
> Could you please try it on some test machine in lab? I think Red Hat has
> enough usable machines for such tests.
> 
> >> OK, any reason you didn't fix this up by using cpu hotplug hooks like
> >> Tejun suggested?  Too complicated?
> > 
> > Yes, it would complicate the code. The patch that removes percpu pointers 
> > shortens the file, using cpu hot-plug hooks would make it bigger.
> 
> Since we prefer source file size to correct solution?

Right, that was my thought too.  Definitely not a good enough
justification.  But a proper fix with cpu hot-plug notifier is more
involved for sure.  It will take time.

Do you have time to look at fixing this issue?

> That said, dmcrypt becomes already too big. I already thought about separating
> some things (keeping the core functionality in one place), e.g. IV generators
> implementation is something what could be logically separated.
> (Is it worth to do it?)
> 
> >>> We could also modify the code to use per_bio_data to save one allocation.
> >>
> >> OK, sounds like a good win.  Can you write a separate followup patch
> >> that makes use of per_bio_data?
> > 
> > I will try it.
> 
> If you have some performance numbers, post it as well please.
> (I would not be surprised that removing percpu struct is sometimes even better
> for performance but this is just wild guess.)
> 
> And if you have better solution to dmcrypt parallel performance, post it too
> but with real hw (with and without crypto acceleration) speed up numbers please. ;-)

Mikulas does have an alternative approach that should be revisited ASAP
given that it also fixes this cpu hotplug issue:
http://people.redhat.com/mpatocka/patches/kernel/dm-crypt-paralelizace/current/series.html

context/timeline:
http://www.redhat.com/archives/dm-devel/2011-October/msg00127.html
http://www.redhat.com/archives/dm-devel/2012-March/msg00181.html
http://www.redhat.com/archives/dm-devel/2013-March/msg00103.html

Christoph pointed out there is precendence for sorting:
http://www.redhat.com/archives/dm-devel/2013-March/msg00104.html

Even though you actively resisted it:
http://www.redhat.com/archives/dm-devel/2013-March/msg00107.html

Progress on improving dm-crypt is long overdue, I'd like to see Mikulas
rebase/repost/retest his dm-crypt parallelization patchset for 3.15 or
3.16.

Mike

Re: dm-crypt: remove per-cpu structure

[Milan Broz]

On 02/22/2014 03:25 PM, Mike Snitzer wrote:
> On Sat, Feb 22 2014 at  5:28am -0500,
> Milan Broz <gmazyland@gmail.com> wrote:
> 
>> On 02/21/2014 08:41 AM, Mikulas Patocka wrote:
>>>>>> Hi Mikulas,
>>>>>>
>>>>>> Obviously avoiding crashes is more important than performance.
>>
>> Yes, but please keep these changes in linux-next for a while
>> before submitting this for stable or you will fix one rare bug and
>> slow down all existing users (with possible another fix to fix later)...
>>
>> I know that taking cpu offline becomes more used these days
>> but still the performance is critical attribute for dmcrypt.
>>
>> (And IIRC this cpu work relocation problem is there for years.)
> 
> Unfortunately, we cannot leave the code in its current state.  Sure it
> has been this way for years but taking cpus offline is _much_ more
> common.
> 
> As I said here:
> https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=for-next&id=fba2d94c5e0f3d848792981977b1f62ce96377ac
> 
> This change may undermine performance improvements intended by commit
> c0297721 ("dm crypt: scale to multiple cpus").  But correctness is more
> important than performance.  The use of per-cpu structures may be
> revisited later (by using cpu hot-plug notifiers to make them safe).

Correctness? Yes. I think Tejun said how to fix it correctly.

I have nothing against removal of percpu structure - but please assess
what it breaks, how it will slow down dmcrypt and mention it in patch
header if you want quick fix.

I couldn't resist but "may undermine" in patch header sounds like
"we have no idea what it breaks but it no longer crashes".

Removal of headache by removing head is always 100% solution to
the particular problem but it has usually also some side effects :-)

...

> Mikulas does have an alternative approach that should be revisited ASAP
> given that it also fixes this cpu hotplug issue:
> http://people.redhat.com/mpatocka/patches/kernel/dm-crypt-paralelizace/current/series.html

Yes, and I spent many hours testing it and while it helped somewhere
it was not perfect solution in every situation. And I wish it works!
Unfortunately, the tests output was not convincing.
But it was described in thread you referenced.

And if he just repost the same patchset few month later without any
more data and no changes, why should I change my opinion?
I can just shut up of course and just maintain userspace and watch
people complaining.

> context/timeline:
> http://www.redhat.com/archives/dm-devel/2011-October/msg00127.html
> http://www.redhat.com/archives/dm-devel/2012-March/msg00181.html
> http://www.redhat.com/archives/dm-devel/2013-March/msg00103.html
> 
> Christoph pointed out there is precendence for sorting:
> http://www.redhat.com/archives/dm-devel/2013-March/msg00104.html

My opinion is that fixing io scheduler work (sorting) in dmcrypt is
just ugly hack and could lead to more problems in future because
it can increase latency and it anticipates some IO access patterns.
Dm-crypt should be (ideally) completely transparent and "invisible"
to IO processing...

I think there is a report of very slow operation of database (IIRC it was
MySQL) running over dmcrypt device. This would be probably nice test
case for these changes.

But I am not nacking it, if you want it, do it, you are maintainer.


> Even though you actively resisted it:
> http://www.redhat.com/archives/dm-devel/2013-March/msg00107.html

I am repeating still repeating the same: Do we have performance
numbers proving it is better in real world scenarios?
Nobody will use ramdisk for dmcrypt.

These are the most used scenarios for dmcrypt I know :

- Notebook with single SSD / rotational disk and multicore (2 or 4) CPU
with AES-NI acceleration, dmcrypt over plain partition or LVM
(typical encrypted distro installation)

- dmcrypt over MD RAID1/5/6 with several multicore CPUs
(think encrypted array for backups on server)
[There is also variant with several dmcrypt devices with MD RAID
over it - which is currently performance disaster - but I am
not sure this is good idea. It was suggested as workaround
before percpu changes were implemented.]

- several dmcrypt devices in system with many cores over LVM
(encrypted storage for VMs. It would be nice to have
fixed CPU limit per dmcrypt device - you do not want to one VM
eat all processing power. I think we discussed with Mikulas
that dmcrypt should have some tweaking parameters to limit used
CPUs per device. But it was long time ago...)

It would be also interesting how it behaves when stacking dmcrypt
devices on top of each other (this is used in TrueCrypt compatibility
mode in crypsetup, but also in TrueCrypt itself).

> Progress on improving dm-crypt is long overdue, I'd like to see Mikulas
> rebase/repost/retest his dm-crypt parallelization patchset for 3.15 or
> 3.16.

I agree and I would put accent on _improving_ in this sentence ;-)

Milan

Re: dm-crypt: remove per-cpu structure

[Mike Snitzer]

On Sat, Feb 22 2014 at 12:34pm -0500,
Milan Broz <gmazyland@gmail.com> wrote:

> On 02/22/2014 03:25 PM, Mike Snitzer wrote:
> > On Sat, Feb 22 2014 at  5:28am -0500,
> > Milan Broz <gmazyland@gmail.com> wrote:
> > 
> >> On 02/21/2014 08:41 AM, Mikulas Patocka wrote:
> >>>>>> Hi Mikulas,
> >>>>>>
> >>>>>> Obviously avoiding crashes is more important than performance.
> >>
> >> Yes, but please keep these changes in linux-next for a while
> >> before submitting this for stable or you will fix one rare bug and
> >> slow down all existing users (with possible another fix to fix later)...
> >>
> >> I know that taking cpu offline becomes more used these days
> >> but still the performance is critical attribute for dmcrypt.
> >>
> >> (And IIRC this cpu work relocation problem is there for years.)
> > 
> > Unfortunately, we cannot leave the code in its current state.  Sure it
> > has been this way for years but taking cpus offline is _much_ more
> > common.
> > 
> > As I said here:
> > https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=for-next&id=fba2d94c5e0f3d848792981977b1f62ce96377ac
> > 
> > This change may undermine performance improvements intended by commit
> > c0297721 ("dm crypt: scale to multiple cpus").  But correctness is more
> > important than performance.  The use of per-cpu structures may be
> > revisited later (by using cpu hot-plug notifiers to make them safe).
> 
> Correctness? Yes. I think Tejun said how to fix it correctly.

OK, patches welcome.

> I have nothing against removal of percpu structure - but please assess
> what it breaks, how it will slow down dmcrypt and mention it in patch
> header if you want quick fix.
> 
> I couldn't resist but "may undermine" in patch header sounds like
> "we have no idea what it breaks but it no longer crashes".
> 
> Removal of headache by removing head is always 100% solution to
> the particular problem but it has usually also some side effects :-)

Show me that the percpu performance is worth the risk of crashing the
kernel.  I'd love to see you prove that.

If Mikulas' changes were embraced 2.5 years ago we'd be in a much better
place than we are now.  I'm making the decision to affect change.  You
may not agree with it but I'm confident dm-crypt will be better for it.
 
> > Even though you actively resisted it:
> > http://www.redhat.com/archives/dm-devel/2013-March/msg00107.html
> 
> I am repeating still repeating the same: Do we have performance
> numbers proving it is better in real world scenarios?
> Nobody will use ramdisk for dmcrypt.
> 
> These are the most used scenarios for dmcrypt I know :
> 
> - Notebook with single SSD / rotational disk and multicore (2 or 4) CPU
> with AES-NI acceleration, dmcrypt over plain partition or LVM
> (typical encrypted distro installation)
> 
> - dmcrypt over MD RAID1/5/6 with several multicore CPUs
> (think encrypted array for backups on server)
> [There is also variant with several dmcrypt devices with MD RAID
> over it - which is currently performance disaster - but I am
> not sure this is good idea. It was suggested as workaround
> before percpu changes were implemented.]
> 
> - several dmcrypt devices in system with many cores over LVM
> (encrypted storage for VMs. It would be nice to have
> fixed CPU limit per dmcrypt device - you do not want to one VM
> eat all processing power. I think we discussed with Mikulas
> that dmcrypt should have some tweaking parameters to limit used
> CPUs per device. But it was long time ago...)
> 
> It would be also interesting how it behaves when stacking dmcrypt
> devices on top of each other (this is used in TrueCrypt compatibility
> mode in crypsetup, but also in TrueCrypt itself).

Thanks for the configuration scenarios.  I think I've asked this of you
before but: Do you have any automated testing?  Something that we hand a
blockdevice to and it produces a result that you find meaningful to use
as a point of comparison.
 
> > Progress on improving dm-crypt is long overdue, I'd like to see Mikulas
> > rebase/repost/retest his dm-crypt parallelization patchset for 3.15 or
> > 3.16.
> 
> I agree and I would put accent on _improving_ in this sentence ;-)

Uh huh...

Re: dm-crypt: remove per-cpu structure

[Milan Broz]

On 02/22/2014 07:29 PM, Mike Snitzer wrote:

> Thanks for the configuration scenarios.  I think I've asked this of you
> before but: Do you have any automated testing?  Something that we hand a
> blockdevice to and it produces a result that you find meaningful to use
> as a point of comparison.

Ask Ondra, he should have all code what I used (I think he modified it later).
(It was some fio scripts, stacked dmcrypt test, some basic dt and fsx tests and
some simple seek test.)

I run it against several versions (no patch, without sort part, etc)
and tried to compare times and throughput.

It is of course possible I did some stupid mistake.

On top of it, I do not have anything better.
And I do not have any suitable hw for testing now, so I can just say my opinion.

I just hope you have more facts than you mentioned here to support your decision
to merge these changes.

Milan

Re: dm-crypt: remove per-cpu structure

[Mikulas Patocka]

On Sat, 22 Feb 2014, Milan Broz wrote:

> On 02/22/2014 03:25 PM, Mike Snitzer wrote:
> > On Sat, Feb 22 2014 at  5:28am -0500,
> > Milan Broz <gmazyland@gmail.com> wrote:
> > 
> >> On 02/21/2014 08:41 AM, Mikulas Patocka wrote:
> >>>>>> Hi Mikulas,
> >>>>>>
> >>>>>> Obviously avoiding crashes is more important than performance.
> >>
> >> Yes, but please keep these changes in linux-next for a while
> >> before submitting this for stable or you will fix one rare bug and
> >> slow down all existing users (with possible another fix to fix later)...
> >>
> >> I know that taking cpu offline becomes more used these days
> >> but still the performance is critical attribute for dmcrypt.
> >>
> >> (And IIRC this cpu work relocation problem is there for years.)
> > 
> > Unfortunately, we cannot leave the code in its current state.  Sure it
> > has been this way for years but taking cpus offline is _much_ more
> > common.
> > 
> > As I said here:
> > https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=for-next&id=fba2d94c5e0f3d848792981977b1f62ce96377ac
> > 
> > This change may undermine performance improvements intended by commit
> > c0297721 ("dm crypt: scale to multiple cpus").  But correctness is more
> > important than performance.  The use of per-cpu structures may be
> > revisited later (by using cpu hot-plug notifiers to make them safe).
> 
> Correctness? Yes. I think Tejun said how to fix it correctly.
> 
> I have nothing against removal of percpu structure - but please assess
> what it breaks, how it will slow down dmcrypt and mention it in patch
> header if you want quick fix.

With the patch applied, dm-crypt uses one more mempool_alloc and 
mempool_free call per requests.

With 512-byte requests and ramdisk as a backing device, dm-crypt can 
process up to 25000 requests per second. On the same machine, you can do 
19000000 mempool_alloc+mempool_free calls per second. So, the slowdown 
caused by mempool_alloc+mempool_free is negligable - in theory, every 
request is slowed down by 1/760.

I and Mike measured it and we didn't see any slowdown caused by the patch.

> I couldn't resist but "may undermine" in patch header sounds like
> "we have no idea what it breaks but it no longer crashes".
> 
> Removal of headache by removing head is always 100% solution to
> the particular problem but it has usually also some side effects :-)
> 
> ...
> 
> > Mikulas does have an alternative approach that should be revisited ASAP
> > given that it also fixes this cpu hotplug issue:
> > http://people.redhat.com/mpatocka/patches/kernel/dm-crypt-paralelizace/current/series.html
> 
> Yes, and I spent many hours testing it and while it helped somewhere
> it was not perfect solution in every situation. And I wish it works!
> Unfortunately, the tests output was not convincing.
> But it was described in thread you referenced.

The parallelization patches help in sequential reading.

In other workloads - such as concurrent access by multiple threads or bulk 
data writing - encryption is already parallelized by the current dm-crypt 
implementation, so there's nothing to gain. The new implementation can't 
improve parallelization, if it is already parallelized with the current 
imeplemtation.

> And if he just repost the same patchset few month later without any
> more data and no changes, why should I change my opinion?
> I can just shut up of course and just maintain userspace and watch
> people complaining.
> 
> > context/timeline:
> > http://www.redhat.com/archives/dm-devel/2011-October/msg00127.html
> > http://www.redhat.com/archives/dm-devel/2012-March/msg00181.html
> > http://www.redhat.com/archives/dm-devel/2013-March/msg00103.html
> > 
> > Christoph pointed out there is precendence for sorting:
> > http://www.redhat.com/archives/dm-devel/2013-March/msg00104.html
> 
> My opinion is that fixing io scheduler work (sorting) in dmcrypt is
> just ugly hack and could lead to more problems in future because
> it can increase latency and it anticipates some IO access patterns.
> Dm-crypt should be (ideally) completely transparent and "invisible"
> to IO processing...

The problem is that the cfq scheduler doesn't merge adjacent requests 
submitted by different thread. You don't have to sort it in dm-crypt, but 
you must submit all the requests from a single thread.

Mikulas