Re: [PATCH] md: dm-crypt: Add option to re-use a new global work-queue.

[Heinz Mauelshagen <heinzm@redhat.com>]

On Thu, 2010-04-22 at 12:42 -0700, San Mehat wrote:
> On Thu, Apr 22, 2010 at 11:47 AM, Milan Broz <mbroz@redhat.com> wrote:
> > On 04/22/2010 08:08 PM, San Mehat wrote:
> >> On Thu, Apr 22, 2010 at 11:03 AM, Milan Broz <mbroz@redhat.com> wrote:
> >>> On 04/22/2010 07:48 PM, San Mehat wrote:
> >>>> Typically, dm-crypt instances each have their own set of kcrypt/kcrypt_io
> >>>> work-queues. This patch adds an option which will create one set of
> >>>> work-queues on init, and re-uses them for all dm-crypt target instances.
> >
> >>> Can you explain the real reason for this patch?
> >>>
> >>
> >> Sure, I'd be happy to explain.
> >
> > (Please add this always to patch header.)
> >
> 
> Will do - thanks.
> 
> >>
> >>   Upcoming versions of android are about to start using dm/dm-crypt
> >> heavily, having
> >> a large number of small dm-crypt instances running on the device (hard
> >> to tell just
> >> how many, but i've seen cases where up to 50 or 60 instances may be
> >> running). This ends up creating 100 - 120 kernel threads, and I was
> >> simply trying to cut that down.
> >
> > Sorry, but I don't take this argument. "Too many notes!" :-)
> >
> > So the problem is with memory allocation? Scheduler? Or where?
> > Kernel threads should be cheap.

Please don't forget to think about the other end of the computer systems
scale: large multiprocessors/multicore systems. There you'd have way too
many threads with just some dm-crypt devices going with the multi-thread
workqueue design.

There ain't now "one for all" solution.

Heinz

> >
> 
> Well the initial consideration was towards memory overhead with so
> many threads that don't do much (in our use-case) on an embedded
> device.
> 
> > If you need 60 crypt devices, you almost surely hit at least starvation
> > problem with one global queue!
> > (Just curious - what are these crypt devices doing?)
> 
> The crypt devices are providing small read-only encrypted file-systems
> - whose backing files exist on an external FAT file-system, and are
> created on-demand as needed. In this usage scenario, we'll only see
> typically a few of these devices being simultaneously accessed, (and
> the sd-card throughput is definitely the long-pole in the performance
> profile, so even when I beat on 80 or 90 concurrent instances, we're
> mainly waiting for mmcqd to complete transactions).
> 
> >
> >> I'd be more than happy to discuss alternatives; but do we *really*
> >> need 2 work-queue threads per instance?
> >
> > yes.
> 
> What if we made a note in the Kconfig advising against using the option in
> stacked configurations? (Or even make it depend on CONFIG_EMBEDDED)
> 
> Thanks for your time,
> 
> -san
> 
> >
> > For separate io queue - see commit cabf08e4d3d1181d7c408edae97fb4d1c31518af
> >
> > | Add post-processing queue (per crypt device) for read operations.
> >
> > | Current implementation uses only one queue for all operations
> > | and this can lead to starvation caused by many requests waiting
> > | for memory allocation. But the needed memory-releasing operation
> > | is queued after these requests (in the same queue).
> >
> >
> > (and there were another problem with async crypt - callback is called
> > in interrupt context, bio must be submitted from separate workqueue IIRC)
> >
> >>> (cc: Alasdair - I think he will not accept the patch anyway.)
> >>
> >> Probably not, but at least we can get the discussion going :)
> >
> > I am not saying that I do not want to discuss this - but we must know
> > the real problems many queues are causing first.
> > And then think about possible solutions.
> >
> > Milan
> >
> 
> 
>