On 05/22/2018 02:27 PM, Oleksandr Andrushchenko wrote: > 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? Naming would be your job ;-) > >> - keep grant-table changes, with config option > Can we consider moving ex-balloon code into grant-table? On the second thought --- yes, if the code is compact enough, which I think it is, you should be able to keep it there. > >> - 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? I would think that not but: ostr@workbase> git grep "#ifdef CONFIG_" include/uapi/ include/uapi/asm-generic/mman-common.h:#ifdef CONFIG_MMAP_ALLOW_UNINITIALIZED include/uapi/linux/atmdev.h:#ifdef CONFIG_COMPAT include/uapi/linux/elfcore.h:#ifdef CONFIG_BINFMT_ELF_FDPIC include/uapi/linux/eventpoll.h:#ifdef CONFIG_PM_SLEEP include/uapi/linux/fb.h:#ifdef CONFIG_FB_BACKLIGHT include/uapi/linux/flat.h:#ifdef CONFIG_BINFMT_SHARED_FLAT include/uapi/linux/hw_breakpoint.h:#ifdef CONFIG_HAVE_MIXED_BREAKPOINTS_REGS ostr@workbase> -boris > 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 > _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel