| Message ID | 1416411391-19060-1-git-send-email-yan.songming@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 19 November 2014 10:36, Yan Songming <yan.songming@linaro.org> wrote: > New API implementing odp_shm_free to match the odp_shm_reserve. > > Signed-off-by: Yan Songming <yan.songming@linaro.org> > --- > v3 change the return value of odp_shm_free. > v2 fix the problem which Maxim found. > --- > .../linux-generic/include/api/odp_shared_memory.h | 2 +- > platform/linux-generic/odp_shared_memory.c | 43 > +++++++++++++++++++--- > 2 files changed, 38 insertions(+), 7 deletions(-) > > diff --git a/platform/linux-generic/include/api/odp_shared_memory.h > b/platform/linux-generic/include/api/odp_shared_memory.h > index ff6f9a9..d42e272 100644 > --- a/platform/linux-generic/include/api/odp_shared_memory.h > +++ b/platform/linux-generic/include/api/odp_shared_memory.h > @@ -81,7 +81,7 @@ odp_shm_t odp_shm_reserve(const char *name, uint64_t > size, uint64_t align, > * @param[in] shm Block handle > * > From the implementation below we can be more precise about the behaviour of this API > * @retval 0 for success > @retval 0 if the handle is already free @retval 0 if the handle free succeeds > - * @retval 1 on failure > + * @retval -1 on failure > @retval -1 on failure to free the handle > */ > int odp_shm_free(odp_shm_t shm); > > diff --git a/platform/linux-generic/odp_shared_memory.c > b/platform/linux-generic/odp_shared_memory.c > index 24a5d60..bc0ab55 100644 > --- a/platform/linux-generic/odp_shared_memory.c > +++ b/platform/linux-generic/odp_shared_memory.c > @@ -114,6 +114,43 @@ static int find_block(const char *name, uint32_t > *index) > return 0; > } > > +int odp_shm_free(odp_shm_t shm) > +{ > + uint32_t i; > + int ret; > + odp_shm_block_t *shm_block; > + uint64_t alloc_size; > + > + i = from_handle(shm); > + if (odp_shm_tbl->block[i].addr == NULL) { > Would it be better to follow the slightly better habit of reversing the constant with the variable NULL == odp_shm_tbl->block[i].addr > + /* free block */ > Is this comment adding value ? > + ODP_DBG("odp_shm_free: Free block\n"); > + return 0; > + } > + > + odp_spinlock_lock(&odp_shm_tbl->lock); > + shm_block = &odp_shm_tbl->block[i]; > + > + alloc_size = shm_block->size + shm_block->align; > + ret = munmap(shm_block->addr, alloc_size); > + if (0 != ret) { > + ODP_DBG("odp_shm_free: munmap failed\n"); > + odp_spinlock_unlock(&odp_shm_tbl->lock); > + return -1; > + } > + > + if (shm_block->flags & ODP_SHM_PROC) { > + ret = shm_unlink(shm_block->name); > + if (0 != ret) { > + ODP_DBG("odp_shm_free: shm_unlink failed\n"); > + odp_spinlock_unlock(&odp_shm_tbl->lock); > + return -1; > + } > + } > + memset(&odp_shm_tbl->block[i], 0, sizeof(odp_shm_block_t)); > + odp_spinlock_unlock(&odp_shm_tbl->lock); > + return 0; > +} > > odp_shm_t odp_shm_reserve(const char *name, uint64_t size, uint64_t align, > uint32_t flags) > @@ -221,12 +258,6 @@ odp_shm_t odp_shm_reserve(const char *name, uint64_t > size, uint64_t align, > return block->hdl; > } > > -int odp_shm_free(odp_shm_t shm ODP_UNUSED) > -{ > - ODP_UNIMPLEMENTED(); > - return 0; > -} > - > odp_shm_t odp_shm_lookup(const char *name) > { > uint32_t i; > -- > 1.8.3.1 > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp >
Change subject to something like this: platform: odp_shared_memory.c: implement odp_shm_free Nit: This should really go as two patches with the commit messages below. 1. api: doxygen: fix retval for odp_shm_free 2. platform: odp_shared_memory.c: implement func odp_shm_free On 2014-11-19 23:36, Yan Songming wrote: > New API implementing odp_shm_free to match the odp_shm_reserve. > > Signed-off-by: Yan Songming <yan.songming@linaro.org> > --- > v3 change the return value of odp_shm_free. > v2 fix the problem which Maxim found. > --- > .../linux-generic/include/api/odp_shared_memory.h | 2 +- > platform/linux-generic/odp_shared_memory.c | 43 +++++++++++++++++++--- > 2 files changed, 38 insertions(+), 7 deletions(-) > > diff --git a/platform/linux-generic/include/api/odp_shared_memory.h b/platform/linux-generic/include/api/odp_shared_memory.h > index ff6f9a9..d42e272 100644 > --- a/platform/linux-generic/include/api/odp_shared_memory.h > +++ b/platform/linux-generic/include/api/odp_shared_memory.h > @@ -81,7 +81,7 @@ odp_shm_t odp_shm_reserve(const char *name, uint64_t size, uint64_t align, > * @param[in] shm Block handle > * > * @retval 0 for success > - * @retval 1 on failure > + * @retval -1 on failure > */ > int odp_shm_free(odp_shm_t shm); > > diff --git a/platform/linux-generic/odp_shared_memory.c b/platform/linux-generic/odp_shared_memory.c > index 24a5d60..bc0ab55 100644 > --- a/platform/linux-generic/odp_shared_memory.c > +++ b/platform/linux-generic/odp_shared_memory.c > @@ -114,6 +114,43 @@ static int find_block(const char *name, uint32_t *index) > return 0; > } > > +int odp_shm_free(odp_shm_t shm) > +{ > + uint32_t i; > + int ret; > + odp_shm_block_t *shm_block; > + uint64_t alloc_size; > + > + i = from_handle(shm); > + if (odp_shm_tbl->block[i].addr == NULL) { > + /* free block */ > + ODP_DBG("odp_shm_free: Free block\n"); > + return 0; > + } > + > + odp_spinlock_lock(&odp_shm_tbl->lock); > + shm_block = &odp_shm_tbl->block[i]; > + > + alloc_size = shm_block->size + shm_block->align; > + ret = munmap(shm_block->addr, alloc_size); > + if (0 != ret) { > + ODP_DBG("odp_shm_free: munmap failed\n"); > + odp_spinlock_unlock(&odp_shm_tbl->lock); > + return -1; > + } > + > + if (shm_block->flags & ODP_SHM_PROC) { Don't we need to ODP_SHM_SW_ONLY as well? Cheers, Anders
On 11/19/2014 11:21 PM, Mike Holmes wrote: > > > On 19 November 2014 10:36, Yan Songming <yan.songming@linaro.org > <mailto:yan.songming@linaro.org>> wrote: > > New API implementing odp_shm_free to match the odp_shm_reserve. > > Signed-off-by: Yan Songming <yan.songming@linaro.org > <mailto:yan.songming@linaro.org>> > --- > v3 change the return value of odp_shm_free. > v2 fix the problem which Maxim found. > --- > .../linux-generic/include/api/odp_shared_memory.h | 2 +- > platform/linux-generic/odp_shared_memory.c | 43 > +++++++++++++++++++--- > 2 files changed, 38 insertions(+), 7 deletions(-) > > diff --git > a/platform/linux-generic/include/api/odp_shared_memory.h > b/platform/linux-generic/include/api/odp_shared_memory.h > index ff6f9a9..d42e272 100644 > --- a/platform/linux-generic/include/api/odp_shared_memory.h > +++ b/platform/linux-generic/include/api/odp_shared_memory.h > @@ -81,7 +81,7 @@ odp_shm_t odp_shm_reserve(const char *name, > uint64_t size, uint64_t align, > * @param[in] shm Block handle > * > > > From the implementation below we can be more precise about the > behaviour of this API > > * @retval 0 for success > > > @retval 0 if the handle is already free > @retval 0 if the handle free succeeds > > - * @retval 1 on failure > + * @retval -1 on failure > > > @retval -1 on failure to free the handle > > */ > int odp_shm_free(odp_shm_t shm); > > diff --git a/platform/linux-generic/odp_shared_memory.c > b/platform/linux-generic/odp_shared_memory.c > index 24a5d60..bc0ab55 100644 > --- a/platform/linux-generic/odp_shared_memory.c > +++ b/platform/linux-generic/odp_shared_memory.c > @@ -114,6 +114,43 @@ static int find_block(const char *name, > uint32_t *index) > return 0; > } > > +int odp_shm_free(odp_shm_t shm) > +{ > + uint32_t i; > + int ret; > + odp_shm_block_t *shm_block; > + uint64_t alloc_size; > + > + i = from_handle(shm); > + if (odp_shm_tbl->block[i].addr == NULL) { > > > Would it be better to follow the slightly better habit of reversing > the constant with the variable > > NULL == odp_shm_tbl->block[i].addr why is that better? I always write tested variable first. Maxim. > + /* free block */ > > > Is this comment adding value ? > > + ODP_DBG("odp_shm_free: Free block\n"); > + return 0; > + } > + > + odp_spinlock_lock(&odp_shm_tbl->lock); > + shm_block = &odp_shm_tbl->block[i]; > + > + alloc_size = shm_block->size + shm_block->align; > + ret = munmap(shm_block->addr, alloc_size); > + if (0 != ret) { > + ODP_DBG("odp_shm_free: munmap failed\n"); > + odp_spinlock_unlock(&odp_shm_tbl->lock); > + return -1; > + } > + > + if (shm_block->flags & ODP_SHM_PROC) { > + ret = shm_unlink(shm_block->name); > + if (0 != ret) { > + ODP_DBG("odp_shm_free: shm_unlink failed\n"); > + odp_spinlock_unlock(&odp_shm_tbl->lock); > + return -1; > + } > + } > + memset(&odp_shm_tbl->block[i], 0, sizeof(odp_shm_block_t)); > + odp_spinlock_unlock(&odp_shm_tbl->lock); > + return 0; > +} > > odp_shm_t odp_shm_reserve(const char *name, uint64_t size, > uint64_t align, > uint32_t flags) > @@ -221,12 +258,6 @@ odp_shm_t odp_shm_reserve(const char *name, > uint64_t size, uint64_t align, > return block->hdl; > } > > -int odp_shm_free(odp_shm_t shm ODP_UNUSED) > -{ > - ODP_UNIMPLEMENTED(); > - return 0; > -} > - > odp_shm_t odp_shm_lookup(const char *name) > { > uint32_t i; > -- > 1.8.3.1 > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> > http://lists.linaro.org/mailman/listinfo/lng-odp > > > > > -- > *Mike Holmes* > Linaro Sr Technical Manager > LNG - ODP > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp
On 19 November 2014 16:13, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > On 11/19/2014 11:21 PM, Mike Holmes wrote: > >> >> >> On 19 November 2014 10:36, Yan Songming <yan.songming@linaro.org <mailto: >> yan.songming@linaro.org>> wrote: >> >> New API implementing odp_shm_free to match the odp_shm_reserve. >> >> Signed-off-by: Yan Songming <yan.songming@linaro.org >> <mailto:yan.songming@linaro.org>> >> >> --- >> v3 change the return value of odp_shm_free. >> v2 fix the problem which Maxim found. >> --- >> .../linux-generic/include/api/odp_shared_memory.h | 2 +- >> platform/linux-generic/odp_shared_memory.c | 43 >> +++++++++++++++++++--- >> 2 files changed, 38 insertions(+), 7 deletions(-) >> >> diff --git >> a/platform/linux-generic/include/api/odp_shared_memory.h >> b/platform/linux-generic/include/api/odp_shared_memory.h >> index ff6f9a9..d42e272 100644 >> --- a/platform/linux-generic/include/api/odp_shared_memory.h >> +++ b/platform/linux-generic/include/api/odp_shared_memory.h >> @@ -81,7 +81,7 @@ odp_shm_t odp_shm_reserve(const char *name, >> uint64_t size, uint64_t align, >> * @param[in] shm Block handle >> * >> >> >> From the implementation below we can be more precise about the behaviour >> of this API >> >> * @retval 0 for success >> >> >> @retval 0 if the handle is already free >> @retval 0 if the handle free succeeds >> >> - * @retval 1 on failure >> + * @retval -1 on failure >> >> >> @retval -1 on failure to free the handle >> >> */ >> int odp_shm_free(odp_shm_t shm); >> >> diff --git a/platform/linux-generic/odp_shared_memory.c >> b/platform/linux-generic/odp_shared_memory.c >> index 24a5d60..bc0ab55 100644 >> --- a/platform/linux-generic/odp_shared_memory.c >> +++ b/platform/linux-generic/odp_shared_memory.c >> @@ -114,6 +114,43 @@ static int find_block(const char *name, >> uint32_t *index) >> return 0; >> } >> >> +int odp_shm_free(odp_shm_t shm) >> +{ >> + uint32_t i; >> + int ret; >> + odp_shm_block_t *shm_block; >> + uint64_t alloc_size; >> + >> + i = from_handle(shm); >> + if (odp_shm_tbl->block[i].addr == NULL) { >> >> >> Would it be better to follow the slightly better habit of reversing the >> constant with the variable >> >> NULL == odp_shm_tbl->block[i].addr >> > > why is that better? I always write tested variable first. > http://www.drpaulcarter.com/cs/common-c-errors.php#2.2 Basically the reverse habit if applied uniformly removes the chance of the linked silly mistake, if we adopt that for ODP we will make it harder to suffer from the issue. But we would have to do it uniformly for best effect so that during a review it catches your eye. > > Maxim. > > + /* free block */ >> >> >> Is this comment adding value ? >> >> + ODP_DBG("odp_shm_free: Free block\n"); >> + return 0; >> + } >> + >> + odp_spinlock_lock(&odp_shm_tbl->lock); >> + shm_block = &odp_shm_tbl->block[i]; >> + >> + alloc_size = shm_block->size + shm_block->align; >> + ret = munmap(shm_block->addr, alloc_size); >> + if (0 != ret) { >> + ODP_DBG("odp_shm_free: munmap failed\n"); >> + odp_spinlock_unlock(&odp_shm_tbl->lock); >> + return -1; >> + } >> + >> + if (shm_block->flags & ODP_SHM_PROC) { >> + ret = shm_unlink(shm_block->name); >> + if (0 != ret) { >> + ODP_DBG("odp_shm_free: shm_unlink failed\n"); >> + odp_spinlock_unlock(&odp_shm_tbl->lock); >> + return -1; >> + } >> + } >> + memset(&odp_shm_tbl->block[i], 0, sizeof(odp_shm_block_t)); >> + odp_spinlock_unlock(&odp_shm_tbl->lock); >> + return 0; >> +} >> >> odp_shm_t odp_shm_reserve(const char *name, uint64_t size, >> uint64_t align, >> uint32_t flags) >> @@ -221,12 +258,6 @@ odp_shm_t odp_shm_reserve(const char *name, >> uint64_t size, uint64_t align, >> return block->hdl; >> } >> >> -int odp_shm_free(odp_shm_t shm ODP_UNUSED) >> -{ >> - ODP_UNIMPLEMENTED(); >> - return 0; >> -} >> - >> odp_shm_t odp_shm_lookup(const char *name) >> { >> uint32_t i; >> -- >> 1.8.3.1 >> >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> >> http://lists.linaro.org/mailman/listinfo/lng-odp >> >> >> >> >> -- >> *Mike Holmes* >> Linaro Sr Technical Manager >> LNG - ODP >> >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org >> http://lists.linaro.org/mailman/listinfo/lng-odp >> > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp >
+1 on Mike’s suggestion. Have debugged one too many accidental assignments that were supposed to be tests over the years. I personally would like to see brackets around single lines on conditionals as well. From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of Mike Holmes Sent: Wednesday, November 19, 2014 4:23 PM To: Maxim Uvarov Cc: lng-odp Subject: Re: [lng-odp] [PATCH v3] add implement for odp_shm_free On 19 November 2014 16:13, Maxim Uvarov <maxim.uvarov@linaro.org<mailto:maxim.uvarov@linaro.org>> wrote: On 11/19/2014 11:21 PM, Mike Holmes wrote: On 19 November 2014 10:36, Yan Songming <yan.songming@linaro.org<mailto:yan.songming@linaro.org> <mailto:yan.songming@linaro.org<mailto:yan.songming@linaro.org>>> wrote: New API implementing odp_shm_free to match the odp_shm_reserve. Signed-off-by: Yan Songming <yan.songming@linaro.org<mailto:yan.songming@linaro.org> <mailto:yan.songming@linaro.org<mailto:yan.songming@linaro.org>>> --- v3 change the return value of odp_shm_free. v2 fix the problem which Maxim found. --- .../linux-generic/include/api/odp_shared_memory.h | 2 +- platform/linux-generic/odp_shared_memory.c | 43 +++++++++++++++++++--- 2 files changed, 38 insertions(+), 7 deletions(-) diff --git a/platform/linux-generic/include/api/odp_shared_memory.h b/platform/linux-generic/include/api/odp_shared_memory.h index ff6f9a9..d42e272 100644 --- a/platform/linux-generic/include/api/odp_shared_memory.h +++ b/platform/linux-generic/include/api/odp_shared_memory.h @@ -81,7 +81,7 @@ odp_shm_t odp_shm_reserve(const char *name, uint64_t size, uint64_t align, * @param[in] shm Block handle * From the implementation below we can be more precise about the behaviour of this API * @retval 0 for success @retval 0 if the handle is already free @retval 0 if the handle free succeeds - * @retval 1 on failure + * @retval -1 on failure @retval -1 on failure to free the handle */ int odp_shm_free(odp_shm_t shm); diff --git a/platform/linux-generic/odp_shared_memory.c b/platform/linux-generic/odp_shared_memory.c index 24a5d60..bc0ab55 100644 --- a/platform/linux-generic/odp_shared_memory.c +++ b/platform/linux-generic/odp_shared_memory.c @@ -114,6 +114,43 @@ static int find_block(const char *name, uint32_t *index) return 0; } +int odp_shm_free(odp_shm_t shm) +{ + uint32_t i; + int ret; + odp_shm_block_t *shm_block; + uint64_t alloc_size; + + i = from_handle(shm); + if (odp_shm_tbl->block[i].addr == NULL) { Would it be better to follow the slightly better habit of reversing the constant with the variable NULL == odp_shm_tbl->block[i].addr why is that better? I always write tested variable first. http://www.drpaulcarter.com/cs/common-c-errors.php#2.2 Basically the reverse habit if applied uniformly removes the chance of the linked silly mistake, if we adopt that for ODP we will make it harder to suffer from the issue. But we would have to do it uniformly for best effect so that during a review it catches your eye. Maxim. + /* free block */ Is this comment adding value ? + ODP_DBG("odp_shm_free: Free block\n"); + return 0; + } + + odp_spinlock_lock(&odp_shm_tbl->lock); + shm_block = &odp_shm_tbl->block[i]; + + alloc_size = shm_block->size + shm_block->align; + ret = munmap(shm_block->addr, alloc_size); + if (0 != ret) { + ODP_DBG("odp_shm_free: munmap failed\n"); + odp_spinlock_unlock(&odp_shm_tbl->lock); + return -1; + } + + if (shm_block->flags & ODP_SHM_PROC) { + ret = shm_unlink(shm_block->name); + if (0 != ret) { + ODP_DBG("odp_shm_free: shm_unlink failed\n"); + odp_spinlock_unlock(&odp_shm_tbl->lock); + return -1; + } + } + memset(&odp_shm_tbl->block[i], 0, sizeof(odp_shm_block_t)); + odp_spinlock_unlock(&odp_shm_tbl->lock); + return 0; +} odp_shm_t odp_shm_reserve(const char *name, uint64_t size, uint64_t align, uint32_t flags) @@ -221,12 +258,6 @@ odp_shm_t odp_shm_reserve(const char *name, uint64_t size, uint64_t align, return block->hdl; } -int odp_shm_free(odp_shm_t shm ODP_UNUSED) -{ - ODP_UNIMPLEMENTED(); - return 0; -} - odp_shm_t odp_shm_lookup(const char *name) { uint32_t i; -- 1.8.3.1 _______________________________________________ lng-odp mailing list lng-odp@lists.linaro.org<mailto:lng-odp@lists.linaro.org> <mailto:lng-odp@lists.linaro.org<mailto:lng-odp@lists.linaro.org>> http://lists.linaro.org/mailman/listinfo/lng-odp -- *Mike Holmes* Linaro Sr Technical Manager LNG - ODP _______________________________________________ lng-odp mailing list lng-odp@lists.linaro.org<mailto:lng-odp@lists.linaro.org> http://lists.linaro.org/mailman/listinfo/lng-odp _______________________________________________ lng-odp mailing list lng-odp@lists.linaro.org<mailto:lng-odp@lists.linaro.org> http://lists.linaro.org/mailman/listinfo/lng-odp -- Mike Holmes Linaro Sr Technical Manager LNG - ODP
On 19 November 2014 17:13, Robbie King (robking) <robking@cisco.com> wrote: > +1 on Mike’s suggestion. Have debugged one too many accidental > assignments > > that were supposed to be tests over the years. I personally would like to > see > > brackets around single lines on conditionals as well. > I have suffered from this also, subsequent work adds code to the "if" but forgets to add braces We would need to update the checkpatch rule to allow it however > > > *From:* lng-odp-bounces@lists.linaro.org [mailto: > lng-odp-bounces@lists.linaro.org] *On Behalf Of *Mike Holmes > *Sent:* Wednesday, November 19, 2014 4:23 PM > *To:* Maxim Uvarov > *Cc:* lng-odp > *Subject:* Re: [lng-odp] [PATCH v3] add implement for odp_shm_free > > > > > > > > On 19 November 2014 16:13, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > > On 11/19/2014 11:21 PM, Mike Holmes wrote: > > > > On 19 November 2014 10:36, Yan Songming <yan.songming@linaro.org <mailto: > yan.songming@linaro.org>> wrote: > > New API implementing odp_shm_free to match the odp_shm_reserve. > > Signed-off-by: Yan Songming <yan.songming@linaro.org > <mailto:yan.songming@linaro.org>> > > > --- > v3 change the return value of odp_shm_free. > v2 fix the problem which Maxim found. > --- > .../linux-generic/include/api/odp_shared_memory.h | 2 +- > platform/linux-generic/odp_shared_memory.c | 43 > +++++++++++++++++++--- > 2 files changed, 38 insertions(+), 7 deletions(-) > > diff --git > a/platform/linux-generic/include/api/odp_shared_memory.h > b/platform/linux-generic/include/api/odp_shared_memory.h > index ff6f9a9..d42e272 100644 > --- a/platform/linux-generic/include/api/odp_shared_memory.h > +++ b/platform/linux-generic/include/api/odp_shared_memory.h > @@ -81,7 +81,7 @@ odp_shm_t odp_shm_reserve(const char *name, > uint64_t size, uint64_t align, > * @param[in] shm Block handle > * > > > From the implementation below we can be more precise about the behaviour > of this API > > * @retval 0 for success > > > @retval 0 if the handle is already free > @retval 0 if the handle free succeeds > > - * @retval 1 on failure > + * @retval -1 on failure > > > @retval -1 on failure to free the handle > > */ > int odp_shm_free(odp_shm_t shm); > > diff --git a/platform/linux-generic/odp_shared_memory.c > b/platform/linux-generic/odp_shared_memory.c > index 24a5d60..bc0ab55 100644 > --- a/platform/linux-generic/odp_shared_memory.c > +++ b/platform/linux-generic/odp_shared_memory.c > @@ -114,6 +114,43 @@ static int find_block(const char *name, > uint32_t *index) > return 0; > } > > +int odp_shm_free(odp_shm_t shm) > +{ > + uint32_t i; > + int ret; > + odp_shm_block_t *shm_block; > + uint64_t alloc_size; > + > + i = from_handle(shm); > + if (odp_shm_tbl->block[i].addr == NULL) { > > > Would it be better to follow the slightly better habit of reversing the > constant with the variable > > NULL == odp_shm_tbl->block[i].addr > > > why is that better? I always write tested variable first. > > > > http://www.drpaulcarter.com/cs/common-c-errors.php#2.2 > > > > Basically the reverse habit if applied uniformly removes the chance of the > linked silly mistake, if we adopt that for ODP we will make it harder to > suffer from the issue. But we would have to do it uniformly for best effect > so that during a review it catches your eye. > > > > > > > > > Maxim. > > + /* free block */ > > > Is this comment adding value ? > > + ODP_DBG("odp_shm_free: Free block\n"); > + return 0; > + } > + > + odp_spinlock_lock(&odp_shm_tbl->lock); > + shm_block = &odp_shm_tbl->block[i]; > + > + alloc_size = shm_block->size + shm_block->align; > + ret = munmap(shm_block->addr, alloc_size); > + if (0 != ret) { > + ODP_DBG("odp_shm_free: munmap failed\n"); > + odp_spinlock_unlock(&odp_shm_tbl->lock); > + return -1; > + } > + > + if (shm_block->flags & ODP_SHM_PROC) { > + ret = shm_unlink(shm_block->name); > + if (0 != ret) { > + ODP_DBG("odp_shm_free: shm_unlink failed\n"); > + odp_spinlock_unlock(&odp_shm_tbl->lock); > + return -1; > + } > + } > + memset(&odp_shm_tbl->block[i], 0, sizeof(odp_shm_block_t)); > + odp_spinlock_unlock(&odp_shm_tbl->lock); > + return 0; > +} > > odp_shm_t odp_shm_reserve(const char *name, uint64_t size, > uint64_t align, > uint32_t flags) > @@ -221,12 +258,6 @@ odp_shm_t odp_shm_reserve(const char *name, > uint64_t size, uint64_t align, > return block->hdl; > } > > -int odp_shm_free(odp_shm_t shm ODP_UNUSED) > -{ > - ODP_UNIMPLEMENTED(); > - return 0; > -} > - > odp_shm_t odp_shm_lookup(const char *name) > { > uint32_t i; > -- > 1.8.3.1 > > > _______________________________________________ > lng-odp mailing list > > lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> > http://lists.linaro.org/mailman/listinfo/lng-odp > > > > > -- > *Mike Holmes* > Linaro Sr Technical Manager > LNG - ODP > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp > > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp > > > > > > -- > > *Mike Holmes* > > Linaro Sr Technical Manager > > LNG - ODP >
On 2014-11-19 16:23, Mike Holmes wrote: > On 19 November 2014 16:13, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > > > On 11/19/2014 11:21 PM, Mike Holmes wrote: > > > >> > >> > >> On 19 November 2014 10:36, Yan Songming <yan.songming@linaro.org <mailto: > >> yan.songming@linaro.org>> wrote: > >> > >> New API implementing odp_shm_free to match the odp_shm_reserve. > >> [...] > >> + uint64_t alloc_size; > >> + > >> + i = from_handle(shm); > >> + if (odp_shm_tbl->block[i].addr == NULL) { > >> > >> > >> Would it be better to follow the slightly better habit of reversing the > >> constant with the variable > >> > >> NULL == odp_shm_tbl->block[i].addr > >> > > > > why is that better? I always write tested variable first. > > > > http://www.drpaulcarter.com/cs/common-c-errors.php#2.2 > > Basically the reverse habit if applied uniformly removes the chance of the > linked silly mistake, if we adopt that for ODP we will make it harder to > suffer from the issue. But we would have to do it uniformly for best effect > so that during a review it catches your eye. +1 for Mike's suggestion. Cheers, Anders
Anders,
-> + if (shm_block->flags & ODP_SHM_PROC) {
->Don't we need to ODP_SHM_SW_ONLY as well?
In odp_shm_reserve if flag is ODP_SHM_PROC, we will use shm_open to create a file,
but when then flag is ODP_SHM_SW_ONLY, we don't do that.
So when we use odp_shm_free, we just use shm_unlink when the flag is ODP_SHM_PROC
yan.songming@linaro.org
From: Anders Roxell
Date: 2014-11-20 04:33
To: Yan Songming
CC: lng-odp
Subject: Re: [lng-odp] [PATCH v3] add implement for odp_shm_free
Change subject to something like this:
platform: odp_shared_memory.c: implement odp_shm_free
Nit: This should really go as two patches with the commit messages below.
1. api: doxygen: fix retval for odp_shm_free
2. platform: odp_shared_memory.c: implement func odp_shm_free
On 2014-11-19 23:36, Yan Songming wrote:
> New API implementing odp_shm_free to match the odp_shm_reserve.
>
> Signed-off-by: Yan Songming <yan.songming@linaro.org>
> ---
> v3 change the return value of odp_shm_free.
> v2 fix the problem which Maxim found.
> ---
> .../linux-generic/include/api/odp_shared_memory.h | 2 +-
> platform/linux-generic/odp_shared_memory.c | 43 +++++++++++++++++++---
> 2 files changed, 38 insertions(+), 7 deletions(-)
>
> diff --git a/platform/linux-generic/include/api/odp_shared_memory.h b/platform/linux-generic/include/api/odp_shared_memory.h
> index ff6f9a9..d42e272 100644
> --- a/platform/linux-generic/include/api/odp_shared_memory.h
> +++ b/platform/linux-generic/include/api/odp_shared_memory.h
> @@ -81,7 +81,7 @@ odp_shm_t odp_shm_reserve(const char *name, uint64_t size, uint64_t align,
> * @param[in] shm Block handle
> *
> * @retval 0 for success
> - * @retval 1 on failure
> + * @retval -1 on failure
> */
> int odp_shm_free(odp_shm_t shm);
>
> diff --git a/platform/linux-generic/odp_shared_memory.c b/platform/linux-generic/odp_shared_memory.c
> index 24a5d60..bc0ab55 100644
> --- a/platform/linux-generic/odp_shared_memory.c
> +++ b/platform/linux-generic/odp_shared_memory.c
> @@ -114,6 +114,43 @@ static int find_block(const char *name, uint32_t *index)
> return 0;
> }
>
> +int odp_shm_free(odp_shm_t shm)
> +{
> + uint32_t i;
> + int ret;
> + odp_shm_block_t *shm_block;
> + uint64_t alloc_size;
> +
> + i = from_handle(shm);
> + if (odp_shm_tbl->block[i].addr == NULL) {
> + /* free block */
> + ODP_DBG("odp_shm_free: Free block\n");
> + return 0;
> + }
> +
> + odp_spinlock_lock(&odp_shm_tbl->lock);
> + shm_block = &odp_shm_tbl->block[i];
> +
> + alloc_size = shm_block->size + shm_block->align;
> + ret = munmap(shm_block->addr, alloc_size);
> + if (0 != ret) {
> + ODP_DBG("odp_shm_free: munmap failed\n");
> + odp_spinlock_unlock(&odp_shm_tbl->lock);
> + return -1;
> + }
> +
> + if (shm_block->flags & ODP_SHM_PROC) {
Don't we need to ODP_SHM_SW_ONLY as well?
Cheers,
Anders
->Nit: This should really go as two patches with the commit messages below. ->1. api: doxygen: fix retval for odp_shm_free ->2. platform: odp_shared_memory.c: implement func odp_shm_free I think make it together is better , because the change of the reval for odp_shm_free is need to according to the implement of the func odp_shm_free. Make one patch can make sure this change will take effect together. yan.songming@linaro.org From: Anders Roxell Date: 2014-11-20 04:33 To: Yan Songming CC: lng-odp Subject: Re: [lng-odp] [PATCH v3] add implement for odp_shm_free Change subject to something like this: platform: odp_shared_memory.c: implement odp_shm_free Nit: This should really go as two patches with the commit messages below. 1. api: doxygen: fix retval for odp_shm_free 2. platform: odp_shared_memory.c: implement func odp_shm_free On 2014-11-19 23:36, Yan Songming wrote: > New API implementing odp_shm_free to match the odp_shm_reserve. > > Signed-off-by: Yan Songming <yan.songming@linaro.org> > --- > v3 change the return value of odp_shm_free. > v2 fix the problem which Maxim found. > --- > .../linux-generic/include/api/odp_shared_memory.h | 2 +- > platform/linux-generic/odp_shared_memory.c | 43 +++++++++++++++++++--- > 2 files changed, 38 insertions(+), 7 deletions(-) > > diff --git a/platform/linux-generic/include/api/odp_shared_memory.h b/platform/linux-generic/include/api/odp_shared_memory.h > index ff6f9a9..d42e272 100644 > --- a/platform/linux-generic/include/api/odp_shared_memory.h > +++ b/platform/linux-generic/include/api/odp_shared_memory.h > @@ -81,7 +81,7 @@ odp_shm_t odp_shm_reserve(const char *name, uint64_t size, uint64_t align, > * @param[in] shm Block handle > * > * @retval 0 for success > - * @retval 1 on failure > + * @retval -1 on failure > */ > int odp_shm_free(odp_shm_t shm); > > diff --git a/platform/linux-generic/odp_shared_memory.c b/platform/linux-generic/odp_shared_memory.c > index 24a5d60..bc0ab55 100644 > --- a/platform/linux-generic/odp_shared_memory.c > +++ b/platform/linux-generic/odp_shared_memory.c > @@ -114,6 +114,43 @@ static int find_block(const char *name, uint32_t *index) > return 0; > } > > +int odp_shm_free(odp_shm_t shm) > +{ > + uint32_t i; > + int ret; > + odp_shm_block_t *shm_block; > + uint64_t alloc_size; > + > + i = from_handle(shm); > + if (odp_shm_tbl->block[i].addr == NULL) { > + /* free block */ > + ODP_DBG("odp_shm_free: Free block\n"); > + return 0; > + } > + > + odp_spinlock_lock(&odp_shm_tbl->lock); > + shm_block = &odp_shm_tbl->block[i]; > + > + alloc_size = shm_block->size + shm_block->align; > + ret = munmap(shm_block->addr, alloc_size); > + if (0 != ret) { > + ODP_DBG("odp_shm_free: munmap failed\n"); > + odp_spinlock_unlock(&odp_shm_tbl->lock); > + return -1; > + } > + > + if (shm_block->flags & ODP_SHM_PROC) { Don't we need to ODP_SHM_SW_ONLY as well? Cheers, Anders
On 20 November 2014 07:46, yan.songming@linaro.org <yan.songming@linaro.org> wrote: > ->Nit: This should really go as two patches with the commit messages below. > ->1. api: doxygen: fix retval for odp_shm_free > ->2. platform: odp_shared_memory.c: implement func odp_shm_free > > I think make it together is better , because the change of the reval for > odp_shm_free > is need to according to the implement of the func odp_shm_free. > Make one patch can make sure this change will take effect together. ok > > ________________________________ > yan.songming@linaro.org > > > From: Anders Roxell > Date: 2014-11-20 04:33 > To: Yan Songming > CC: lng-odp > Subject: Re: [lng-odp] [PATCH v3] add implement for odp_shm_free > Change subject to something like this: > platform: odp_shared_memory.c: implement odp_shm_free > > Nit: This should really go as two patches with the commit messages below. > 1. api: doxygen: fix retval for odp_shm_free > 2. platform: odp_shared_memory.c: implement func odp_shm_free > > On 2014-11-19 23:36, Yan Songming wrote: >> New API implementing odp_shm_free to match the odp_shm_reserve. >> >> Signed-off-by: Yan Songming <yan.songming@linaro.org> >> --- >> v3 change the return value of odp_shm_free. >> v2 fix the problem which Maxim found. >> --- >> .../linux-generic/include/api/odp_shared_memory.h | 2 +- >> platform/linux-generic/odp_shared_memory.c | 43 >> +++++++++++++++++++--- >> 2 files changed, 38 insertions(+), 7 deletions(-) >> >> diff --git a/platform/linux-generic/include/api/odp_shared_memory.h >> b/platform/linux-generic/include/api/odp_shared_memory.h >> index ff6f9a9..d42e272 100644 >> --- a/platform/linux-generic/include/api/odp_shared_memory.h >> +++ b/platform/linux-generic/include/api/odp_shared_memory.h >> @@ -81,7 +81,7 @@ odp_shm_t odp_shm_reserve(const char *name, uint64_t >> size, uint64_t align, >> * @param[in] shm Block handle >> * >> * @retval 0 for success >> - * @retval 1 on failure >> + * @retval -1 on failure >> */ >> int odp_shm_free(odp_shm_t shm); >> >> diff --git a/platform/linux-generic/odp_shared_memory.c >> b/platform/linux-generic/odp_shared_memory.c >> index 24a5d60..bc0ab55 100644 >> --- a/platform/linux-generic/odp_shared_memory.c >> +++ b/platform/linux-generic/odp_shared_memory.c >> @@ -114,6 +114,43 @@ static int find_block(const char *name, uint32_t >> *index) >> return 0; >> } >> >> +int odp_shm_free(odp_shm_t shm) >> +{ >> + uint32_t i; >> + int ret; >> + odp_shm_block_t *shm_block; >> + uint64_t alloc_size; >> + >> + i = from_handle(shm); >> + if (odp_shm_tbl->block[i].addr == NULL) { >> + /* free block */ >> + ODP_DBG("odp_shm_free: Free block\n"); >> + return 0; >> + } >> + >> + odp_spinlock_lock(&odp_shm_tbl->lock); >> + shm_block = &odp_shm_tbl->block[i]; >> + >> + alloc_size = shm_block->size + shm_block->align; >> + ret = munmap(shm_block->addr, alloc_size); >> + if (0 != ret) { >> + ODP_DBG("odp_shm_free: munmap failed\n"); >> + odp_spinlock_unlock(&odp_shm_tbl->lock); >> + return -1; >> + } >> + >> + if (shm_block->flags & ODP_SHM_PROC) { > > Don't we need to ODP_SHM_SW_ONLY as well? > > Cheers, > Anders
On 11/20/2014 01:49 AM, Anders Roxell wrote: > On 2014-11-19 16:23, Mike Holmes wrote: >> On 19 November 2014 16:13, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: >> >>> On 11/19/2014 11:21 PM, Mike Holmes wrote: >>> >>>> >>>> On 19 November 2014 10:36, Yan Songming <yan.songming@linaro.org <mailto: >>>> yan.songming@linaro.org>> wrote: >>>> >>>> New API implementing odp_shm_free to match the odp_shm_reserve. >>>> > [...] > >>>> + uint64_t alloc_size; >>>> + >>>> + i = from_handle(shm); >>>> + if (odp_shm_tbl->block[i].addr == NULL) { >>>> >>>> >>>> Would it be better to follow the slightly better habit of reversing the >>>> constant with the variable >>>> >>>> NULL == odp_shm_tbl->block[i].addr >>>> >>> why is that better? I always write tested variable first. >>> >> http://www.drpaulcarter.com/cs/common-c-errors.php#2.2 >> >> Basically the reverse habit if applied uniformly removes the chance of the >> linked silly mistake, if we adopt that for ODP we will make it harder to >> suffer from the issue. But we would have to do it uniformly for best effect >> so that during a review it catches your eye. > +1 for Mike's suggestion. Thanks, that is reasonable. Sometime I really did = instead of == in the past. Maxim. > > Cheers, > Anders
On 11/20/2014 10:15 AM, Maxim Uvarov wrote: > On 11/20/2014 01:49 AM, Anders Roxell wrote: >> On 2014-11-19 16:23, Mike Holmes wrote: >>> On 19 November 2014 16:13, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: >>> >>>> On 11/19/2014 11:21 PM, Mike Holmes wrote: >>>> >>>>> >>>>> On 19 November 2014 10:36, Yan Songming <yan.songming@linaro.org >>>>> <mailto: >>>>> yan.songming@linaro.org>> wrote: >>>>> >>>>> New API implementing odp_shm_free to match the odp_shm_reserve. >>>>> >> [...] >> >>>>> + uint64_t alloc_size; >>>>> + >>>>> + i = from_handle(shm); >>>>> + if (odp_shm_tbl->block[i].addr == NULL) { >>>>> >>>>> >>>>> Would it be better to follow the slightly better habit of reversing >>>>> the >>>>> constant with the variable >>>>> >>>>> NULL == odp_shm_tbl->block[i].addr >>>>> >>>> why is that better? I always write tested variable first. >>>> >>> http://www.drpaulcarter.com/cs/common-c-errors.php#2.2 >>> >>> Basically the reverse habit if applied uniformly removes the chance >>> of the >>> linked silly mistake, if we adopt that for ODP we will make it harder to >>> suffer from the issue. But we would have to do it uniformly for best >>> effect >>> so that during a review it catches your eye. >> +1 for Mike's suggestion. > > Thanks, that is reasonable. Sometime I really did = instead of == in the > past. That's a matter of taste. Modern compiler will give you a warning if in the code above = is used instead of ==. Personally I prefer a direct form, because it is logical and easier to read.
In GCC you'll get a warning if you write:
if (x = 6) ...
and, at least in the case of ODP, where we compile with warnings treated as
errors, the code will fail compilation. To intentionally do an assignment
within an if you need an extra set of parentheses:
if ((x = 6) == somevar) ...
which confirms that you intended = vs. ==.
Currently, checkpatch flags as an error single statements that have braces:
if (x) {
....single statement
}
and insists on :
if (x)
...single statement
Personally I see nothing wrong with either convention. The former has the
advantage that if you want to add another line to the test at some point in
the future you don't have to also add the braces.
On Thu, Nov 20, 2014 at 4:21 AM, Taras Kondratiuk <
taras.kondratiuk@linaro.org> wrote:
> On 11/20/2014 10:15 AM, Maxim Uvarov wrote:
> > On 11/20/2014 01:49 AM, Anders Roxell wrote:
> >> On 2014-11-19 16:23, Mike Holmes wrote:
> >>> On 19 November 2014 16:13, Maxim Uvarov <maxim.uvarov@linaro.org>
> wrote:
> >>>
> >>>> On 11/19/2014 11:21 PM, Mike Holmes wrote:
> >>>>
> >>>>>
> >>>>> On 19 November 2014 10:36, Yan Songming <yan.songming@linaro.org
> >>>>> <mailto:
> >>>>> yan.songming@linaro.org>> wrote:
> >>>>>
> >>>>> New API implementing odp_shm_free to match the odp_shm_reserve.
> >>>>>
> >> [...]
> >>
> >>>>> + uint64_t alloc_size;
> >>>>> +
> >>>>> + i = from_handle(shm);
> >>>>> + if (odp_shm_tbl->block[i].addr == NULL) {
> >>>>>
> >>>>>
> >>>>> Would it be better to follow the slightly better habit of reversing
> >>>>> the
> >>>>> constant with the variable
> >>>>>
> >>>>> NULL == odp_shm_tbl->block[i].addr
> >>>>>
> >>>> why is that better? I always write tested variable first.
> >>>>
> >>> http://www.drpaulcarter.com/cs/common-c-errors.php#2.2
> >>>
> >>> Basically the reverse habit if applied uniformly removes the chance
> >>> of the
> >>> linked silly mistake, if we adopt that for ODP we will make it harder
> to
> >>> suffer from the issue. But we would have to do it uniformly for best
> >>> effect
> >>> so that during a review it catches your eye.
> >> +1 for Mike's suggestion.
> >
> > Thanks, that is reasonable. Sometime I really did = instead of == in the
> > past.
>
> That's a matter of taste. Modern compiler will give you a warning if in
> the code above = is used instead of ==.
> Personally I prefer a direct form, because it is logical and easier to
> read.
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp
>
diff --git a/platform/linux-generic/include/api/odp_shared_memory.h b/platform/linux-generic/include/api/odp_shared_memory.h index ff6f9a9..d42e272 100644 --- a/platform/linux-generic/include/api/odp_shared_memory.h +++ b/platform/linux-generic/include/api/odp_shared_memory.h @@ -81,7 +81,7 @@ odp_shm_t odp_shm_reserve(const char *name, uint64_t size, uint64_t align, * @param[in] shm Block handle * * @retval 0 for success - * @retval 1 on failure + * @retval -1 on failure */ int odp_shm_free(odp_shm_t shm); diff --git a/platform/linux-generic/odp_shared_memory.c b/platform/linux-generic/odp_shared_memory.c index 24a5d60..bc0ab55 100644 --- a/platform/linux-generic/odp_shared_memory.c +++ b/platform/linux-generic/odp_shared_memory.c @@ -114,6 +114,43 @@ static int find_block(const char *name, uint32_t *index) return 0; } +int odp_shm_free(odp_shm_t shm) +{ + uint32_t i; + int ret; + odp_shm_block_t *shm_block; + uint64_t alloc_size; + + i = from_handle(shm); + if (odp_shm_tbl->block[i].addr == NULL) { + /* free block */ + ODP_DBG("odp_shm_free: Free block\n"); + return 0; + } + + odp_spinlock_lock(&odp_shm_tbl->lock); + shm_block = &odp_shm_tbl->block[i]; + + alloc_size = shm_block->size + shm_block->align; + ret = munmap(shm_block->addr, alloc_size); + if (0 != ret) { + ODP_DBG("odp_shm_free: munmap failed\n"); + odp_spinlock_unlock(&odp_shm_tbl->lock); + return -1; + } + + if (shm_block->flags & ODP_SHM_PROC) { + ret = shm_unlink(shm_block->name); + if (0 != ret) { + ODP_DBG("odp_shm_free: shm_unlink failed\n"); + odp_spinlock_unlock(&odp_shm_tbl->lock); + return -1; + } + } + memset(&odp_shm_tbl->block[i], 0, sizeof(odp_shm_block_t)); + odp_spinlock_unlock(&odp_shm_tbl->lock); + return 0; +} odp_shm_t odp_shm_reserve(const char *name, uint64_t size, uint64_t align, uint32_t flags) @@ -221,12 +258,6 @@ odp_shm_t odp_shm_reserve(const char *name, uint64_t size, uint64_t align, return block->hdl; } -int odp_shm_free(odp_shm_t shm ODP_UNUSED) -{ - ODP_UNIMPLEMENTED(); - return 0; -} - odp_shm_t odp_shm_lookup(const char *name) { uint32_t i;
New API implementing odp_shm_free to match the odp_shm_reserve. Signed-off-by: Yan Songming <yan.songming@linaro.org> --- v3 change the return value of odp_shm_free. v2 fix the problem which Maxim found. --- .../linux-generic/include/api/odp_shared_memory.h | 2 +- platform/linux-generic/odp_shared_memory.c | 43 +++++++++++++++++++--- 2 files changed, 38 insertions(+), 7 deletions(-)