Date: Thu, 24 Jan 2013 15:33:50 -0500 From: Jung-uk Kim <jkim@FreeBSD.org> To: Andriy Gapon <avg@freebsd.org> Cc: freebsd-acpi@freebsd.org Subject: Re: uma for acpi object cache Message-ID: <51019AAE.10501@FreeBSD.org> In-Reply-To: <51018223.4030702@FreeBSD.org> References: <20130122175629.GA1714@garage.freebsd.pl> <51008661.4060006@FreeBSD.org> <510101B4.4030409@FreeBSD.org> <51017D79.6060202@FreeBSD.org> <51018223.4030702@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
-----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-----
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?51019AAE.10501>