This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Fix elision in nested trylock.


On Thu, 2013-06-27 at 21:16 +0200, Andi Kleen wrote:
> Ok I see what you mean. This can be simply fixed by moving the abort
> outside the adaption check, at the very beginning of
> __lll_trylock_elision.

That would be correct, but whether it's the right approach depends on
two things:
1) An xabort outside of a transaction must be fast.  If it's slow for
some reason, this would slow down locks for which we could but don't use
elision.
2) That it doesn't matter at all whether you abort transactions that
have trylock() calls in them (when elision is enabled globally, but not
used for this particular lock).

I don't see any comments about this, nor a discussion.  What was wrong
with "if (*try_lock > 0) _xabort(_ABORT_LOCK_BUSY);" (except perhaps a
missing builtin_expect)?  Finally, this only works for HTMs for which
xabort outside a transaction is a no-op.

This needs a more extensive comment.

> Then it will always abort, even if adapted.
> 
> I'll do this change. Thanks.

What I *strongly* dislike about how you applied the fix is that you
added just a tiny comment:
+  /* Implement broken POSIX semantics by forbidden nesting 
+     trylock. Sorry.  */

Why?

It wasn't clear to you at first what my patch was fixing, despite it
coming with an explanation and a larger comment in the code.  So if the
problem isn't obvious to you, why do you think it will be obvious to
anyone else?  Why?

You didn't even have to write an explanatory comment yourself, you could
have just used what I had in my patch.  If you didn't like what I had,
you could have just fixed it.  IOW, it would have been easy to do the
right thing.

Instead, you chose to replace it with a two-liner that is mostly a rant
about POSIX, and doesn't explain the problem nor the alternative
solutions at all.  Why?  Can't you see that this is a disservice to
anyone else who will have to read or maintain this code in the future,
and thus also a disservice to the project?

And if it's indeed a disservice to the project, what do you think we
should do to get the code into a state that makes maintenance easy?
Disapprove all your patches until you do the right thing and think about
making it easy for everyone else to understand and maintain your code?
Or should we just fix up the lack of documentation and other mess
afterwards on our own, assuming that there's no hope you will ever do
that?  This is seriously annoying to me.


Torvald


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]