Re: [Xen-devel] [RFC 1/3] xen/balloon: Allow allocating DMA buffers

From: Oleksandr Andrushchenko
Date: Tue May 22 2018 - 13:33:50 EST

On 05/22/2018 09:02 PM, Boris Ostrovsky wrote:
On 05/22/2018 11:00 AM, Oleksandr Andrushchenko wrote:
On 05/22/2018 05:33 PM, Boris Ostrovsky wrote:
On 05/22/2018 01:55 AM, Oleksandr Andrushchenko wrote:
On 05/21/2018 11:36 PM, Boris Ostrovsky wrote:
On 05/21/2018 03:13 PM, Oleksandr Andrushchenko wrote:
On 05/21/2018 09:53 PM, Boris Ostrovsky wrote:
On 05/21/2018 01:32 PM, Oleksandr Andrushchenko wrote:
On 05/21/2018 07:35 PM, Boris Ostrovsky wrote:
On 05/21/2018 01:40 AM, Oleksandr Andrushchenko wrote:
On 05/19/2018 01:04 AM, Boris Ostrovsky wrote:
On 05/17/2018 04:26 AM, Oleksandr Andrushchenko wrote:
From: Oleksandr Andrushchenko
<oleksandr_andrushchenko@xxxxxxxx>
A commit message would be useful.
Sure, v1 will have it
Signed-off-by: Oleksandr Andrushchenko
<oleksandr_andrushchenko@xxxxxxxx>

ÂÂÂÂÂÂÂÂÂ for (i = 0; i < nr_pages; i++) {
-ÂÂÂÂÂÂÂ page = alloc_page(gfp);
-ÂÂÂÂÂÂÂ if (page == NULL) {
-ÂÂÂÂÂÂÂÂÂÂÂ nr_pages = i;
-ÂÂÂÂÂÂÂÂÂÂÂ state = BP_EAGAIN;
-ÂÂÂÂÂÂÂÂÂÂÂ break;
+ÂÂÂÂÂÂÂ if (ext_pages) {
+ÂÂÂÂÂÂÂÂÂÂÂ page = ext_pages[i];
+ÂÂÂÂÂÂÂ } else {
+ÂÂÂÂÂÂÂÂÂÂÂ page = alloc_page(gfp);
+ÂÂÂÂÂÂÂÂÂÂÂ if (page == NULL) {
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ nr_pages = i;
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ state = BP_EAGAIN;
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ break;
+ÂÂÂÂÂÂÂÂÂÂÂ }
ÂÂÂÂÂÂÂÂÂÂÂÂÂ }
ÂÂÂÂÂÂÂÂÂÂÂÂÂ scrub_page(page);
ÂÂÂÂÂÂÂÂÂÂÂÂÂ list_add(&page->lru, &pages);
@@ -529,7 +565,7 @@ static enum bp_state
decrease_reservation(unsigned long nr_pages, gfp_t gfp)
ÂÂÂÂÂÂÂÂÂ i = 0;
ÂÂÂÂÂÂÂÂÂ list_for_each_entry_safe(page, tmp, &pages, lru) {
ÂÂÂÂÂÂÂÂÂÂÂÂÂ /* XENMEM_decrease_reservation requires a GFN */
-ÂÂÂÂÂÂÂ frame_list[i++] = xen_page_to_gfn(page);
+ÂÂÂÂÂÂÂ frames[i++] = xen_page_to_gfn(page);
ÂÂÂÂÂ Â #ifdef CONFIG_XEN_HAVE_PVMMU
ÂÂÂÂÂÂÂÂÂÂÂÂÂ /*
@@ -552,18 +588,22 @@ static enum bp_state
decrease_reservation(unsigned long nr_pages, gfp_t gfp)
ÂÂÂÂÂ #endif
ÂÂÂÂÂÂÂÂÂÂÂÂÂ list_del(&page->lru);
ÂÂÂÂÂ -ÂÂÂÂÂÂÂ balloon_append(page);
+ÂÂÂÂÂÂÂ if (!ext_pages)
+ÂÂÂÂÂÂÂÂÂÂÂ balloon_append(page);
So what you are proposing is not really ballooning. You are just
piggybacking on existing interfaces, aren't you?
Sort of. Basically I need to {increase|decrease}_reservation, not
actually
allocating ballooned pages.
Do you think I can simply EXPORT_SYMBOL for
{increase|decrease}_reservation?
Any other suggestion?
I am actually wondering how much of that code you end up reusing.
You
pretty much create new code paths in both routines and common code
ends
up being essentially the hypercall.
Well, I hoped that it would be easier to maintain if I modify
existing
code
to support both use-cases, but I am also ok to create new
routines if
this
seems to be reasonable - please let me know
ÂÂÂÂ So the question is --- would it make
sense to do all of this separately from the balloon driver?
This can be done, but which driver will host this code then? If we
move from
the balloon driver, then this could go to either gntdev or
grant-table.
What's your preference?
A separate module?
Is there any use for this feature outside of your zero-copy DRM
driver?
Intel's hyper dma-buf (Dongwon/Matt CC'ed), V4L/GPU at least.

At the time I tried to upstream zcopy driver it was discussed and
decided that
it would be better if I remove all DRM specific code and move it to
Xen drivers.
Thus, this RFC.

But it can also be implemented as a dedicated Xen dma-buf driver
which
will have all the
code from this RFC + a bit more (char/misc device handling at least).
This will also require a dedicated user-space library, just like
libxengnttab.so
for gntdev (now I have all new IOCTLs covered there).

If the idea of a dedicated Xen dma-buf driver seems to be more
attractive we
can work toward this solution. BTW, I do support this idea, but
was not
sure if Xen community accepts yet another driver which duplicates
quite some code
of the existing gntdev/balloon/grant-table. And now after this RFC I
hope that all cons
and pros of both dedicated driver and gntdev/balloon/grant-table
extension are
clearly seen and we can make a decision.
IIRC the objection for a separate module was in the context of gntdev
was discussion, because (among other things) people didn't want to
have
yet another file in /dev/xen/

Here we are talking about (a new) balloon-like module which doesn't
create any new user-visible interfaces. And as for duplicating code
---
as I said, I am not convinced there is much of duplication.

I might even argue that we should add a new config option for this
module.
I am not quite sure I am fully following you here: so, you suggest
that we have balloon.c unchanged, but instead create a new
module (namely a file under the same folder as balloon.c, e.g.
dma-buf-reservation.c) and move those {increase|decrease}_reservation
routines (specific to dma-buf) to that new file? And make it selectable
via Kconfig? If so, then how about the changes to grant-table and
gntdev?
Those will look inconsistent then.
Inconsistent with what? The changes to grant code will also be under the
new config option.
Ah, ok.

Option 1. We will have Kconfig option which will cover dma-buf
changes in balloon,
I really don't think your changes to balloon driver belong there. The
have nothing to do with ballooning,

grant-table and gntdev. And for that we will
create dedicated routines in balloon and grant-table (copy of
the existing ones, but modified to fit dma-buf use-case) and
those under something like "#if CONFIG_XEN_DMABUF"?
This is relatively easy to do for balloon/grant-table, but not that
easy for gntdev: there still seems to be lots of code which can be
reused,
so I'll have to put lots of "#if CONFIG_XEN_DMABUF" there. Even more,
I change
interfaces of the existing gntdev routines which won't look cute with
#if's, IMO.

Option 2. Try moving dma-buf related changes from balloon and
grant-table to a new file. Then gntdev's Kconfig concerns from above
will still
be there, but balloon/grant-table functionality will be localized in a
new module.
I don't see a problem with leaving your code (from patch 2) where it is
now, in grant table. It's a small change and it seems to me a single
#ifdef/#endif would cover it, even if you factor out common code there
as we've discussed. To my eye it logically belongs there. Just like your
gntdev changes belong to gntdev file. (Presumably, because I haven't
actually looked at them ;-))

So my suggestion is
- separate module for your changes in balloon.c
Ok, so, basically, the changes I need from the balloon driver is
{increase|decrease}_reservation and DMAable memory allocations, so
I'll move that into a separate file: what could be the name for such a file?

- keep grant-table changes, with config option
Can we consider moving ex-balloon code into grant-table?

- keep gntdev changes, with config option.
I'll try to see what happens to gntdev with Kconfig option wrt function prototype
changes. I also have to check if UAPI of gntdev can also support CONFIG_XXX ifdefs
w/o problems - do you by chance know if #if CONFIG_ is ok for UAPI files?
Or I can leave UAPI as is and ifdef in .ioctl callback.
(but when you get to post
actual patches I would appreciate if you could split this into a series
of logical changes and not post a one giant patch).
Of course, as this is at RFC stage the idea was to roll out all the changes at once, so
everyone has the full picture and don't need to collect changes from set of patches.

-boris

Thank you,
Oleksandr
I am still missing your point here?

If you suggest a new kernel driver module:
IMO, there is nothing bad if we create a dedicated kernel module
(driver) for Xen dma-buf handling selectable under Kconfig option.
Yes, this will create a yet another device under /dev/xen,
but most people will never see it if we set Kconfig to default to "n".
And then we'll need user-space support for that, so Xen tools will
be extended with libxendmabuf.so or so.
This way all Xen dma-buf support can be localized at one place which
might be easier to maintain. What is more it could be totally
transparent
to most of us as Kconfig option won't be set by default (both kernel
and Xen).
The downside is that we will end up having another device for doing
things that are not that different from what we are already doing with
existing gnttab device. Or are they?
Agree, but Kconfig option, IMO, won't make it look nice because
of gntdev changes and code reuse.
-boris
Thank you,
Oleksandr

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel