[dpdk-dev] [PATCH v3 0/4] fix creation of duplicate lpm and hash
On 04/06/2016 01:20 PM, De Lara Guarch, Pablo wrote: >> On 04/06/2016 12:32 PM, De Lara Guarch, Pablo wrote: >>> >>> I wonder if you should include something in release notes. >>> We are fixing the API, so I guess we don't need to follow the deprecation >> process, but at least a note in the documentation? >> >> Good idea, I'll send a v4 with the deprecation notice. > > Well, not sure if this needs a deprecation notice. > I mean, it is an API fix: yes, this is changing what the function returns > in a particular situation (when the hash/lpm already exists) , > but it was going against the API documentation, so a deprecation notice > should not be necessary. > (just my opinion, I could be quite wrong here :P). > > I was thinking more on adding a note in Resolved issues. Yes, agree, it's a bug fix. Another argument to not follow the API change process is that the initial behavior was to return EEXIST, but it was changed by this commit: http://dpdk.org/browse/dpdk/commit/?id=916e4f4f4e By the way, the "Fixes:" line was not referencing this commit in the v3, I'll also change that in v4. Thanks, Olivier
[dpdk-dev] [PATCH v3 0/4] fix creation of duplicate lpm and hash
On 04/06/2016 12:32 PM, De Lara Guarch, Pablo wrote: > > I wonder if you should include something in release notes. > We are fixing the API, so I guess we don't need to follow the deprecation > process, but at least a note in the documentation? Good idea, I'll send a v4 with the deprecation notice. > Apart from that, > > Series-acked-by: Pablo de Lara Thank you for the review. Olivier
[dpdk-dev] [PATCH v3 0/4] fix creation of duplicate lpm and hash
> -Original Message- > From: Olivier Matz [mailto:olivier.matz at 6wind.com] > Sent: Wednesday, April 06, 2016 12:15 PM > To: De Lara Guarch, Pablo; dev at dpdk.org > Cc: Richardson, Bruce > Subject: Re: [dpdk-dev] [PATCH v3 0/4] fix creation of duplicate lpm and hash > > > > On 04/06/2016 12:32 PM, De Lara Guarch, Pablo wrote: > > > > I wonder if you should include something in release notes. > > We are fixing the API, so I guess we don't need to follow the deprecation > process, but at least a note in the documentation? > > Good idea, I'll send a v4 with the deprecation notice. Well, not sure if this needs a deprecation notice. I mean, it is an API fix: yes, this is changing what the function returns in a particular situation (when the hash/lpm already exists) , but it was going against the API documentation, so a deprecation notice should not be necessary. (just my opinion, I could be quite wrong here :P). I was thinking more on adding a note in Resolved issues. Thanks, Pablo > > > Apart from that, > > > > Series-acked-by: Pablo de Lara > > Thank you for the review. > > Olivier
[dpdk-dev] [PATCH v3 0/4] fix creation of duplicate lpm and hash
> -Original Message- > From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com] > Sent: Tuesday, April 05, 2016 4:52 PM > To: Richardson, Bruce; De Lara Guarch, Pablo > Cc: dev at dpdk.org; Olivier Matz > Subject: Re: [dpdk-dev] [PATCH v3 0/4] fix creation of duplicate lpm and hash > > 2016-04-05 13:53, Olivier Matz: > > Seen while trying to fix the func_reentrancy autotest. The > > series addresses several issues: > > > > 1/ Hash and lpm return a pointer to an existing object if the user requests > the > >creation with an already existing name. This look dangerous: when an > object > >is returned, the user does not know if it should be freed or not. > > > > 2/ There is a race condition in cuckoo_hash as the lock is not held in > >rte_hash_create(). We could find some cases where NULL is returned > when the > >object already exists (ex: when rte_ring_create() fails). > > > > 3/ There is a race condition func_reentrancy that can fail even if the > > tested > >API behaves correctly. > > Pablo, Bruce, > What do you think of these fixes for 16.04? I was reviewing them yesterday, but couldn't finish in time. I will do now.
[dpdk-dev] [PATCH v3 0/4] fix creation of duplicate lpm and hash
2016-04-05 13:53, Olivier Matz: > Seen while trying to fix the func_reentrancy autotest. The > series addresses several issues: > > 1/ Hash and lpm return a pointer to an existing object if the user requests > the >creation with an already existing name. This look dangerous: when an object >is returned, the user does not know if it should be freed or not. > > 2/ There is a race condition in cuckoo_hash as the lock is not held in >rte_hash_create(). We could find some cases where NULL is returned when the >object already exists (ex: when rte_ring_create() fails). > > 3/ There is a race condition func_reentrancy that can fail even if the tested >API behaves correctly. Pablo, Bruce, What do you think of these fixes for 16.04?
[dpdk-dev] [PATCH v3 0/4] fix creation of duplicate lpm and hash
Seen while trying to fix the func_reentrancy autotest. The series addresses several issues: 1/ Hash and lpm return a pointer to an existing object if the user requests the creation with an already existing name. This look dangerous: when an object is returned, the user does not know if it should be freed or not. 2/ There is a race condition in cuckoo_hash as the lock is not held in rte_hash_create(). We could find some cases where NULL is returned when the object already exists (ex: when rte_ring_create() fails). 3/ There is a race condition func_reentrancy that can fail even if the tested API behaves correctly. RFC -> v1: - split the patch in 4 patches - on error, set rte_errno to EEXIST when relevant - fix locking in cuckoo_hash creation v1 -> v2: - fix compilation issue in cuckoo hash - update the hash test to conform to the new behavior - rework locking modification in cuckoo_hash - passed autotests: hash, lpm, lpm6, func_reentrancy v2 -> v3: - rebase against head - add "Fixes:" in commit messages - properly set lpm or hash pointers to NULL on error before returning Olivier Matz (4): lpm: allocation of an existing object should fail hash: allocation of an existing object should fail hash: keep the list locked at creation autotest: fix func reentrancy app/test/test_func_reentrancy.c | 31 +++-- app/test/test_hash.c | 65 +-- app/test/test_lpm6.c | 2 +- lib/librte_hash/rte_cuckoo_hash.c | 72 +++ lib/librte_hash/rte_fbk_hash.c| 5 ++- lib/librte_lpm/rte_lpm.c | 10 -- lib/librte_lpm/rte_lpm6.c | 5 ++- 7 files changed, 103 insertions(+), 87 deletions(-) -- 2.1.4
