Re: [PATCH 1/2] udp: introduce struct udp_table and multiple rwlocks

[Eric Dumazet <dada1@cosmosbay.com>]

Christian Bell a écrit :
> 
> On Oct 28, 2008, at 1:37 PM, Eric Dumazet wrote:
> 
>> -extern struct hlist_head udp_hash[UDP_HTABLE_SIZE];
>> -extern rwlock_t udp_hash_lock;
>> +struct udp_hslot {
>> +    struct hlist_head    head;
>> +    rwlock_t        lock;
>> +};
> 
> This structure should be aligned up to cacheline to reduce false sharing 
> of more than one hslot.


Yes, I though about that. But : a full cache line is a waste of memory, and
choosing a power of two alignement is not easy because of 32bit/64bit arches,
and fact that sozepf(wrlock_t) can be > 4 if DEBUG

> 
>> +    } else {
>> +        hslot = &udptable->hash[udp_hashfn(net, snum)];
>> +        write_lock_bh(&hslot->lock);
>> +        if (udp_lib_lport_inuse(net, snum, hslot, sk, saddr_comp))
>> +            goto fail;
> 
> The fail: label below should still unlock_bh when the above condition 
> fails.
> 
>>
>> +    }
>>     inet_sk(sk)->num = snum;
>>     sk->sk_hash = snum;
>>     if (sk_unhashed(sk)) {
>> -        sk_add_node(sk, &udptable[udp_hashfn(net, snum)]);
>> +        sk_add_node(sk, &hslot->head);
>>         sock_prot_inuse_add(sock_net(sk), sk->sk_prot, 1);
>>     }
>> +    write_unlock_bh(&hslot->lock);
>>     error = 0;
>> fail:
>> -    write_unlock_bh(&udp_hash_lock);
>>     return error;
>> }

Good spoting, the write_unlock_bh(&hslot->lock); must be moved after the "fail:" label.

Thanks a lot