Hash: SHA1

On 2013-01-24 13:49:07 -0500, Andriy Gapon wrote:
> on 24/01/2013 20:29 Jung-uk Kim said the following:
>> On 2013-01-24 04:41:08 -0500, Andriy Gapon wrote:
>>> on 24/01/2013 02:54 Jung-uk Kim said the following: I think
>>> that I have a much better patch for all potential ACPI object
>>> cache problems :-) 
>>> http://people.freebsd.org/~avg/acpi-uma-cache.diff
>>> What do you think?
>> We have to fix this bug because local cache is always used for 
>> userland applications, e.g., iasl.
> Could you please clarify what problem/bug is fixed by that patch? I
> looked hard but couldn't spot any difference besides moving the
> link pointer from offset 8 to offset 0.

If I am not completely mistaken, this is what's happening:


Please see ACPI_OBJECT_COMMON_HEADER macro change in the commit I
mentioned the pull request.

Before the commit:
    UINT8                           Descriptor;
    UINT8                           Type;
    UINT16                          ReferenceCount;
    union acpi_operand_object       *NextObject;
    UINT8                           Flags;

After the commit:
    union acpi_operand_object       *NextObject;
    UINT8                           DescriptorType;
    UINT8                           Type;
    UINT16                          ReferenceCount;
    UINT8                           Flags;

It may not look obvious but LinkOffset was used to keep offset of
NextObject (and it was only "safe" for certain compilers & platforms).

Alas, it is completely bogus now because the pointer became the first
element of any object.  Although it was the right decision, the author
forgot to change the LinkOffset with this commit, I guess.  Now,
modifying DescriptorType, Type, ReferenceCount, or Flags silently
corrupts the linked list.  Similarly, modifying any of these before
setting the pointer gets silently clobbered.  For example,
ACPI_SET_DESCRIPTOR_TYPE() in AcpiOsReleaseObject() is effectively
no-op because it's overwritten later.

Does it make sense to you?

>> BTW, I tried something like that long ago.  In fact, the first
>> attempt goes all the way back to this patch (warning: it's naive,
>> broken, and overly complicated):
>> http://people.freebsd.org/~jkim/acpica/OsdCache.diff
>> I have more up-to-date and correct patch to use UMA but I'm still
>> not 100% convinced whether we want to do it or not.
> Hmm, your patch looks a bit more complicated than mine. What is all
> that extra stuff that you have there?

The main issue was AcpiOsPurgeCache().  For example, we didn't have
anything like Linux's kmem_cache_shrink() at the time:


It seems you implemented that with zone_drain() but it wasn't
available until this commit:


Also, I had to make sure the cache is empty before we do
uma_zdestroy(), so on and so forth.

>> When utcache.c works, it works fairly well, actually. :-)
> Well, my primary motivation for the patch is all the reports about
> mysterious panics that seem to involve the cache: 
> http://thread.gmane.org/gmane.os.freebsd.devel.acpi/7562 
> http://thread.gmane.org/gmane.os.freebsd.devel.acpi/7613 
> http://thread.gmane.org/gmane.os.freebsd.devel.acpi/7077
> There were a few more reports with the same theme. I hoped that
> using uma(9) instead of hand-rolled code would lead to better 
> diagnostic and debugging cabilities.

Hmm...  I am not really sure local cache is to blame here.  If you
really want to prove your theory, I think a simple modification to
utcache.c should do:

     Cache->LinkOffset = 8;
     Cache->ListName   = CacheName;
     Cache->ObjectSize = ObjectSize;
- -    Cache->MaxDepth   = MaxDepth;
+    Cache->MaxDepth   = 0;

     *ReturnCache = Cache;
     return (AE_OK);

This should effectively kill object caching.

Jung-uk Kim
Version: GnuPG v2.0.19 (FreeBSD)

freebsd-acpi@freebsd.org mailing list
To unsubscribe, send any mail to "freebsd-acpi-unsubscr...@freebsd.org"

Reply via email to