Re: [PATCH v1] hash: add tsx support for cuckoo hash

["Shen, Wei1" <wei1.shen@intel.com>]

Hi Stephen,

Greetings. Thanks for your great feedback. Let’s me address your concern here.

1) It changes ABI, so it breaks old programs
The patch uses the extra_flag field in the rte_hash_parameters struct to set the default insertion behavior. Today there is only one bit used by this flag (RTE_HASH_EXTRA_FLAGS_TRANS_MEM_SUPPORT 0x1) and we used the next unused bit (RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD 0x2) in this patch. So ABI are maintained.

2) What about older processors, need to detect and handle them at runtime.
Correct. This patch is based on the previous Transactional Memory patch. Since these previous patches already assume the user to check the presence of TSX so we build on top this assumption. But I personally agree with you that handling TSX check should be made easier.
http://dpdk.org/ml/archives/dev/2015-June/018571.html
http://dpdk.org/ml/archives/dev/2015-June/018566.html 

3) Why can't this just be the default behavior with correct fallback to locking on older processors.
This is an excellent point. We discussed this before. Our thought at that time is, since TSX insertion is a bit slower than without anything (TSX or other locks), it would benefit apps that is designed to have a single writer to the hash table (for instance in some master-slave model). We might need more feedback from user about whether making it default is more desirable if most the app is designed with multi-writer manner.

Thanks,


-- 
Best,

Wei Shen.






On 5/6/16, 9:56 PM, "Stephen Hemminger" <stephen@networkplumber.org> wrote:

>On Fri,  6 May 2016 21:05:02 +0100
>Shen Wei <wei1.shen@intel.com> wrote:
>
>> --- a/lib/librte_hash/rte_cuckoo_hash.c
>> +++ b/lib/librte_hash/rte_cuckoo_hash.c
>> @@ -1,7 +1,7 @@
>>  /*-
>>   *   BSD LICENSE
>>   *
>> - *   Copyright(c) 2010-2015 Intel Corporation. All rights reserved.
>> + *   Copyright(c) 2010-2016 Intel Corporation. All rights reserved.
>>   *   All rights reserved.
>>   *
>>   *   Redistribution and use in source and binary forms, with or without
>> @@ -100,7 +100,9 @@ EAL_REGISTER_TAILQ(rte_hash_tailq)
>>  
>>  #define KEY_ALIGNMENT			16
>>  
>> -#define LCORE_CACHE_SIZE		8
>> +#define LCORE_CACHE_SIZE		64
>> +
>> +#define RTE_HASH_BFS_QUEUEs_MAX_LEN  5000
>>  
>>  #if defined(RTE_ARCH_X86) || defined(RTE_ARCH_ARM64)
>>  /*
>> @@ -190,6 +192,7 @@ struct rte_hash {
>>  							memory support */
>>  	struct lcore_cache *local_free_slots;
>>  	/**< Local cache per lcore, storing some indexes of the free slots */
>> +	uint8_t multiwrite_add; /**< Multi-write safe hash add behavior */
>>  } __rte_cache_aligned;
>>  
>
>I like the idea of using TSX to allow multi-writer safety, but there are
>several problems with this patch.
>
>1) It changes ABI, so it breaks old programs
>2) What about older processors, need to detect and handle them at runtime.
>3) Why can't this just be the default behavior with correct
>   fallback to locking on older processors.
>
>Actually lock ellision in DPDK is an interesting topic in general that
>needs to be addressed.