linux-kernel - Re: [patch 1/5] seqlock: Remove unused functions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CA+55aFzCQEiV2ZfB3s983fe-zpyFhhc+QvpA58qeLWJxRJtU0A@mail.gmail.com>
Date:	Thu, 15 Mar 2012 09:29:50 -0700
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Thomas Gleixner <tglx@...utronix.de>
Cc:	LKML <linux-kernel@...r.kernel.org>,
	Al Viro <viro@...iv.linux.org.uk>, Ingo Molnar <mingo@...e.hu>,
	Peter Zijlstra <peterz@...radead.org>,
	Nick Piggin <npiggin@...nel.dk>
Subject: Re: [patch 1/5] seqlock: Remove unused functions

So I have to say, I hate this entire series.

Seriously, what the heck is the point of this churn? It's all entirely
pointless searc-and-replace as far as I can tell, with absolutely zero
upside.

It makes the low-level filesystems have to be aware of things that
they don't want to know and *shouldn't* know. Why should a filesystem
care that d_lock is a seqlock, and have to use a locking function that
they've never seen before and is very specialized?

The "seq" part of the dentry is something only the lookup code and the
internal dentry code should care about. NOBODY ELSE should ever care.

Also, we have actually tried to largely split the D$ lines, so the
d_seq field isn't even necessarily in the same cacheline as the d_lock
part. Very much on purpose: the beginning of the 'struct dentry' is
largely read-only for the lookup part, and can (hopefully) actually be
shared across CPU's for hot directory entries.

Sure, we may have screwed that up, and maybe it turns out that we
write to it too much, but it really was the *intention*. And you
fundamentally and totally screwed that up, and put the largely
read-only sequence count next to the d_lock thing.

So quite frankly, I think the whole series is total and utter garbage.
And there isn't even any *explanation" for the garbage. You say that
you are unifying things, but dammit, in order to unify them you end up
*adding*new*f&^#ing*code*. You add all those "seq_spin_trylock()" etc
counters that really shouldn't be added because nobody needs them, but
you have to add them because you turned what was a perfectly good
spinlock into a seq_spinlock.

I didn't do a full line count, but I think you added more lines than
you removed. The *only* actual removal of code was the few little "use
a seq_spin_init()" instead of initializing the sequence count and
spinlocks separately. Everything else was just search-and-replace with
less common functions.  And addition of those special function code.

Maybe there is some huge advantage that I'm missing - like the fact
that you could optimize the code using some very special new hardware
transactional memory trick that you have pre-production hardware for
now. But dammit, if that is the case, you should have written that out
in some big letters and explained exactly why you are sending out this
series that seems to be a lot of stupid code churn and that actually
makes the code noticeably worse, bigger, and less flexible.

So a honking big NAK on this whole series unless you can explain with
numbers and show with numbers what the advantage of the abortion is.

                               Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/