Re: uma for acpi object cache
On 24/01/2013 19:49, 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. > >> 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? > >> 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. > Hello, I don't know if it's related, I tried your patch and sometime my dmesg is filled with that : Feb 11 19:06:16 Melon kernel: ACPI Warning: Large Reference Count (0x5F3E) in object 0xfe0003820990Large Reference Count (0x5F47) in object 0xfe00017f7798 (20110527/utdelete-481) Feb 11 19:06:16 Melon kernel: ACPI Warning: Large Reference Count (0x5F47) in object 0xfe0003820990 (20110527/utdelete-481) Feb 11 19:06:16 Melon kernel: ACPI Warning: Large Reference Count (0x5F47) in object 0xfe00018dcc18 (20110527/utdelete-481) Feb 11 19:06:16 Melon kernel: ACPI Warning: Large Reference Count (0x5F47) in object 0xfe00038207e0 (20110527/utdelete-481) Feb 11 19:06:16 Melon kernel: ACPI Warning: Large Reference Count (0x5F45) in object 0xfe00018076c0 (20110527/utdelete-481) Feb 11 19:06:16 Melon kernel: ACPI Warning: Large Reference Count (0x5F48) in object 0xfe00017f7798 (20110527/utdelete-481) Feb 11 19:06:16 Melon kernel: ACPI Warning: Large Reference Count (0x5F48) in object 0xfe0003820990 (20110527/utdelete-481) And my computer get just unusable, I must shutdown by force because that messages does not stop at all. I have more and more ACPI problems since I've updated to 9.1... ___ freebsd-acpi@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-acpi To unsubscribe, send any mail to "freebsd-acpi-unsubscr...@freebsd.org"
Re: uma for acpi object cache
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 2013-01-26 03:30:27 -0500, Andriy Gapon wrote: > on 25/01/2013 10:38 Andriy Gapon said the following: >> on 24/01/2013 22:33 Jung-uk Kim said the following: >>> On 2013-01-24 13:49:07 -0500, Andriy Gapon wrote: on 24/01/2013 20:29 Jung-uk Kim said the following: > 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. >> >> That's a very simple trick, I wonder why I didn't think about it >> :-) Now I need to wait until one of the reporters resurfaces. > > And just to clarify - I didn't and don't suspect the cache code > itself. I suspect some code that uses the cache (directly or > indirectly) - something like double-free or use-after-free, etc. I haven't tried it for ages but ACPI_DBG_TRACK_ALLOCATIONS was meant to track such problems. Try adding it in acfreebsd.h. Jung-uk Kim -BEGIN PGP SIGNATURE- Version: GnuPG v2.0.19 (FreeBSD) iQEcBAEBAgAGBQJRBsY6AAoJECXpabHZMqHO/sYH/0T/Imtxw70IwN5Ri7qP2C0G +zkZeUosV8hUI8xmXOdQMtyYhBXQprAoewXvAAnx0iCTLYtjNDt0lMGvoyyMDqOW bwupzYE+6GhaTDDc+8bfSxxMbDXgXqFuIqkohBnZPAQ7GdfLNwY2KHwGlA6T+o/s XailQ+sq9zN4VJ6SDoNAvpi1+O9FsO1GJBfnIihdY/S8b99n2ffpOO2XcAIIPwCE huxWnZ/GYVPmoVuvEKyc7CzzsGxRZfdQpJ5e7oPvwM5JAG7GEmc1VzRDPBtCFBWL VO3sBH3ObKX2+H0LhU9tskCVsmuzD0U+/ygF3P8NRkuLP7Y/yQiAWIoZeEr1saI= =05h0 -END PGP SIGNATURE- ___ freebsd-acpi@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-acpi To unsubscribe, send any mail to "freebsd-acpi-unsubscr...@freebsd.org"
Re: uma for acpi object cache
on 25/01/2013 10:38 Andriy Gapon said the following: > on 24/01/2013 22:33 Jung-uk Kim said the following: >> On 2013-01-24 13:49:07 -0500, Andriy Gapon wrote: >>> on 24/01/2013 20:29 Jung-uk Kim said the following: 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. > > That's a very simple trick, I wonder why I didn't think about it :-) > Now I need to wait until one of the reporters resurfaces. > And just to clarify - I didn't and don't suspect the cache code itself. I suspect some code that uses the cache (directly or indirectly) - something like double-free or use-after-free, etc. -- Andriy Gapon ___ freebsd-acpi@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-acpi To unsubscribe, send any mail to "freebsd-acpi-unsubscr...@freebsd.org"
Re: uma for acpi object cache
on 24/01/2013 22:33 Jung-uk Kim said the following: > On 2013-01-24 13:49:07 -0500, Andriy Gapon wrote: >> on 24/01/2013 20:29 Jung-uk Kim said the following: >>> 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. That's a very simple trick, I wonder why I didn't think about it :-) Now I need to wait until one of the reporters resurfaces. -- Andriy Gapon ___ freebsd-acpi@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-acpi To unsubscribe, send any mail to "freebsd-acpi-unsubscr...@freebsd.org"
Re: uma for acpi object cache
on 24/01/2013 22:33 Jung-uk Kim said the following: > On 2013-01-24 13:49:07 -0500, Andriy Gapon wrote: >> on 24/01/2013 20:29 Jung-uk Kim said the following: >>> 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. OK, I see. I don't think that any of that is really needed (now). If you don't object I'll commit my variant in 1-2 weeks from now. -- Andriy Gapon ___ freebsd-acpi@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-acpi To unsubscribe, send any mail to "freebsd-acpi-unsubscr...@freebsd.org"
Re: uma for acpi object cache
on 24/01/2013 22:33 Jung-uk Kim said the following: > 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? Hmm, not sure... Are you trying to improve behavior in use-after-free scenario or some such abnormal situation? My understanding is this. An object is either in use or it's free/cached. When it is in use, then LinkOffset has nothing to do with object (cache linking is not applied to the object). When it is cached, then it is not accessed as the real object (via ACPI_OBJECT_COMMON_HEADER fields), it is accessed only as a cached entity via the LinkOffset hackery. So, I do not see any bug in the current code. Your change makes it look a little bit less ugly, though. But I do not see any functional change. -- Andriy Gapon ___ freebsd-acpi@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-acpi To unsubscribe, send any mail to "freebsd-acpi-unsubscr...@freebsd.org"
Re: uma for acpi object cache
-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- ___ freebsd-acpi@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-acpi To unsubscribe, send any mail to "freebsd-acpi-unsubscr...@freebsd.org"
