Re: [Xen-devel] [PATCH] xen/swiotlb: don't initialize swiotlb twice on arm64

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 23 May 2019, Julien Grall wrote:
> On 23/05/2019 00:26, Stefano Stabellini wrote:
> > From: Stefano Stabellini <stefanos@xxxxxxxxxx>
> > 
> > On arm64 swiotlb is already initialized by mem_init. We don't want to
> 
> Arm64 will not always initialize the swiotlb. It will only be done if the user
> force it or there are memory above the DMA limit.
> 
> > initialize it twice, the memory is already allocated. Detect this
> > condition in swiotlb-xen and skip the second initialization.
> 
> I understand that the memory allocated by swiotlb will be replaced with
> freeing memory. So you at least have a memory leak.
> 
> However, the logic to allocate the memory is quite different. For instance,
> AFAICT, swiotlb will allocate low pages while xen swiotlb will alloc any
> pages.

That's right.


> So I think your commit message should contain a bit more details on the
> implication. I vaguely remember that on Xilinx on needed to use low memory as
> much as possible. Is this patch actually trying to fix that?

Yes, as a side-effect. Aside from the fruitless endeavor of allocating
memory twice, we also end up trading good low-memory pages for
high-memory pages. So, a side effect of this patch is that low-memory
pages become available via swiotlb-xen.

 
> > Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
> > 
> > ---
> > 
> > There are other issues which I found recently affecting the swiotlb on
> > arm64 -- I'll send the other patches separately.
> > 
> > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> > index 877baf2..8fcda2bf4 100644
> > --- a/drivers/xen/swiotlb-xen.c
> > +++ b/drivers/xen/swiotlb-xen.c
> > @@ -206,6 +206,7 @@ int __ref xen_swiotlb_init(int verbose, bool early)
> >     int rc = -ENOMEM;
> >     enum xen_swiotlb_err m_ret = XEN_SWIOTLB_UNKNOWN;
> >     unsigned int repeat = 3;
> > +   bool pre_initialized = false;
> >             xen_io_tlb_nslabs = swiotlb_nr_tbl();
> >   retry:
> > @@ -214,7 +215,10 @@ int __ref xen_swiotlb_init(int verbose, bool early)
> >     /*
> >      * Get IO TLB memory from any location.
> >      */
> > -   if (early) {
> > +   if (io_tlb_start != 0) {
> 
> Rather than adding an extra if in a already difficult code to read. Can we
> move the allocation in a separate function and only call it if necessary?

Maybe I have a better idea. If io_tlb_start != 0, we could skip
everything else in this function and basically just return.


> > +           xen_io_tlb_start = phys_to_virt(io_tlb_start);
> > +           pre_initialized = true;
> > +   } else if (early) {
> >             xen_io_tlb_start = memblock_alloc(PAGE_ALIGN(bytes),
> >                                               PAGE_SIZE);
> >             if (!xen_io_tlb_start)
> > @@ -264,7 +268,7 @@ int __ref xen_swiotlb_init(int verbose, bool early)
> >                      verbose))
> >                     panic("Cannot allocate SWIOTLB buffer");
> >             rc = 0;
> > -   } else
> > +   } else if (!pre_initialized)
> >             rc = swiotlb_late_init_with_tbl(xen_io_tlb_start,
> > xen_io_tlb_nslabs);
> >             if (!rc)

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel