[alsa-devel] [PATCH] ALSA: pcm: fix buffer_bytes max constrained by preallocated bytes issue

[Takashi Iwai]

On Thu, 16 Jan 2020 15:14:28 +0100,
Jie, Yang wrote:
> 
> > -----Original Message-----
> > From: Alsa-devel <alsa-devel-bounces at alsa-project.org> On Behalf Of
> > Takashi Iwai
> > Sent: Thursday, January 16, 2020 7:51 PM
> > To: Keyon Jie <yang.jie at linux.intel.com>
> > Cc: alsa-devel at alsa-project.org
> > Subject: Re: [alsa-devel] [PATCH] ALSA: pcm: fix buffer_bytes max
> > constrained by preallocated bytes issue
> > 
> > On Thu, 16 Jan 2020 12:25:38 +0100,
> > Keyon Jie wrote:
> > >
> > > On Thu, 2020-01-16 at 11:27 +0100, Takashi Iwai wrote:
> > > > On Thu, 16 Jan 2020 10:50:33 +0100,
> > > >
> > > > Oh, you're right, and I completely misread the patch.
> > > >
> > > > Now I took a coffee and can tell you the story behind the scene.
> > > >
> > > > I believe the current code is intentionally limiting the size to the
> > > > preallocated size.  This limitation was brought for not trying to
> > > > allocate a larger buffer when the buffer has been preallocated.  In
> > > > the past, most hardware allocated the continuous pages for a buffer
> > > > and the allocation of a large buffer fails quite likely.  This was
> > > > the reason of the buffer preallocation.  So, the driver wanted to
> > > > tell the user-space the limit.  If user needs to have an extra large
> > > > buffer, they are supposed to fiddle with prealloc procfs (either
> > > > setting zero to clear the preallocation or setting a large enough
> > > > buffer beforehand).
> > >
> > > Thank you for the sharing, it is interesting and knowledge learned to
> > > me.
> > >
> > > >
> > > > For SG-buffers, though, limitation makes less sense than continuous
> > > > pages.  e.g. a patch below removes the limitation for SG-buffers.
> > > > But changing this would definitely cause the behavior difference,
> > > > and I don't know whether it's a reasonable move -- I'm afraid that
> > > > apps would start hogging too much memory if the limitation is gone.
> > >
> > > I just went through all invoking to snd_pcm_lib_preallocate_pages*(),
> > > for those SNDRV_DMA_TYPE_DEV, some of them set the *size* equal to
> > the
> > > *max*, some set the *max* several times to the *size*, IMHO, the
> > > *max*s are matched to those hardware's limiatation, comparing to the
> > > *size*s, aren't they?
> > >
> > > In this case, I still think my patch hanle all
> > > TYPE_DEV/SNDRV_DMA_TYPE_DEV/TYPE_SG/SNDRV_DMA_TYPE_DEV
> > cases more
> > > gracefully, we will still take the limitation from the specific driver
> > > set, from the *max* param, and the test results looks very nice here,
> > > we will take what the user space wanted for buffer-bytes via aply
> > > exactly, as long as it is suitable for the interval and constraints.
> > 
> > Well, I have a mixed feeling.  Certainly we'd need some better way to allow a
> > larger buffer allocation, especially for HDA.  OTOH, if the buffer was
> > preallocated, it's meant to be used actually.  That's the point of the
> > hw_constraint setup.
> 
> So if the buffer was preallocated, it won't be re-allocated at hw_params() stage,
> is this conflict with the re-allocate logic in hw_params()?

If a larger buffer than the preallocated one is requested, the PCM
core tries to allocate another buffer while the preallocated one
remains kept untouched.  It's because such a larger allocation is
supposed to be one-off thing.  Normal usages should fit with the
preallocated buffer size.

That said, if a larger buffer is required too frequently, it makes no
sense to keep the preallocation.  Or, preallocate larger buffers
instead.

> > And now thinking again after another cup of coffee, I wonder why we do
> > preallocate for HDA at all.  For HD-audio, the allocation of any large buffer
> > would succeed very likely because of SG-buffer.
> > 
> > So, just setting 0 to the preallocation size (but keeping else) would work, e.g.
> > something like below?  The help text needs adjustment, but you can see the
> > rough idea.
> 
> So, do you suggest not doing preallocation(or calling it with 0 size) for all driver
> with TYPE_SG? I am fine if this is the recommended method, I can try this on SOF
> I2S platform to see if it can work as we required for very large buffer size.

This really depends on the use case, and I'm not yet sure whether no
preallocation is really recommended or not.  Without preallocation,
each PCM open is involved with a large amount of page allocations, and
it makes easier for users to hog resources more easily.  It'll use
vmalloc addresses that aren't unlimited, and may trigger OOM easily.


thanks,

Takashi