Alsa-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Takashi Iwai <tiwai@suse.de>
To: "Jie, Yang" <yang.jie@intel.com>
Cc: "alsa-devel@alsa-project.org" <alsa-devel@alsa-project.org>,
	Keyon Jie <yang.jie@linux.intel.com>
Subject: Re: [alsa-devel] [PATCH] ALSA: pcm: fix buffer_bytes	max	constrained by preallocated bytes issue
Date: Thu, 16 Jan 2020 16:45:16 +0100
Message-ID: <s5hpnfj5rsj.wl-tiwai@suse.de> (raw)
In-Reply-To: <E7B1D079BA13FB44A978CC8F69C7D6A96F98EDB4@SHSMSX105.ccr.corp.intel.com>

On Thu, 16 Jan 2020 15:14:28 +0100,
Jie, Yang wrote:
> 
> > -----Original Message-----
> > From: Alsa-devel <alsa-devel-bounces@alsa-project.org> On Behalf Of
> > Takashi Iwai
> > Sent: Thursday, January 16, 2020 7:51 PM
> > To: Keyon Jie <yang.jie@linux.intel.com>
> > Cc: alsa-devel@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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

      parent reply index

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-16  4:53 Keyon Jie
2020-01-16  7:15 ` Takashi Iwai
2020-01-16  9:50   ` Keyon Jie
2020-01-16 10:27     ` Takashi Iwai
2020-01-16 11:25       ` Keyon Jie
2020-01-16 11:50         ` Takashi Iwai
2020-01-16 14:14           ` Jie, Yang
2020-01-16 15:31             ` Jie, Yang
2020-01-16 16:07               ` Takashi Iwai
2020-01-16 16:39               ` Pierre-Louis Bossart
2020-01-16 17:25                 ` Rajwa, Marcin
2020-01-16 17:40                   ` Pierre-Louis Bossart
2020-01-16 20:37                     ` Takashi Iwai
2020-01-17  5:30                       ` Keyon Jie
2020-01-17  7:57                         ` Takashi Iwai
2020-01-17 10:13                           ` Keyon Jie
2020-01-17 10:30                             ` Takashi Iwai
2020-01-17 10:56                               ` Keyon Jie
2020-01-17 11:15                                 ` Takashi Iwai
2020-01-17  5:37                     ` Keyon Jie
2020-01-17  8:00                       ` Takashi Iwai
2020-01-17 10:43                         ` Keyon Jie
2020-01-17 11:12                           ` Takashi Iwai
2020-01-19  3:52                             ` Keyon Jie
2020-01-19  7:09                               ` Takashi Iwai
2020-01-19  8:11                                 ` Keyon Jie
2020-01-19  9:04                                   ` Takashi Iwai
2020-01-19 10:14                                     ` Keyon Jie
2020-01-19 10:43                                       ` Takashi Iwai
2020-01-20  2:23                                         ` Keyon Jie
2020-01-16 15:45             ` Takashi Iwai [this message]

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=s5hpnfj5rsj.wl-tiwai@suse.de \
    --to=tiwai@suse.de \
    --cc=alsa-devel@alsa-project.org \
    --cc=yang.jie@intel.com \
    --cc=yang.jie@linux.intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Alsa-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/alsa-devel/0 alsa-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 alsa-devel alsa-devel/ https://lore.kernel.org/alsa-devel \
		alsa-devel@alsa-project.org
	public-inbox-index alsa-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.alsa-project.alsa-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git