Message-ID: <f8775649-34eb-04ac-2264-609b33cdd504@oracle.com>
Date:   Tue, 22 May 2018 10:33:58 -0400
From:   Boris Ostrovsky <boris.ostrovsky@...cle.com>
To:     Oleksandr Andrushchenko <andr2000@...il.com>,
        Oleksandr Andrushchenko <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 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.


>
> 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?

-boris

Powered by blists - more mailing lists