[dpdk-dev,1/4] lpm: allocation of an existing object should fail
Commit Message
Change the API of rte_lpm*_create() functions to return NULL and set
rte_errno to EEXIST when the object name already exists.
These functions were returning a pointer to the existing object in that
case, but it is a problem as the caller did not know if the object had
to be freed or not.
Doing this change also makes the lpm API more consistent with the other
APIs (mempool, rings, ...).
Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
app/test/test_lpm6.c | 2 +-
lib/librte_lpm/rte_lpm.c | 10 ++++++++--
lib/librte_lpm/rte_lpm6.c | 5 ++++-
3 files changed, 13 insertions(+), 4 deletions(-)
Comments
On Wed, 30 Mar 2016 17:30:24 +0200
Olivier Matz <olivier.matz@6wind.com> wrote:
> diff --git a/lib/librte_lpm/rte_lpm6.c b/lib/librte_lpm/rte_lpm6.c
> index 4c44cd7..9877a30 100644
> --- a/lib/librte_lpm/rte_lpm6.c
> +++ b/lib/librte_lpm/rte_lpm6.c
> @@ -182,8 +182,11 @@ rte_lpm6_create(const char *name, int socket_id,
> if (strncmp(name, lpm->name, RTE_LPM6_NAMESIZE) == 0)
> break;
> }
> - if (te != NULL)
> + if (te != NULL) {
> + lpm = NULL;
> + rte_errno = EEXIST;
> goto exit;
> + }
>
> /* allocate tailq entry */
>
with older memzone model, objects in huge memory area were never freed.
That means when application restarts it finds the old LPM and works.
With your change it would break such an application.
Hi Stephen,
On 03/30/2016 11:46 PM, Stephen Hemminger wrote:
> On Wed, 30 Mar 2016 17:30:24 +0200
> Olivier Matz <olivier.matz@6wind.com> wrote:
>
>> diff --git a/lib/librte_lpm/rte_lpm6.c b/lib/librte_lpm/rte_lpm6.c
>> index 4c44cd7..9877a30 100644
>> --- a/lib/librte_lpm/rte_lpm6.c
>> +++ b/lib/librte_lpm/rte_lpm6.c
>> @@ -182,8 +182,11 @@ rte_lpm6_create(const char *name, int socket_id,
>> if (strncmp(name, lpm->name, RTE_LPM6_NAMESIZE) == 0)
>> break;
>> }
>> - if (te != NULL)
>> + if (te != NULL) {
>> + lpm = NULL;
>> + rte_errno = EEXIST;
>> goto exit;
>> + }
>>
>> /* allocate tailq entry */
>>
>
> with older memzone model, objects in huge memory area were never freed.
> That means when application restarts it finds the old LPM and works.
> With your change it would break such an application.
>
Could you be more precise about the use case you are
describing? Are you talking about a secondary process?
The API description of lpm and hash says since the first
release that EEXIST should be returned if a memzone with
the same name already exists:
* @return
* Handle to LPM object on success, NULL otherwise with rte_errno set
* to an appropriate values. Possible rte_errno values include:
* - E_RTE_NO_CONFIG - function could not get pointer to rte_config
structure
* - E_RTE_SECONDARY - function was called from a secondary process
instance
* - EINVAL - invalid parameter passed to function
* - ENOSPC - the maximum number of memzones has already been allocated
* - EEXIST - a memzone with the same name already exists
* - ENOMEM - no appropriate memory area found in which to create memzone
*/
struct rte_lpm *
rte_lpm_create(const char *name, int socket_id,
const struct rte_lpm_config *config);
* @return
* Pointer to hash table structure that is used in future hash table
* operations, or NULL on error, with error code set in rte_errno.
* Possible rte_errno errors include:
* - E_RTE_NO_CONFIG - function could not get pointer to rte_config
structure
* - E_RTE_SECONDARY - function was called from a secondary process
instance
* - ENOENT - missing entry
* - EINVAL - invalid parameter passed to function
* - ENOSPC - the maximum number of memzones has already been allocated
* - EEXIST - a memzone with the same name already exists
* - ENOMEM - no appropriate memory area found in which to create memzone
*/
struct rte_hash *
rte_hash_create(const struct rte_hash_parameters *params);
From my point of view, the behavior I'm fixing is more a bug
fix than an API change. But if required, I can send a deprecation
notice for 16.04 and have the fix integrated for 16.07.
On Wed, Mar 30, 2016 at 02:46:49PM -0700, Stephen Hemminger wrote:
> On Wed, 30 Mar 2016 17:30:24 +0200
> Olivier Matz <olivier.matz@6wind.com> wrote:
>
> > diff --git a/lib/librte_lpm/rte_lpm6.c b/lib/librte_lpm/rte_lpm6.c
> > index 4c44cd7..9877a30 100644
> > --- a/lib/librte_lpm/rte_lpm6.c
> > +++ b/lib/librte_lpm/rte_lpm6.c
> > @@ -182,8 +182,11 @@ rte_lpm6_create(const char *name, int socket_id,
> > if (strncmp(name, lpm->name, RTE_LPM6_NAMESIZE) == 0)
> > break;
> > }
> > - if (te != NULL)
> > + if (te != NULL) {
> > + lpm = NULL;
> > + rte_errno = EEXIST;
> > goto exit;
> > + }
> >
> > /* allocate tailq entry */
> >
>
> with older memzone model, objects in huge memory area were never freed.
> That means when application restarts it finds the old LPM and works.
> With your change it would break such an application.
Is all the memory not zeroed on initialization?
/Bruce
On 03/31/2016 09:35 AM, Olivier Matz wrote:
> On 03/30/2016 11:46 PM, Stephen Hemminger wrote:
>> with older memzone model, objects in huge memory area were never freed.
>> That means when application restarts it finds the old LPM and works.
>> With your change it would break such an application.
>>
>
> Could you be more precise about the use case you are
> describing? Are you talking about a secondary process?
>
> The API description of lpm and hash says since the first
> release that EEXIST should be returned if a memzone with
> the same name already exists:
>
> * @return
> * Handle to LPM object on success, NULL otherwise with rte_errno set
> * to an appropriate values. Possible rte_errno values include:
> * - E_RTE_NO_CONFIG - function could not get pointer to rte_config
> structure
> * - E_RTE_SECONDARY - function was called from a secondary process
> instance
> * - EINVAL - invalid parameter passed to function
> * - ENOSPC - the maximum number of memzones has already been allocated
> * - EEXIST - a memzone with the same name already exists
> * - ENOMEM - no appropriate memory area found in which to create memzone
> */
> struct rte_lpm *
> rte_lpm_create(const char *name, int socket_id,
> const struct rte_lpm_config *config);
>
> * @return
> * Pointer to hash table structure that is used in future hash table
> * operations, or NULL on error, with error code set in rte_errno.
> * Possible rte_errno errors include:
> * - E_RTE_NO_CONFIG - function could not get pointer to rte_config
> structure
> * - E_RTE_SECONDARY - function was called from a secondary process
> instance
> * - ENOENT - missing entry
> * - EINVAL - invalid parameter passed to function
> * - ENOSPC - the maximum number of memzones has already been allocated
> * - EEXIST - a memzone with the same name already exists
> * - ENOMEM - no appropriate memory area found in which to create memzone
> */
> struct rte_hash *
> rte_hash_create(const struct rte_hash_parameters *params);
>
>
> From my point of view, the behavior I'm fixing is more a bug
> fix than an API change. But if required, I can send a deprecation
> notice for 16.04 and have the fix integrated for 16.07.
>
Stephen, any comment on this please?
The problem is today some unit tests are not passing correctly.
Thanks
@@ -222,7 +222,7 @@ test1(void)
/* rte_lpm6_create: lpm name == LPM2 */
lpm3 = rte_lpm6_create("LPM1", SOCKET_ID_ANY, &config);
- TEST_LPM_ASSERT(lpm3 == lpm1);
+ TEST_LPM_ASSERT(lpm3 == NULL);
rte_lpm6_free(lpm1);
rte_lpm6_free(lpm2);
@@ -209,8 +209,11 @@ rte_lpm_create_v20(const char *name, int socket_id, int max_rules,
if (strncmp(name, lpm->name, RTE_LPM_NAMESIZE) == 0)
break;
}
- if (te != NULL)
+ if (te != NULL) {
+ lpm = NULL;
+ rte_errno = EEXIST;
goto exit;
+ }
/* allocate tailq entry */
te = rte_zmalloc("LPM_TAILQ_ENTRY", sizeof(*te), 0);
@@ -280,8 +283,11 @@ rte_lpm_create_v1604(const char *name, int socket_id,
if (strncmp(name, lpm->name, RTE_LPM_NAMESIZE) == 0)
break;
}
- if (te != NULL)
+ if (te != NULL) {
+ lpm = NULL;
+ rte_errno = EEXIST;
goto exit;
+ }
/* allocate tailq entry */
te = rte_zmalloc("LPM_TAILQ_ENTRY", sizeof(*te), 0);
@@ -182,8 +182,11 @@ rte_lpm6_create(const char *name, int socket_id,
if (strncmp(name, lpm->name, RTE_LPM6_NAMESIZE) == 0)
break;
}
- if (te != NULL)
+ if (te != NULL) {
+ lpm = NULL;
+ rte_errno = EEXIST;
goto exit;
+ }
/* allocate tailq entry */
te = rte_zmalloc("LPM6_TAILQ_ENTRY", sizeof(*te), 0);