| Message ID | alpine.DEB.2.21.1905221622190.20440@sstabellini-ThinkPad-T480s (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/swiotlb: don't initialize swiotlb twice on arm64 | expand |
Hi, On 23/05/2019 00:26, Stefano Stabellini wrote: > From: Stefano Stabellini <stefanos@xilinx.com> > > 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. 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? > > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> > > --- > > 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? > + 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) > Cheers,
On Thu, 23 May 2019, Julien Grall wrote: > On 23/05/2019 00:26, Stefano Stabellini wrote: > > From: Stefano Stabellini <stefanos@xilinx.com> > > > > 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@xilinx.com> > > > > --- > > > > 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)
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) { + 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)