mbox series

[v2,0/4] hash: deprecate lock ellision and read/write concurreny flags

Message ID 20181101232522.702-1-honnappa.nagarahalli@arm.com (mailing list archive)
Headers
Series hash: deprecate lock ellision and read/write concurreny flags |

Message

Honnappa Nagarahalli Nov. 1, 2018, 11:25 p.m. UTC 
  Various configuration flags in rte_hash library result in increase of
number of test cases. Configuration flags for enabling transactional
memory use and read/write concurrency are not required. These features
should be supported by default. Please refer to [1] for more context.

This patch marks these flags for deprecation in 19.02 release and cleans
up the test cases.

[1] http://mails.dpdk.org/archives/dev/2018-October/117268.html

Honnappa Nagarahalli (4):
  hash: prepare for deprecation of flags
  hash: deprecate lock ellision and read/write concurreny flags
  test/hash: stop using lock ellision and read/write concurreny flags
  doc/hash: deprecate lock ellision and read/write concurreny flags

 doc/guides/rel_notes/deprecation.rst |   5 +
 lib/librte_hash/rte_cuckoo_hash.c    | 321 ++++++++++++++++++++++++++-
 lib/librte_hash/rte_hash.h           |  20 +-
 lib/librte_hash/rte_hash_version.map |   7 +
 test/test/test_hash_multiwriter.c    |  20 +-
 test/test/test_hash_perf.c           |  40 ++--
 test/test/test_hash_readwrite.c      |  84 ++-----
 test/test/test_hash_readwrite_lf.c   | 189 ++++------------
 8 files changed, 432 insertions(+), 254 deletions(-)

Comments

Bruce Richardson Nov. 2, 2018, 11:25 a.m. UTC | #1 
On Thu, Nov 01, 2018 at 06:25:18PM -0500, Honnappa Nagarahalli wrote:
> Various configuration flags in rte_hash library result in increase of
> number of test cases. Configuration flags for enabling transactional
> memory use and read/write concurrency are not required. These features
> should be supported by default. Please refer to [1] for more context.
> 
> This patch marks these flags for deprecation in 19.02 release and cleans
> up the test cases.
> 
> [1] http://mails.dpdk.org/archives/dev/2018-October/117268.html
> 
> Honnappa Nagarahalli (4): hash: prepare for deprecation of flags hash:
> deprecate lock ellision and read/write concurreny flags test/hash: stop
> using lock ellision and read/write concurreny flags doc/hash: deprecate
> lock ellision and read/write concurreny flags
> 
While I'd like to reduce the flags and do cleanup, I'm a little concerned
about putting this scope of changes in so late in the release. I wonder if
less drastic changes could work as well for this release, and do the
cleanup later.
For example, rather than deprecating the flags now, how about just change
the default for when no flags are set? If user has set flags, follow the
existing path - if flags is set to zero, then have the defaults be to use
RW concurrency or TSX.

/Bruce
Honnappa Nagarahalli Nov. 2, 2018, 5:38 p.m. UTC | #2 
> 
> On Thu, Nov 01, 2018 at 06:25:18PM -0500, Honnappa Nagarahalli wrote:
> > Various configuration flags in rte_hash library result in increase of
> > number of test cases. Configuration flags for enabling transactional
> > memory use and read/write concurrency are not required. These features
> > should be supported by default. Please refer to [1] for more context.
> >
> > This patch marks these flags for deprecation in 19.02 release and
> > cleans up the test cases.
> >
> > [1] http://mails.dpdk.org/archives/dev/2018-October/117268.html
> >
> > Honnappa Nagarahalli (4): hash: prepare for deprecation of flags hash:
> > deprecate lock ellision and read/write concurreny flags test/hash:
> > stop using lock ellision and read/write concurreny flags doc/hash:
> > deprecate lock ellision and read/write concurreny flags
> >
> While I'd like to reduce the flags and do cleanup, I'm a little concerned about
> putting this scope of changes in so late in the release. I wonder if less drastic
> changes could work as well for this release, and do the cleanup later.
Thank you Bruce for the review. This patch series is not fixing any user related problems, let us skip this for 18.11. It will give us time as well to think through and get this right.

> For example, rather than deprecating the flags now, how about just change
> the default for when no flags are set? If user has set flags, follow the existing
> path - if flags is set to zero, then have the defaults be to use RW concurrency
> or TSX.
This changes the behavior of the library and what the flags mean, still requires ABI change, but does not need deprecation of flags (I guess this is what you meant). However, it will not solve the problem of losing the capability to disable TSX.

> 
> /Bruce
Ferruh Yigit Dec. 20, 2018, 8:10 p.m. UTC | #3 
On 11/2/2018 5:38 PM, Honnappa Nagarahalli wrote:
>>
>> On Thu, Nov 01, 2018 at 06:25:18PM -0500, Honnappa Nagarahalli wrote:
>>> Various configuration flags in rte_hash library result in increase of
>>> number of test cases. Configuration flags for enabling transactional
>>> memory use and read/write concurrency are not required. These features
>>> should be supported by default. Please refer to [1] for more context.
>>>
>>> This patch marks these flags for deprecation in 19.02 release and
>>> cleans up the test cases.
>>>
>>> [1] http://mails.dpdk.org/archives/dev/2018-October/117268.html
>>>
>>> Honnappa Nagarahalli (4): hash: prepare for deprecation of flags hash:
>>> deprecate lock ellision and read/write concurreny flags test/hash:
>>> stop using lock ellision and read/write concurreny flags doc/hash:
>>> deprecate lock ellision and read/write concurreny flags
>>>
>> While I'd like to reduce the flags and do cleanup, I'm a little concerned about
>> putting this scope of changes in so late in the release. I wonder if less drastic
>> changes could work as well for this release, and do the cleanup later.
> Thank you Bruce for the review. This patch series is not fixing any user related problems, let us skip this for 18.11. It will give us time as well to think through and get this right.

There was no update in the scope of 19.02, I guess it will skip for 19.02 too.
Patchset updated as "Change requested" in patchwork.

> 
>> For example, rather than deprecating the flags now, how about just change
>> the default for when no flags are set? If user has set flags, follow the existing
>> path - if flags is set to zero, then have the defaults be to use RW concurrency
>> or TSX.
> This changes the behavior of the library and what the flags mean, still requires ABI change, but does not need deprecation of flags (I guess this is what you meant). However, it will not solve the problem of losing the capability to disable TSX.
> 
>>
>> /Bruce