lkml.org 
[lkml]   [2009]   [Apr]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] netfilter: use per-CPU r**ursive lock {XV}
    Linus Torvalds a écrit :
    >
    > On Mon, 27 Apr 2009, Linus Torvalds wrote:
    >> BTW: THIS IS TOTALLY UNTESTED.
    >
    > Gaah. I should have read through it one more time before sending.
    >
    >> static inline void xt_info_rdunlock_bh(void)
    >> {
    >> struct xt_info_lock *lock;
    >>
    >> lock = &__get_cpu_var(xt_info_locks);
    >> if (!--lock->readers)
    >> spin_unlock(&lock->lock);
    >> }
    >
    > This one was missing the "local_bh_enable()" at the end.
    >
    > There may be other bugs, but that's the one I noticed immediately when
    > reading what I sent out. Oops.

    I am not sure my day job will permit me to polish a patch mixing all
    the bits and comments. But I am glad we eventually got back spinlocks
    which are probably better than rwlocks for implementing this stuff.

    Instead of submitting a full patch again, we could first submit a new
    include file containg all comments and inline functions ?

    This include file could be local to netfilter, with a big stick on
    it to forbids its use on other areas (No changes in Documentation/ )

    Then, as soon as we can go back to pure RCU solution, we can safely
    delete this controversial-locking-nesting-per-cpu-thing ?


    Instead of local/global name that Paul suggested, that was about
    'global' locking all locks at the same time. Not any more the good
    name IMHO

    Maybe something like local/remote or owner/foreigner ?

    xt_info_owner_lock_bh(), xt_info_owner_unlock_bh()
    xt_info_foreigner_lock(), xt_info_foreigner_unlock()

    One comment about this comment you wrote :

    /*
    * The "writer" side needs to get exclusive access to the lock,
    * regardless of readers. This must be called with bottom half
    * processing (and thus also preemption) disabled.
    */
    static inline void xt_info_wrlock(unsigned int cpu)
    {
    spin_lock(&per_cpu(xt_info_locks, cpu).lock);
    }

    static inline void xt_info_wrunlock(unsigned int cpu)
    {
    spin_unlock(&per_cpu(xt_info_locks, cpu).lock);
    }

    Its true that BH should be disabled if caller runs
    on the cpu it wants to lock.
    For other ones (true foreigners), there is
    no requirement about BH (current cpu could be interrupted
    by a softirq and packets could fly)

    We could use following construct and not require disabling BH
    more than a short period of time.
    (But preemption disabled for the whole duration)

    preempt_disable(); // could be cpu_migration_disable();

    int curcpu = smp_processor_id();
    /*
    * Gather stats for current cpu : must disable BH
    * before trying to lock.
    */
    local_bh_disable();
    xt_info_wrlock(curcpu);
    // copy stats of this cpu on my private data (not shown here)
    xt_info_wrunlock(curcpu);
    local_bh_enable();

    for_each_possible_cpu(cpu) {
    if (cpu == curcpu)
    continue;
    xt_info_wrlock(cpu);
    // fold stats of "cpu" on my private data (not shown here)
    xt_info_wrunlock((cpu);
    }
    preempt_enable(); // could be cpu_migration_enable();


    So your initial comment could be changed to :

    /*
    * The "writer" side needs to get exclusive access to the lock,
    * regardless of readers. If caller is about to lock its own lock,
    * he must have disabled BH before. For other cpus, no special
    * care but preemption disabled to guarantee no cpu migration.
    */

    Back to work now :)

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

    \
     
     \ /
      Last update: 2009-04-28 09:07    [W:2.520 / U:0.732 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site