Hello, Tang. On Thu, Jun 20, 2013 at 01:52:50PM +0800, Tang Chen wrote: > 1. It is difficult to tell which memory allocation is temporary and > which one is permanent when memblock is allocating memory. So, we > can only wait till boot is complete, and see which remains. > But, we have the second difficulty. > > 2. In memblock.reserve[], we cannot tell why we allocated this memory > just from the array item, right? So it is difficult to do the > relocation. If in the future, we have to allocate permanent memory > for other new purposes, we have to do the relocation again and again. > (Not sure if I understand the point correctly. I think there isn't > a generic way to relocate memory used for different purposes.) I was suggesting two separate things. * As memblock allocator can relocate itself. There's no point in avoiding setting NUMA node while parsing and registering NUMA topology. Just parse and register NUMA info and later tell it to relocate itself out of hot-pluggable node. A number of patches in the series is doing this dancing - carefully reordering NUMA probing. No need to do that. It's really fragile thing to do. * Once you get the above out of the way, I don't think there are a lot of permanent allocations in the way before NUMA is initialized. Re-order the remaining ones if that's cleaner to do. If that gets overly messy / fragile, copying them around or freeing and reloading afterwards could be an option too. There isn't much point in being super-efficient about ACPI override table. Being cleaner and more robust is far more important. As for distinguishing temporary / permanent, it shouldn't be difficult to make memblock track all allocations before NUMA info becomes online and then verify that those areas are free by the time boot is complete. Just mark the reserved areas allocated before NUMA info is fully available. > If you also had a look at the Part2 patches, you will see that I > introduced a flags member into memblock to specify different types > of memory, which will help to recognize hotpluggable memory. My > thinking is that ensure memblock will not allocate hotpluggable > memory. I think this is the most safe and easy way to satisfy hotplug > requirement. And you can use exactly the same mechanism to track memory areas which were allocated before NUMA info was fully available, right? > So you don't agree to serialize the operations at boot time. No, I'm not disagreeing that some ordering is necessary. My point is that things seem to be going that way too far. Sure, some reordering is necessary but it doesn't have to be this fragile. Careful reordering isn't the only way to achieve it. > About this patch-set from Yinghai, actually he is doing a job that I > failed to do. And he also included a lot of other things in the > patch-set, such as extend max number of overridable acpi tables, local > node pagetable, and so on. Doing multiple things to achieve a goal in a patchset might not be optimal but is usually okay if properly explained. What's not okay is not explaining the overall goal, approach and design in the head message, poor quality of patch description and code documentation. This part of code is almost inherently fragile and difficult to debug and patchset like this would degrade the maintainability and I really don't want to spend hours trying to decipher what the overall approach is by trying to navigate maze of poorly documented patches only to find out that some of the basic approaches are not very agreeable. We could have had this exact discussion way earlier if the head message properly described what was going on and the review process would have been much more pleasant for all involved parties. I don't think it matters whose patches go in how as long as they are attributed correctly. The end result - what goes in the git tree as log and code changes - matters, and it needs to be whole lot better. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
