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

[Oleksandr Andrushchenko]

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.

> -boris
Thank you,
Oleksandr
[1] https://lists.freedesktop.org/archives/dri-devel/2018-April/173163.html