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

[Stefano Stabellini <sstabellini@kernel.org>]

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)

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