uma for acpi object cache

[Jung-uk Kim]

-----BEGIN PGP SIGNED MESSAGE-----
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:

https://github.com/otcshare/acpica/pull/3

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:

http://www.kernel.org/doc/htmldocs/kernel-api/API-kmem-cache-shrink.html

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

http://svnweb.freebsd.org/base?view=revision&revision=166213

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
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.19 (FreeBSD)

iQEcBAEBAgAGBQJRAZquAAoJECXpabHZMqHOE9EIANaY52hh9wpflBCsISJHHmS0
MTtrEiLeC+SqUd8Z+WN0QCLkg9xitryuoyDEK+bMKfn5p5zjJQEL4OyEHSuN37I3
j06UU8gcti6Q8nv5f0EjgT/RR9WR8/AJfIta6neaiX+5cZxARpj86avD+hf8Mv71
7LiiDtbDIzkwf4bXM0kkhs5+UPCqlkCzZUHzMNQ8CZsmtIy8vfw3wagpYfX0nMhN
YjdZkADo2f46lgZw409VBOxfwewrzrhYWeCG3ETPBM0YCYRsmU47dWNlnWFkqIQY
OZT4BIu0sHtGYzCwamWKBDCSklpzGgYqk2V4eRZcm8b/BLCnS712GkqZfNYsei0=
=ObAy
-----END PGP SIGNATURE-----