Re: [PATCH 6/7] ACPI: Page based coalescing of I/O remappings optimization

[Huang Ying <ying.huang@intel.com>]

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