Date: Thu, 29 Jan 2015 15:41:09 +0000 From: "rrs (Randall Stewart)" <phabric-noreply@FreeBSD.org> To: freebsd-net@freebsd.org Subject: [Differential] [Commented On] D1711: Changes to the callout code to restore active semantics and also add a test-framework and test to validate thecallout code (and potentially for use by other tests). Message-ID: <6b9877fabfc054b3bce9c3a97f7e0455@localhost.localdomain> In-Reply-To: <differential-rev-PHID-DREV-vhk6ww63dvpj6egspuyt-req@FreeBSD.org> References: <differential-rev-PHID-DREV-vhk6ww63dvpj6egspuyt-req@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
rrs added a comment.
To answer jhb and kostikbel
1) Yes this does address the two issues that Hans re-write of the callout system did without changing the KPI.
There may be other bugs as well, but with the test framework and the old code I could reproduce both issues
(spin lock held to long and broken callout_active() semantic). The spin lock held to long I was able
to get a vmcore and actually pinpoint *exactly* why it happened and *exactly* how the patch fixes it (as
Gleb would say, no mystery ...)
2) Yes, its a good idea to split the test framework out as imp suggested, which I will do into test framework and callout changes.
3) As to simpler testing, yes we originally were running the test out of the kldload, but this causes
at least two problems. It appears that the GIANT lock is held during kldload, any test that needs to do locking
can experience LOR's and other fun stuff with Witness on. Second it becomes *much* harder to do multi-threaded testing.
This is/was essential to find the bugs and reproduce them in the callout system. Basically you just put
num_threads = 2 and you have two kthreads running the tests which helps located nasty races more easily ;-)
4) Imp, in an early version of the patch, complained I was not using the macros and what he had
complained about was just code that I cut and paste. Thats what prompted me to fix and use
the macro's properly throughout the code. Backing that out would be painful at this point so
I see no need to split it out except to do a *lot* of extra work.
REVISION DETAIL
https://reviews.freebsd.org/D1711
To: rrs, gnn, rwatson, adrian, sbruno, lstewart, imp, hselasky
Cc: jhb, kostikbel, emaste, delphij, neel, erj, freebsd-net
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?6b9877fabfc054b3bce9c3a97f7e0455>