-
Notifications
You must be signed in to change notification settings - Fork 97
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
logging: Remove linker 'magic' and just use statics for logging callsites #322
Conversation
Once more, The justification is unsound to say the least.
I beg your pardon? Are there actual persistent issues around this feature? Let's fix them. Is any client program too worry to run into problems? Come on, point me at any project and I am confident I can find a feature But it always comes at cost of something else, here:
|
6a4a4b2
to
353981c
Compare
ack from me. |
count me in the queue of ackers, so ACK |
Add one more disruption to already a long list why this is basically
Congrats! |
Thanks for the solution. Is there any plan when to release a new version with this? It appears dlm_stonith is basically broken with libqb-1.0.3:
|
Rebuilding dlm might fix that. I'm going to start on a 2.0rc series shortly. Once we get the hi-res timestamps merged I think. |
Thanks for the information. It was rebuilt but didn't work. But now I realized dlm_stonith calls stonith_api_kick_helper() but it's not explicitly linked with libstonithd according to the Makefile. No idea why, but it's probably relevant... |
Oh, well, stonith_api_kick_helper() is an inline function from pacemaker's stonith-ng.h header file which manipulates stuff with dlopen() and dlsym(). This doesn't seem to work with libqb-1.0.3 though. |
@jfriesse which version of libqb is correct? |
@jfriesse which version of libqb is the corosync 3.0.2 relayed? |
@GitSoftwareNow Corosync works with most of the versions, but you must keep in sync the installed version and version which is corosync linked with. Currently I wouldn't be to hesitate to recommend current git master. Also please consider to use corosync issue you've opened for other questions, because commenting under already merged libqb PR is not optimal. |
It is my (and several others') opinion that the linker 'magic' used to maintain callsite data in libqb is hugely over complicated and unnecessarily fragile. It's main purpose seems to have been to improve performance but empirical testing shows this to be tiny at best. The overhead of sprintf makes minor optimisations in this code pointless.
With this code removed, libqb allocates callsites using a C static variable at run-time. This sounds bad but in actuality it merely moves the allocation from program load time to the first few milliseconds of program run-time. Applications like corosync and pacemaker spend most of their time in small loops doing the same work over and over again so the overhead doesn't apply and jitter does not occur.
We've tested this with corosync and pacemaker under valgrind and massif and the differences are minimal and even then only show up under artificial stress testing.
For this change I've bumped the soname up to 20 to indicate this is an incompatible change. I'm open to suggestions as to a release number but am currently thinking of 1.2.0