[PATCH] snd-hda-intel: fix insufficient memory

[PATCH] snd-hda-intel: fix insufficient memory

[wbrana]

Module allocates insufficient memory for multichannel and high quality
audio (96 kHz, 24 bit)
Patch for 2.6.19-* changes default/maximal size from 64/128 to 256/4096 kB.

--- sound/pci/hda/hda_intel.c.orig      2006-09-29 13:40:36.000000000 +0200
+++ sound/pci/hda/hda_intel.c   2006-11-05 16:45:13.000000000 +0100
@@ -1270,5 +1270,5 @@
        snd_pcm_lib_preallocate_pages_for_all(pcm, SNDRV_DMA_TYPE_DEV,
                                              snd_dma_pci_data(chip->pci),
-                                             1024 * 64, 1024 * 128);
+                                             1024 * 256, 1024 * 4096);
        chip->pcm[pcm_dev] = pcm;
        if (chip->pcm_devs < pcm_dev + 1)

Re: [PATCH] snd-hda-intel: fix insufficient memory

[Andrew Morton]

On Tue, 21 Nov 2006 21:33:29 +0100
wbrana@gmail.com wrote:

> Module allocates insufficient memory for multichannel and high quality
> audio (96 kHz, 24 bit)
> Patch for 2.6.19-* changes default/maximal size from 64/128 to 256/4096 kB.
> 
> --- sound/pci/hda/hda_intel.c.orig      2006-09-29 13:40:36.000000000 +0200
> +++ sound/pci/hda/hda_intel.c   2006-11-05 16:45:13.000000000 +0100
> @@ -1270,5 +1270,5 @@
>         snd_pcm_lib_preallocate_pages_for_all(pcm, SNDRV_DMA_TYPE_DEV,
>                                               snd_dma_pci_data(chip->pci),
> -                                             1024 * 64, 1024 * 128);
> +                                             1024 * 256, 1024 * 4096);
>         chip->pcm[pcm_dev] = pcm;
>         if (chip->pcm_devs < pcm_dev + 1)

This was recently increased, but not by such a large amount:

	snd_pcm_lib_preallocate_pages_for_all(pcm, SNDRV_DMA_TYPE_DEV,
					      snd_dma_pci_data(chip->pci),
					      1024 * 64, 1024 * 1024);

is that sufficient?

[RCU] adds a prefetch() in rcu_do_batch()

[Eric Dumazet]

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

On some workloads, (for example when lot of close() syscalls are done), RCU 
qlen can be quite large, and RCU heads are no longer in cpu cache when 
rcu_do_batch() is called.

This patches adds a prefetch() in rcu_do_batch() to give CPU a hint to bring 
back cache lines containing 'struct rcu_head's.

Most list manipulations macros include prefetch(), but not open coded ones (at 
least with current C compilers :) )

I got a nice speedup on a trivial benchmark  (3.48 us per iteration instead of 
3.95 us on a 1.6 GHz Pentium-M)
while (1) { pipe(p); close(fd[0]); close(fd[1]);}

Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>

[-- Attachment #2: rcu.prefetch.patch --]
[-- Type: text/plain, Size: 492 bytes --]

--- linux-2.6.19-rc6/kernel/rcupdate.c	2006-11-16 05:03:40.000000000 +0100
+++ linux-2.6.19-rc6-ed/kernel/rcupdate.c	2006-11-22 15:12:09.000000000 +0100
@@ -235,12 +235,14 @@ static void rcu_do_batch(struct rcu_data
 
 	list = rdp->donelist;
 	while (list) {
-		next = rdp->donelist = list->next;
+		next = list->next;
+		prefetch(next);
 		list->func(list);
 		list = next;
 		if (++count >= rdp->blimit)
 			break;
 	}
+	rdp->donelist = list;
 
 	local_irq_disable();
 	rdp->qlen -= count;

[PATCH] dont insert pipe dentries into dentry_hashtable.

[Eric Dumazet]

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

We currently insert pipe dentries into the global dentry hashtable.
This is *suboptimal* because there is currently no way these entries can be 
used for a lookup(). (/proc/xxx/fd/xxx uses a different mechanism). Inserting 
them in dentry hashtable slow dcache lookups.


To let __dpath() still work correctly (ie not adding a " (deleted)") after 
dentry name, we do : 

- Right after d_alloc(), pretend they are hashed by clearing the 
DCACHE_UNHASHED bit. 

- Call d_instantiate() instead of d_add() : dentry is not inserted in hash 
table.

__dpath() & friends work as intended during dentry lifetime.

- At dismantle time, once dput() must clear the dentry, setting again 
DCACHE_UNHASHED bit inside the custom d_delete() function provided by pipe 
code, so that dput() can just kill_it.

This patch, combined with the next one (avoid RCU for never hashed dentries) 
reduced time of { pipe(p); close(p[0]); close(p[1]);} on my UP machine 
(1.6GHz Pentium-M) from 3.23 us to 2.86 us 
(But this patch does not depend on other patches, only bench results)

Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>

[-- Attachment #2: pipe_nohash_dentry.patch --]
[-- Type: text/plain, Size: 1494 bytes --]

--- linux-2.6.19-rc6/fs/pipe.c	2006-11-22 17:33:52.000000000 +0100
+++ linux-2.6.19-rc6-ed/fs/pipe.c	2006-11-22 17:53:02.000000000 +0100
@@ -830,7 +830,14 @@ void free_pipe_info(struct inode *inode)
 static struct vfsmount *pipe_mnt __read_mostly;
 static int pipefs_delete_dentry(struct dentry *dentry)
 {
-	return 1;
+	/*
+	 * At creation time, we pretended this dentry was hashed
+	 * (by clearing DCACHE_UNHASHED bit in d_flags)
+	 * At delete time, we restore the truth : not hashed.
+	 * (so that dput() can proceed correctly)
+	 */
+	dentry->d_flags |= DCACHE_UNHASHED;
+	return 0;
 }
 
 static struct dentry_operations pipefs_dentry_operations = {
@@ -891,17 +898,22 @@ struct file *create_write_pipe(void)
 	if (!inode)
 		goto err_file;
 
-	sprintf(name, "[%lu]", inode->i_ino);
+	this.len = sprintf(name, "[%lu]", inode->i_ino);
 	this.name = name;
-	this.len = strlen(name);
-	this.hash = inode->i_ino; /* will go */
+	this.hash = 0;
 	err = -ENOMEM;
 	dentry = d_alloc(pipe_mnt->mnt_sb->s_root, &this);
 	if (!dentry)
 		goto err_inode;
 
 	dentry->d_op = &pipefs_dentry_operations;
-	d_add(dentry, inode);
+	/*
+	 * We dont want to publish this dentry into global dentry hash table. 
+	 * We pretend dentry is already hashed, by unsetting DCACHE_UNHASHED
+	 * This permits a working /proc/$pid/fd/XXX on pipes
+	 */
+	dentry->d_flags &= ~DCACHE_UNHASHED;
+	d_instantiate(dentry, inode);
 	f->f_vfsmnt = mntget(pipe_mnt);
 	f->f_dentry = dentry;
 	f->f_mapping = inode->i_mapping;

Re: [PATCH] dont insert pipe dentries into dentry_hashtable.

[Andrew Morton]

On Wed, 22 Nov 2006 18:48:41 +0100
Eric Dumazet <dada1@cosmosbay.com> wrote:

> We currently insert pipe dentries into the global dentry hashtable.
> This is *suboptimal* because there is currently no way these entries can be 
> used for a lookup(). (/proc/xxx/fd/xxx uses a different mechanism). Inserting 
> them in dentry hashtable slow dcache lookups.
> 
> 
> To let __dpath() still work correctly (ie not adding a " (deleted)") after 
> dentry name, we do : 
> 
> - Right after d_alloc(), pretend they are hashed by clearing the 
> DCACHE_UNHASHED bit. 
> 
> - Call d_instantiate() instead of d_add() : dentry is not inserted in hash 
> table.
> 
> __dpath() & friends work as intended during dentry lifetime.
> 
> - At dismantle time, once dput() must clear the dentry, setting again 
> DCACHE_UNHASHED bit inside the custom d_delete() function provided by pipe 
> code, so that dput() can just kill_it.
> 
> This patch, combined with the next one (avoid RCU for never hashed dentries) 
> reduced time of { pipe(p); close(p[0]); close(p[1]);} on my UP machine 
> (1.6GHz Pentium-M) from 3.23 us to 2.86 us 
> (But this patch does not depend on other patches, only bench results)

The DCACHE_UNHASHED games seem hacky.

Would it be cleaner to define a new dentry.d_flags bit which can be used to
indicate that this is a hashing-not-needed dentry, and to handle that over
in dcache.c?

Re: [PATCH] dont insert pipe dentries into dentry_hashtable.

[Al Viro]

On Wed, Nov 22, 2006 at 01:36:18PM -0800, Andrew Morton wrote:
> The DCACHE_UNHASHED games seem hacky.
> 
> Would it be cleaner to define a new dentry.d_flags bit which can be used to
> indicate that this is a hashing-not-needed dentry, and to handle that over
> in dcache.c?

Much more invasive change, actually, and on fairly hot paths.

Re: [PATCH] dont insert pipe dentries into dentry_hashtable.

[David Miller]

From: Andrew Morton <akpm@osdl.org>
Date: Wed, 22 Nov 2006 13:36:18 -0800

> The DCACHE_UNHASHED games seem hacky.

Note this DCACHE_UNHASHES scheme was Al Viro's suggested
implementation :-)

Re: [RCU] adds a prefetch() in rcu_do_batch()

[Paul E. McKenney]

On Wed, Nov 22, 2006 at 04:02:29PM +0100, Eric Dumazet wrote:
> On some workloads, (for example when lot of close() syscalls are done), RCU
> qlen can be quite large, and RCU heads are no longer in cpu cache when
> rcu_do_batch() is called.
> 
> This patches adds a prefetch() in rcu_do_batch() to give CPU a hint to bring
> back cache lines containing 'struct rcu_head's.
> 
> Most list manipulations macros include prefetch(), but not open coded ones (at
> least with current C compilers :) )
> 
> I got a nice speedup on a trivial benchmark  (3.48 us per iteration instead of
> 3.95 us on a 1.6 GHz Pentium-M)
> while (1) { pipe(p); close(fd[0]); close(fd[1]);}

Interesting!  How much of the speedup was due to the prefetch() and how
much to removing the extra store to rdp->donelist?

							Thanx, Paul

> Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>

> --- linux-2.6.19-rc6/kernel/rcupdate.c	2006-11-16 05:03:40.000000000 +0100
> +++ linux-2.6.19-rc6-ed/kernel/rcupdate.c	2006-11-22 15:12:09.000000000 +0100
> @@ -235,12 +235,14 @@ static void rcu_do_batch(struct rcu_data
> 
>  	list = rdp->donelist;
>  	while (list) {
> -		next = rdp->donelist = list->next;
> +		next = list->next;
> +		prefetch(next);
>  		list->func(list);
>  		list = next;
>  		if (++count >= rdp->blimit)
>  			break;
>  	}
> +	rdp->donelist = list;
> 
>  	local_irq_disable();
>  	rdp->qlen -= count;

Re: [RCU] adds a prefetch() in rcu_do_batch()

[Eric Dumazet]

On Thursday 30 November 2006 02:25, Paul E. McKenney wrote:
> On Wed, Nov 22, 2006 at 04:02:29PM +0100, Eric Dumazet wrote:
> > On some workloads, (for example when lot of close() syscalls are done),
> > RCU qlen can be quite large, and RCU heads are no longer in cpu cache
> > when rcu_do_batch() is called.
> >
> > This patches adds a prefetch() in rcu_do_batch() to give CPU a hint to
> > bring back cache lines containing 'struct rcu_head's.
> >
> > Most list manipulations macros include prefetch(), but not open coded
> > ones (at least with current C compilers :) )
> >
> > I got a nice speedup on a trivial benchmark  (3.48 us per iteration
> > instead of 3.95 us on a 1.6 GHz Pentium-M)
> > while (1) { pipe(p); close(fd[0]); close(fd[1]);}
>
> Interesting!  How much of the speedup was due to the prefetch() and how
> much to removing the extra store to rdp->donelist?

I only benchmarked the prefetch() case.
 
Then, when cooking the patch I found I could do the rdp->donelist affectation 
after the loop. I am not sure it's worth to do another benchmark for this 
trivial optimization (Please dont tell me its not a valid one :) )

Eric

Re: [RCU] adds a prefetch() in rcu_do_batch()

[Paul E. McKenney]

On Thu, Nov 30, 2006 at 09:55:52AM +0100, Eric Dumazet wrote:
> On Thursday 30 November 2006 02:25, Paul E. McKenney wrote:
> > On Wed, Nov 22, 2006 at 04:02:29PM +0100, Eric Dumazet wrote:
> > > On some workloads, (for example when lot of close() syscalls are done),
> > > RCU qlen can be quite large, and RCU heads are no longer in cpu cache
> > > when rcu_do_batch() is called.
> > >
> > > This patches adds a prefetch() in rcu_do_batch() to give CPU a hint to
> > > bring back cache lines containing 'struct rcu_head's.
> > >
> > > Most list manipulations macros include prefetch(), but not open coded
> > > ones (at least with current C compilers :) )
> > >
> > > I got a nice speedup on a trivial benchmark  (3.48 us per iteration
> > > instead of 3.95 us on a 1.6 GHz Pentium-M)
> > > while (1) { pipe(p); close(fd[0]); close(fd[1]);}
> >
> > Interesting!  How much of the speedup was due to the prefetch() and how
> > much to removing the extra store to rdp->donelist?
> 
> I only benchmarked the prefetch() case.
> 
> Then, when cooking the patch I found I could do the rdp->donelist affectation
> after the loop. I am not sure it's worth to do another benchmark for this
> trivial optimization (Please dont tell me its not a valid one :) )

It would be a good idea to check it out.  Modern CPUs can be a bit
on the tricky side.  I have seen cases where removing instructions
slowed things down.  And it can't be -that- hard to run the other
two cases!

							Thanx, Paul

Re: [PATCH] snd-hda-intel: fix insufficient memory

[wbrana]

Module uses shorter buffer if memory isn't sufficient.
It doesn't change size of allocated memory automatically.
User has to change it with command like this:
echo 256 > /proc/asound/card0/pcm0p/sub0/prealloc
I can change it, but many regular users don't know it.
This card is used in new PCs, which have at least 256 MB RAM.
256 kB shouldn't be too much. Whole block isn't used if it isn't needed.

On 11/22/06, Andrew Morton <akpm@osdl.org> wrote:
> On Tue, 21 Nov 2006 21:33:29 +0100
> wbrana@gmail.com wrote:
>
> > Module allocates insufficient memory for multichannel and high quality
> > audio (96 kHz, 24 bit)
> > Patch for 2.6.19-* changes default/maximal size from 64/128 to 256/4096 kB.
> >
> > --- sound/pci/hda/hda_intel.c.orig      2006-09-29 13:40:36.000000000 +0200
> > +++ sound/pci/hda/hda_intel.c   2006-11-05 16:45:13.000000000 +0100
> > @@ -1270,5 +1270,5 @@
> >         snd_pcm_lib_preallocate_pages_for_all(pcm, SNDRV_DMA_TYPE_DEV,
> >                                               snd_dma_pci_data(chip->pci),
> > -                                             1024 * 64, 1024 * 128);
> > +                                             1024 * 256, 1024 * 4096);
> >         chip->pcm[pcm_dev] = pcm;
> >         if (chip->pcm_devs < pcm_dev + 1)
>
> This was recently increased, but not by such a large amount:
>
>         snd_pcm_lib_preallocate_pages_for_all(pcm, SNDRV_DMA_TYPE_DEV,
>                                               snd_dma_pci_data(chip->pci),
>                                               1024 * 64, 1024 * 1024);
>
> is that sufficient?
>

Re: [PATCH] snd-hda-intel: fix insufficient memory

[Andrew Morton]

On Wed, 22 Nov 2006 18:19:49 +0100
wbrana@gmail.com wrote:

> Module uses shorter buffer if memory isn't sufficient.
> It doesn't change size of allocated memory automatically.
> User has to change it with command like this:
> echo 256 > /proc/asound/card0/pcm0p/sub0/prealloc
> I can change it, but many regular users don't know it.
> This card is used in new PCs, which have at least 256 MB RAM.
> 256 kB shouldn't be too much. Whole block isn't used if it isn't needed.

That didn't answer my question.

> On 11/22/06, Andrew Morton <akpm@osdl.org> wrote:
> > On Tue, 21 Nov 2006 21:33:29 +0100
> > wbrana@gmail.com wrote:
> >
> > > Module allocates insufficient memory for multichannel and high quality
> > > audio (96 kHz, 24 bit)
> > > Patch for 2.6.19-* changes default/maximal size from 64/128 to 256/4096 kB.
> > >
> > > --- sound/pci/hda/hda_intel.c.orig      2006-09-29 13:40:36.000000000 +0200
> > > +++ sound/pci/hda/hda_intel.c   2006-11-05 16:45:13.000000000 +0100
> > > @@ -1270,5 +1270,5 @@
> > >         snd_pcm_lib_preallocate_pages_for_all(pcm, SNDRV_DMA_TYPE_DEV,
> > >                                               snd_dma_pci_data(chip->pci),
> > > -                                             1024 * 64, 1024 * 128);
> > > +                                             1024 * 256, 1024 * 4096);
> > >         chip->pcm[pcm_dev] = pcm;
> > >         if (chip->pcm_devs < pcm_dev + 1)
> >
> > This was recently increased, but not by such a large amount:
> >
> >         snd_pcm_lib_preallocate_pages_for_all(pcm, SNDRV_DMA_TYPE_DEV,
> >                                               snd_dma_pci_data(chip->pci),
> >                                               1024 * 64, 1024 * 1024);
> >
> > is that sufficient?
> >

Are the new settings of 64kb and 1MB sufficient?  If not, by how much must
they be increased, and why?

Re: [PATCH] snd-hda-intel: fix insufficient memory

[wbrana]

On 11/22/06, Andrew Morton <akpm@osdl.org> wrote:
>
> Are the new settings of 64kb and 1MB sufficient?  If not, by how much must
> they be increased, and why?
>
>
Default size should be increased to 256 kB to allow same buffer length
 ( 16384 frames )
with 2 and 8 channels or same buffer time with 48 kHz/16 bit and 96 kHz/32 bit.
Maximal size should be at least 4096 kB, which should't limit buffer
time with any sound
like 192 kHz/8 channels/32 bit.

Re: [PATCH] snd-hda-intel: fix insufficient memory

[Andrew Morton]

On Thu, 23 Nov 2006 21:17:21 +0100
wbrana@gmail.com wrote:

> On 11/22/06, Andrew Morton <akpm@osdl.org> wrote:
> >
> > Are the new settings of 64kb and 1MB sufficient?  If not, by how much must
> > they be increased, and why?
> >
> >
> Default size should be increased to 256 kB to allow same buffer length
>  ( 16384 frames )
> with 2 and 8 channels or same buffer time with 48 kHz/16 bit and 96 kHz/32 bit.
> Maximal size should be at least 4096 kB, which should't limit buffer
> time with any sound
> like 192 kHz/8 channels/32 bit.

Please send a new patch, with a *full* changelog which completely describes
the need for the change.