Hi, Myron, On Sat, 2010-10-23 at 00:27 +0800, Myron Stowe wrote: > On Fri, 2010-10-22 at 13:17 +0800, Huang Ying wrote: > > On Fri, 2010-10-22 at 11:23 +0800, Myron Stowe wrote: > > > On Fri, 2010-10-22 at 09:03 +0800, Huang Ying wrote: > > > > Hi, Myron, > > > > > <snip> > > > > > > > > Why not check the existing map (that is, acpi_map_lookup()) before the > > > > ioremap()? In APEI tables, several GAS may be in same page, so we need > > > > ioremap() for the first page only. > > > > > > Yes, even in other instances, several registers (GAS) may reside within > > > the same page(s) of a previously, still active, ioremapping and thus > > > only one ioremapping needs to be in place (with the subsequent > > > mapping(s) that fall within the same page(s) of the existing, active, > > > remapping then only needing to increment its reference). > > > > > > > > > In the existing implementation - atomicio.c:acpi_pre_map() - there is a > > > check at the beginning of the routine to see if a new mapping fits > > > within the page(s) of a previous entry's page(s) that is still active: > > > __acpi_try_ioremap(). Then later in the same routine, the same > > > __acpi_try_ioremap() check has to be made again due to a potential mp > > > race between the initial check and the insertion of a potentially new > > > entry onto the list. > > > > > > Since the second check *must* take place, > > > > No. If the first check succeeds, the second check is not necessary, it > > is sufficient just to increase the reference count and return the mapped > > virtual address. The first check will eliminate > > kmalloc/ioremap/iounmap/kfree for physical address has been mapped by > > previous calling. > > Hay Haung: > > Let's take two scenarios and see if I can express myself better why I > believe that the second check must take place. Let's also stick with > the existing atomicio.c:acpi_pre_map() implementation for now just to > keep things simpler. > > > Scenario 1: Assume that an MMIO remapping is already in place - mapping > A. At some later point in time another call is made to map in some > other MMIO area and this mapping - mapping B - resides within the same > page(s) of mapping A which is still active, and thus, still on the list. > > In this scenario, the first __acpi_try_ioremap() check will detect that > mapping B resides within mapping A, increment the reference of mapping > A, and then acpi_pre_map() short circuits out. Things are handled > correctly and the first check nicely short circuits out so the second > check was never even reached. > > Now let's take another scenario, Scenario 2: Assume that an MMIO > remapping is occurring on some processor of a MP system to put mapping A > into place (so the the processor is processing code somewhere between > "pg_off = ..." and "kref_init()"). While the processor is in the > process of putting mapping A into place it becomes blocked for any of a > number of reasons. Another processor now runs and puts a mapping into > place - mapping C - that is equal to or greater in size than mapping A > which also encompasses the same page(s) of mapping A. So now, at this > point in time, mapping C is on the list but mapping A is not. > > At some later point in time the processor running mapping A will > continue. It acquires the spin_lock immediately following the > "kref_init()" and must now check again to see if some other processor > put a similar mapping into place. In this scenario, exactly such has > happened - which is why I think you put this check in place to begin > with. The MP race is properly detected by the second > __acpi_try_ioremap() check which increments the reference of mapping C > and mapping A is backed out (iounmap/kfree). > > So with the existing atomicio.c:acpi_pre_map() implementation there are > two __acpi_try_ioremap() checks. The first check handles situations > like Scenario 1 and the second check handles situations like Scenario 2. > > It was situations like Scenario 2 that I was thinking of in my initial > response when I said that the second check *must* take place. I agree, > as indicated above, in situations like Scenario 1 the second check is > never even reached. > > Hopefully I have now expressed better why I believe the second check > *must* take place? > > > Now back to your original question - why did I not include the first > check in the re-factoring? > > My thinking was along the lines of: "Yes, the first check is nice in > that it short circuits additional remappings that match existing > mappings. Thinking further, consider that since the second check must > still occur due to situations like Scenario 2, and, since the second > check would not only catch situations like Scenario 2 but also > additionally catches any additional remappings matching existing ones in > place like Scenario 1 - why not just keep the code simpler/smaller and > not include the first check?" > > I agree that such a tack does have trade offs. I chose to keep the code > simpler/smaller at the expense of giving up the "short circuit" benefits > that the first check provides in situations like Scenario 1. The code, > without the first check still handles situations like Scenario 1 - just > not as efficiently (i.e. it incurs the expense of > "kmalloc/ioremap/iounmap/kfree for physical address has been mapped by > previous calling" as you properly pointed out. > > With this explanation of my actions do you see any issues with such an > approach? Now, I understand your idea. But I still think it is better to keep the first check. Because it has some benefit with the cost of just a few lines of simple code. I think it should have no much harm to code readability. Best Regards, Huang Ying -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
