From: David Riley <davidriley@chromium.org> To: Gerd Hoffmann <kraxel@redhat.com> Cc: dri-devel@lists.freedesktop.org, virtualization@lists.linux-foundation.org, "David Airlie" <airlied@linux.ie>, "Daniel Vetter" <daniel@ffwll.ch>, "Gurchetan Singh" <gurchetansingh@chromium.org>, "Stéphane Marchesin" <marcheu@chromium.org>, linux-kernel@vger.kernel.org Subject: Re: [PATCH] drm/virtio: Use vmalloc for command buffer allocations. Date: Fri, 30 Aug 2019 10:49:25 -0700 [thread overview] Message-ID: <CAASgrz0SXc2bEXq4xPCry_oHMXNbau36Q9i20anbFq1X0FsoMQ@mail.gmail.com> (raw) In-Reply-To: <20190830111605.twzssycagmjhfa45@sirius.home.kraxel.org> Hi Gerd, On Fri, Aug 30, 2019 at 4:16 AM Gerd Hoffmann <kraxel@redhat.com> wrote: > > Hi, > > > > > - kfree(vbuf->data_buf); > > > > + kvfree(vbuf->data_buf); > > > > > > if (is_vmalloc_addr(vbuf->data_buf)) ... > > > > > > needed here I gues? > > > > > > > kvfree() handles vmalloc/kmalloc/kvmalloc internally by doing that check. > > Ok. > > > - videobuf_vmalloc_to_sg in drivers/media/v4l2-core/videobuf-dma-sg.c, > > assumes contiguous array of scatterlist and that the buffer being converted > > is page aligned > > Well, vmalloc memory _is_ page aligned. True, but this function gets called for all potential enqueuings (eg resource_create_3d, resource_attach_backing) and I was concerned that some other usage in the future might not have that guarantee. But if that's overly being paranoid, I'm willing to backtrack on that. > sg_alloc_table_from_pages() does alot of what you need, you just need a > small loop around vmalloc_to_page() create a struct page array > beforehand. That feels like an extra allocation when under memory pressure and more work, to not gain much -- there still needs to be a function that iterates through all the pages. But I don't feel super strongly about it and can change it if you think that it will be less maintenance overhead. > Completely different approach: use get_user_pages() and don't copy the > execbuffer at all. This one I'd rather not do unless we can show that the copy is actually a problem. As it stands I expect this to be a fallback instead of the regular case, and if it's not then the VMs need to revisit how long the control queue size is. Right now most execbuffers end up being copied into a single contiguous buffer which results in 3 descriptors of the virtio queue. If vmalloc ends up being used (which is only a fallback because vmemdup_user tries kmalloc first still), then there'll be 66 descriptors for a 256KB buffer. I think that's fine for exceptional cases, but not ideal if it was commonplace. Chia-I suggested that we could have a flag for the ioctl execbuffer indicating that the buffer is BO to solve that, but again I'd prefer to see that it's actually a problem. I'll also be moving the sg table construction out of the critical section and properly accounting for the required number of descriptors before entering it in response to his comments. Thanks, Dave
WARNING: multiple messages have this Message-ID (diff)
From: David Riley <davidriley@chromium.org> To: Gerd Hoffmann <kraxel@redhat.com> Cc: "David Airlie" <airlied@linux.ie>, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, "Gurchetan Singh" <gurchetansingh@chromium.org>, "Stéphane Marchesin" <marcheu@chromium.org>, virtualization@lists.linux-foundation.org Subject: Re: [PATCH] drm/virtio: Use vmalloc for command buffer allocations. Date: Fri, 30 Aug 2019 10:49:25 -0700 [thread overview] Message-ID: <CAASgrz0SXc2bEXq4xPCry_oHMXNbau36Q9i20anbFq1X0FsoMQ@mail.gmail.com> (raw) In-Reply-To: <20190830111605.twzssycagmjhfa45@sirius.home.kraxel.org> Hi Gerd, On Fri, Aug 30, 2019 at 4:16 AM Gerd Hoffmann <kraxel@redhat.com> wrote: > > Hi, > > > > > - kfree(vbuf->data_buf); > > > > + kvfree(vbuf->data_buf); > > > > > > if (is_vmalloc_addr(vbuf->data_buf)) ... > > > > > > needed here I gues? > > > > > > > kvfree() handles vmalloc/kmalloc/kvmalloc internally by doing that check. > > Ok. > > > - videobuf_vmalloc_to_sg in drivers/media/v4l2-core/videobuf-dma-sg.c, > > assumes contiguous array of scatterlist and that the buffer being converted > > is page aligned > > Well, vmalloc memory _is_ page aligned. True, but this function gets called for all potential enqueuings (eg resource_create_3d, resource_attach_backing) and I was concerned that some other usage in the future might not have that guarantee. But if that's overly being paranoid, I'm willing to backtrack on that. > sg_alloc_table_from_pages() does alot of what you need, you just need a > small loop around vmalloc_to_page() create a struct page array > beforehand. That feels like an extra allocation when under memory pressure and more work, to not gain much -- there still needs to be a function that iterates through all the pages. But I don't feel super strongly about it and can change it if you think that it will be less maintenance overhead. > Completely different approach: use get_user_pages() and don't copy the > execbuffer at all. This one I'd rather not do unless we can show that the copy is actually a problem. As it stands I expect this to be a fallback instead of the regular case, and if it's not then the VMs need to revisit how long the control queue size is. Right now most execbuffers end up being copied into a single contiguous buffer which results in 3 descriptors of the virtio queue. If vmalloc ends up being used (which is only a fallback because vmemdup_user tries kmalloc first still), then there'll be 66 descriptors for a 256KB buffer. I think that's fine for exceptional cases, but not ideal if it was commonplace. Chia-I suggested that we could have a flag for the ioctl execbuffer indicating that the buffer is BO to solve that, but again I'd prefer to see that it's actually a problem. I'll also be moving the sg table construction out of the critical section and properly accounting for the required number of descriptors before entering it in response to his comments. Thanks, Dave _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2019-08-30 17:49 UTC|newest] Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-08-29 21:24 [PATCH] drm/virtio: Use vmalloc for command buffer allocations David Riley 2019-08-30 6:08 ` Gerd Hoffmann 2019-08-30 6:08 ` Gerd Hoffmann 2019-08-30 6:36 ` David Riley 2019-08-30 6:36 ` David Riley 2019-08-30 11:16 ` Gerd Hoffmann 2019-08-30 16:24 ` Chia-I Wu 2019-09-02 5:19 ` Gerd Hoffmann 2019-09-02 5:19 ` Gerd Hoffmann 2019-08-30 16:24 ` Chia-I Wu 2019-08-30 17:49 ` David Riley [this message] 2019-08-30 17:49 ` David Riley 2019-09-02 5:28 ` Gerd Hoffmann 2019-09-03 20:27 ` David Riley 2019-09-03 20:27 ` David Riley 2019-09-02 5:28 ` Gerd Hoffmann 2019-08-30 17:49 ` David Riley 2019-08-30 11:16 ` Gerd Hoffmann 2019-08-30 16:18 ` Chia-I Wu 2019-08-30 16:18 ` Chia-I Wu 2019-09-05 22:00 ` [PATCH v2] " David Riley 2019-09-05 22:00 ` David Riley 2019-09-06 5:18 ` Gerd Hoffmann 2019-09-09 17:12 ` David Riley 2019-09-09 17:12 ` David Riley 2019-09-09 17:12 ` David Riley 2019-09-06 5:18 ` Gerd Hoffmann 2019-09-10 20:06 ` [PATCH v3 1/2] drm/virtio: Rewrite virtio_gpu_queue_ctrl_buffer using fenced version David Riley 2019-09-10 20:06 ` David Riley 2019-09-11 5:12 ` Gerd Hoffmann 2019-09-11 5:12 ` Gerd Hoffmann 2019-09-11 17:35 ` David Riley 2019-09-11 17:35 ` David Riley 2019-09-11 5:12 ` Gerd Hoffmann 2019-09-10 20:06 ` [PATCH v3 2/2] drm/virtio: Use vmalloc for command buffer allocations David Riley 2019-09-10 20:06 ` David Riley 2019-09-11 18:14 ` [PATCH v4 0/2] drm/virtio: Use vmalloc for command buffer alllocations David Riley 2019-09-11 18:14 ` David Riley 2019-09-11 18:14 ` [PATCH v4 1/2] drm/virtio: Rewrite virtio_gpu_queue_ctrl_buffer using fenced version David Riley 2019-09-11 18:14 ` David Riley 2019-09-11 18:14 ` [PATCH v4 2/2] drm/virtio: Use vmalloc for command buffer allocations David Riley 2019-09-12 7:51 ` Gerd Hoffmann 2019-09-12 7:51 ` Gerd Hoffmann 2019-09-11 18:14 ` David Riley -- strict thread matches above, loose matches on Subject: below -- 2019-08-29 21:24 [PATCH] " David Riley
Reply instructions: You may reply publicly 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=CAASgrz0SXc2bEXq4xPCry_oHMXNbau36Q9i20anbFq1X0FsoMQ@mail.gmail.com \ --to=davidriley@chromium.org \ --cc=airlied@linux.ie \ --cc=daniel@ffwll.ch \ --cc=dri-devel@lists.freedesktop.org \ --cc=gurchetansingh@chromium.org \ --cc=kraxel@redhat.com \ --cc=linux-kernel@vger.kernel.org \ --cc=marcheu@chromium.org \ --cc=virtualization@lists.linux-foundation.org \ /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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.