Konrad Rzeszutek Wilk <konrad.w...@oracle.com> writes: > On Mon, Jan 07, 2013 at 06:22:51PM -0800, Eric W. Biederman wrote: >> Shuah Khan <shuahk...@gmail.com> writes: >> >> > On Mon, Jan 7, 2013 at 8:26 AM, Konrad Rzeszutek Wilk >> > <konrad.w...@oracle.com> wrote: >> >> On Fri, Jan 04, 2013 at 02:10:25PM -0800, Yinghai Lu wrote: >> >>> On Fri, Jan 4, 2013 at 1:02 PM, Shuah Khan <shuahk...@gmail.com> wrote: >> >>> > Pani'cing the system doesn't sound like a good option to me in this >> >>> > case. This change to disable swiotlb is made for kdump. However, with >> >>> > this change several system fail to boot, unless crashkernel_low=72M is >> >>> > specified. >> >>> >> >>> this patchset is new feature to put second kdump kernel above 4G. >> >>> >> >>> > >> >>> > I would the say the right approach to solve this would be to not >> >>> > change the current pci_swiotlb_detect_override() behavior and treat >> >>> > swiotlb =1 upon entry equivalent to swiotlb_force set. >> >>> >> >>> that will make intel system have to take crashkernel_low=72M too. >> >>> otherwise intel system will get panic during swiotlb allocation. >> >> >> >> Two things: >> >> >> >> 1). You need to wrap the 'is_enough_..' in CONFIG_KEXEC, which means >> >> that the function needs to go in a header file. >> >> 2). The check for 1MB is suspect. Why only 1MB? You mentioned it is >> >> b/c of crashkernel_low=72M (which I am not seeing in v3.8 >> >> kernel-parameters.txt? >> >> Is that part of your mega-patchset?). Anyhow, there seems to be a >> >> disconnect - >> >> what if the user supplied crashkernel_low=27M? Perhaps the >> >> 'is_enough' >> >> should also parse the bootparams to double-check that there is enough >> >> low-mem space? But then if the kernel grows then 72M might not be >> >> enough - >> >> you might need 82M with 3.9. >> >> >> >> Perhaps a better way for this is to do: >> >> 1). Change 'is_enough' to check only for 4MB. >> >> 2). When booting as kexec, the SWIOTLB would only use 4MB instead >> >> of 64MB? >> >> >> >> Or, we could also use the post-late SWIOTLB initialization similiary >> >> to how it was >> >> done on ia64. This would mean that the AMD VI code would just call >> >> the >> >> .. something like this - NOT tested or even compile tested: >> >> >> >> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c >> >> index c1c74e0..e7fa8f7 100644 >> >> --- a/drivers/iommu/amd_iommu.c >> >> +++ b/drivers/iommu/amd_iommu.c >> >> @@ -3173,6 +3173,24 @@ int __init amd_iommu_init_dma_ops(void) >> >> if (unhandled && max_pfn > MAX_DMA32_PFN) { >> >> /* There are unhandled devices - initialize swiotlb for >> >> them */ >> >> swiotlb = 1; >> >> + /* Late (so no bootmem allocator) usage and only if the >> >> early SWIOTLB >> >> + * hadn't been allocated (which can happen on kexec >> >> kernels booted >> >> + * above 4GB). */ >> >> + if (!swiotlb_nr_tbl()) { >> >> + int retry = 3; >> >> + int mb_size = 64; >> >> + int rc = 0; >> >> +retry_me: >> >> + if (retry < 0) >> >> + panic("We tried setting %dMB for SWIOTLB >> >> but got -ENOMEM", mb_size << 1); >> >> + rc = swiotlb_late_init_with_default_size(mb_size >> >> * (1<<20)); >> >> + if (rc) { >> >> + retry --; >> >> + mb_size >> 1; >> >> + goto retry_me; >> >> + } >> >> + dma_ops = &swiotlb_dma_ops; >> >> + } >> >> } >> >> >> >> amd_iommu_stats_init(); >> >> >> >> And then the early SWIOTLB initialization for 64MB can fail and we are >> >> still OK. >> >>> >> > >> > Yinghai/Konrad, >> > >> > Did more testing. btw this patch depends on your [v7u1,25/31] >> > memblock: add memblock_mem_size(). Here are the test results: >> > >> > 1. When there is not enough memory: (enough_mem_for_swiotlb() returns >> > false) >> > system will panic in amd_iommu_init_dma_ops(). >> > >> > 2. When there is enough memory: (enough_mem_for_swiotlb() returns true): >> > swiotlb is reserved >> > pci_swiotlb_late_init() leaves the buffer allocated since swiotlb=1 >> > with that getting changed in amd_iommu_init_dma_ops(). >> > >> > I agree with Konrad that the logic should be wrapped in CONFIG_KEXEC. >> >> If enough_mem_for_swiotlb needs to be conditional on CONFIG_KEXEC the >> code is architected wrong. None of this logic has anything to do with >> kexec except that the kexec path is one way to get this condition to >> happen. Especially since the kexec'd kernel where this condition occurs >> does not need kexec support built in. > > Fair enough - with the 'memmap' command line options one can trigger > this. >> >> Yinghai I sat down and read your patch and the approach you are taking >> is totally wrong. >> >> The problem is that swiotlb_init() in lib/swiotlb.c does not know how to >> fail without panic'ing the system. >> >> Which leaves two valid approaches. >> - Create a variant of swiotlb_init that can fail for use on x86 and >> handle the failure. > > As an safe-fail step we could retry with an smaller size until a fit is found. > >> - Delay initializing the swiotlb until someone actually needs a mapping >> from it. > > So late init the SWIOTLB and perhaps have multiple "segments" of 4MB > of SWIOTLB that can grow as we exhaust its memory. Could work. >> >> Delaying the initialization of the swiotlb is out because the code >> needs an early memory allocation to get a large chunk of contiguous >> memory for the bounce buffers. > > Or it can use the late init, but with a smaller chunk of memory.
A reasonable point. >> Which means the panics that occurr in swiotlb_init() need to be delayed >> until someone something actually needs bounce buffers from the swiotlb. >> >> Although arguably what should actually happen instead of panic() is that >> swiotlb_map_single should simply fail early when it was not possible to >> preallocate bounce buffers. > > This sounds like a Catch-22. Fail early implies that it would have to do > this when using the bootmem allocator. Sorry I meant was something like. ... swiotlb_init(...) { ... if (!alloc_bootmem_low_pages(...)) noiotlb_memory = true; ... } ... swiotlb_map_single(...) { if (noiotlb_memory) return SWIOTLB_MAP_ERROR; .... } With the noiotlb_memory check happening early. > But the swiotlb_map_single is not > called at that time - it is called _after_ the bootmem allocator has been > de-activated. Actually it is called pretty late - when built-in PCI devices > start off or when 'udev' starts scanning the PCI bus and loading modules. > > I think I am misunderstanding you - could you clarify please? Does the above help? Eric -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/