Message-ID: <8587f479-f5f0-0ca6-cd32-76fa94a9aed0@oracle.com>
Date:   Tue, 22 May 2018 15:09:17 -0400
From:   Boris Ostrovsky <boris.ostrovsky@...cle.com>
To:     Oleksandr Andrushchenko <andr2000@...il.com>,
        "Oleksandr_Andrushchenko@...m.com" <Oleksandr_Andrushchenko@...m.com>,
        xen-devel@...ts.xenproject.org, linux-kernel@...r.kernel.org,
        dri-devel@...ts.freedesktop.org, linux-media@...r.kernel.org,
        jgross@...e.com, konrad.wilk@...cle.com
Cc:     daniel.vetter@...el.com, matthew.d.roper@...el.com,
        dongwon.kim@...el.com
Subject: Re: [Xen-devel] [RFC 1/3] xen/balloon: Allow allocating DMA buffers

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@...m.com>
>>>>>>>>>>>> A commit message would be useful.
>>>>>>>>>>> Sure, v1 will have it
>>>>>>>>>>>>> Signed-off-by: Oleksandr Andrushchenko
>>>>>>>>>>>>> <oleksandr_andrushchenko@...m.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,
>> 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@...kbase> 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@...kbase>


-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@...ts.xenproject.org
>>> https://lists.xenproject.org/mailman/listinfo/xen-devel
>

Powered by blists - more mailing lists