> -----Original Message----- > From: Shen, Wei1 > Sent: Monday, May 09, 2016 5:52 PM > To: Stephen Hemminger > Cc: dev at dpdk.org; De Lara Guarch, Pablo; Maciocco, Christian; Gobriel, > Sameh > Subject: Re: [dpdk-dev] [PATCH v1] hash: add tsx support for cuckoo hash > > 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.
Agree on this. Also, if the problem is on the rte_hash (because of the change of the cache size or the addition of a new field at the end), that should not be a problem, as far as I understand, since it is modifying an internal structure (rte_hash was made internal back in v2.1). > > 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 at networkplumber.org> wrote: > > >On Fri, 6 May 2016 21:05:02 +0100 > >Shen Wei <wei1.shen at 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.
