Re: [PATCH v6 5/9] ALSA: virtio: handling control and I/O messages for the PCM device

From: Takashi Iwai <tiwai@suse.de>
To: Anton Yakovlev <anton.yakovlev@opensynergy.com>
Cc: <virtualization@lists.linux-foundation.org>,
	<alsa-devel@alsa-project.org>, <virtio-dev@lists.oasis-open.org>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	Jaroslav Kysela <perex@perex.cz>, Takashi Iwai <tiwai@suse.com>,
	<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v6 5/9] ALSA: virtio: handling control and I/O messages for the PCM device
Date: Mon, 01 Mar 2021 14:32:04 +0100	[thread overview]
Message-ID: <s5h35xfj8yz.wl-tiwai@suse.de> (raw)
In-Reply-To: <b3de8563-1776-4296-2cf5-883c831dfbe8@opensynergy.com>

On Mon, 01 Mar 2021 10:25:05 +0100,
Anton Yakovlev wrote:
> 
> On 28.02.2021 12:27, Takashi Iwai wrote:
> > On Sat, 27 Feb 2021 09:59:52 +0100,
> > Anton Yakovlev wrote:
> >> +/**
> >> + * virtsnd_pcm_event() - Handle the PCM device event notification.
> >> + * @snd: VirtIO sound device.
> >> + * @event: VirtIO sound event.
> >> + *
> >> + * Context: Interrupt context.
> >
> > OK, then nonatomic PCM flag is invalid...
> 
> Well, no. Here, events are kind of independent entities. PCM-related
> events are just a special case of more generic events, which can carry
> any kind of notification/payload. (And at the moment, only XRUN
> notification is supported for PCM substreams.) So it has nothing to do
> with the atomicity of the PCM device itself.

OK, thanks.

Basically the only question is how snd_pcm_period_elapsed() is called.
And I see that it's called inside queue->lock, and this already
invalidates the nonatomic PCM mode.  So the code needs the fix: either
fix this locking (and the context is guaranteed not to be an irq
context), or change to the normal PCM mode without nonatomic flag.
Both would bring some side-effect, and we need further changes, I
suppose...

> >> +/**
> >> + * virtsnd_pcm_sg_num() - Count the number of sg-elements required to represent
> >> + *                        vmalloc'ed buffer.
> >> + * @data: Pointer to vmalloc'ed buffer.
> >> + * @length: Buffer size.
> >> + *
> >> + * Context: Any context.
> >> + * Return: Number of physically contiguous parts in the @data.
> >> + */
> >> +static int virtsnd_pcm_sg_num(u8 *data, unsigned int length)
> >> +{
> >> +     phys_addr_t sg_address;
> >> +     unsigned int sg_length;
> >> +     int num = 0;
> >> +
> >> +     while (length) {
> >> +             struct page *pg = vmalloc_to_page(data);
> >> +             phys_addr_t pg_address = page_to_phys(pg);
> >> +             size_t pg_length;
> >> +
> >> +             pg_length = PAGE_SIZE - offset_in_page(data);
> >> +             if (pg_length > length)
> >> +                     pg_length = length;
> >> +
> >> +             if (!num || sg_address + sg_length != pg_address) {
> >> +                     sg_address = pg_address;
> >> +                     sg_length = pg_length;
> >> +                     num++;
> >> +             } else {
> >> +                     sg_length += pg_length;
> >> +             }
> >> +
> >> +             data += pg_length;
> >> +             length -= pg_length;
> >> +     }
> >> +
> >> +     return num;
> >> +}
> >> +
> >> +/**
> >> + * virtsnd_pcm_sg_from() - Build sg-list from vmalloc'ed buffer.
> >> + * @sgs: Preallocated sg-list to populate.
> >> + * @nsgs: The maximum number of elements in the @sgs.
> >> + * @data: Pointer to vmalloc'ed buffer.
> >> + * @length: Buffer size.
> >> + *
> >> + * Splits the buffer into physically contiguous parts and makes an sg-list of
> >> + * such parts.
> >> + *
> >> + * Context: Any context.
> >> + */
> >> +static void virtsnd_pcm_sg_from(struct scatterlist *sgs, int nsgs, u8 *data,
> >> +                             unsigned int length)
> >> +{
> >> +     int idx = -1;
> >> +
> >> +     while (length) {
> >> +             struct page *pg = vmalloc_to_page(data);
> >> +             size_t pg_length;
> >> +
> >> +             pg_length = PAGE_SIZE - offset_in_page(data);
> >> +             if (pg_length > length)
> >> +                     pg_length = length;
> >> +
> >> +             if (idx == -1 ||
> >> +                 sg_phys(&sgs[idx]) + sgs[idx].length != page_to_phys(pg)) {
> >> +                     if (idx + 1 == nsgs)
> >> +                             break;
> >> +                     sg_set_page(&sgs[++idx], pg, pg_length,
> >> +                                 offset_in_page(data));
> >> +             } else {
> >> +                     sgs[idx].length += pg_length;
> >> +             }
> >> +
> >> +             data += pg_length;
> >> +             length -= pg_length;
> >> +     }
> >> +
> >> +     sg_mark_end(&sgs[idx]);
> >> +}
> >
> > Hmm, I thought there can be already a handy helper to convert vmalloc
> > to sglist, but apparently not.  It should have been trivial to get the
> > page list from vmalloc, e.g.
> >
> > int vmalloc_to_page_list(void *p, struct page **page_ret)
> > {
> >          struct vmap_area *va;
> >
> >          va = find_vmap_area((unsigned long)p);
> >          if (!va)
> >                  return 0;
> >          *page_ret = va->vm->pages;
> >          return va->vm->nr_pages;
> > }
> >
> > Then you can set up the sg list in a single call from the given page
> > list.
> >
> > But it's just a cleanup, and let's mark it as a room for
> > improvements.
> 
> Yeah, we can take a look into some kind of optimizations here. But I
> suspect, the overall code will look similar. It is not enough just to
> get a list of pages, you also need to build a list of physically
> contiguous regions from it.

I believe the standard helper does it.  But it's for sg_table, hence
the plain scatterlist needs to be extracted from there, but most of
complex things are in the standard code.

But it's merely an optimization and something for future.

Takashi