netdev - Re: [PATCH 1/2] udp: introduce struct udp

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: <490788B2.1060301@cosmosbay.com>
Date:	Tue, 28 Oct 2008 22:48:34 +0100
From:	Eric Dumazet <dada1@...mosbay.com>
To:	Christian Bell <christian@...i.com>
CC:	netdev@...r.kernel.org
Subject: Re: [PATCH 1/2] udp: introduce struct udp_table and multiple rwlocks

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


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html