Messages in this thread | | | Subject | Re: [Xen-devel] [RFC 1/3] xen/balloon: Allow allocating DMA buffers | From | Oleksandr Andrushchenko <> | Date | Tue, 22 May 2018 18:00:33 +0300 |
| |
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@epam.com> >>>>>>>>> A commit message would be useful. >>>>>>>> Sure, v1 will have it >>>>>>>>>> Signed-off-by: Oleksandr Andrushchenko >>>>>>>>>> <oleksandr_andrushchenko@epam.com> >>>>>>>>>> >>>>>>>>>> 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, 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 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
| |