lkml.org 
[lkml]   [2012]   [Mar]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [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


    \
     
     \ /
      Last update: 2012-03-15 17:33    [from the cache]
    ©2003-2014 Jasper Spaans. Advertise on this site