The race condition should be avoided. That can be done easily by first doing
the shm_reserve(). If that succeeds, a pool can be allocated normally behind
the lock.
shm = shm_reserve();
if (shm == invalid)
return -1;
LOCK();
// Search a free pool and reserve it (like today)
UNLOCK();
if (pool == NULL) {
shm_free(shm);
return -1;
}
pool->ring_shm = shm;
...
-Petri
> -----Original Message-----
> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of Maxim
> Uvarov
> Sent: Monday, January 09, 2017 6:57 PM
> To: Bill Fischofer <bill.fischo...@linaro.org>
> Cc: lng-odp-forward <lng-odp@lists.linaro.org>
> Subject: Re: [lng-odp] [API-NEXT PATCHv3] linux-generic: pool: defer ring
> allocation until pool creation
>
> Petri, do you have objections for this patch for locked area? If not,
> than Bill can do v2 with more clean shm names.
>
> Maxim.
>
> On 12/16/16 17:08, Bill Fischofer wrote:
> > On Fri, Dec 16, 2016 at 7:13 AM, Maxim Uvarov <maxim.uva...@linaro.org>
> wrote:
> >> On 12/16/16 11:06, Savolainen, Petri (Nokia - FI/Espoo) wrote:
> >>>>
> >>>> @@ -172,6 +172,7 @@ static pool_t *reserve_pool(void)
> >>>> {
> >>>> int i;
> >>>> pool_t *pool;
> >>>> + char ring_name[ODP_POOL_NAME_LEN];
> >>>>
> >>>> for (i = 0; i < ODP_CONFIG_POOLS; i++) {
> >>>> pool = pool_entry(i);
> >>>> @@ -180,6 +181,17 @@ static pool_t *reserve_pool(void)
> >>>> if (pool->reserved == 0) {
> >>>> pool->reserved = 1;
> >>>> UNLOCK(&pool->lock);
> >>>> + sprintf(ring_name, "_odp_pool_ring_%d", i);
> >>
> >>
> >> shm reserve will create /tmp/odp-pid-_odp_pool_ring_%d
> >>
> >> which looks like more odp works there. I think name pool_ring_%d is
> >> better here.
> >
> > I can certainly change that and will do so in the next rev. I'd like
> > to get Petri's feedback on the reserve question first, however, so
> > that this doesn't have to be two revisions.
> >
> >>
> >> Maxim.
> >>
> >>
> >>>> + pool->ring_shm =
> >>>> + odp_shm_reserve(ring_name,
> >>>> + sizeof(pool_ring_t),
> >>>> + ODP_CACHE_LINE_SIZE, 0);
> >>>> + if (pool->ring_shm == ODP_SHM_INVALID) {
> >>>> + ODP_ERR("Unable to alloc pool ring
> %d\n", i);
> >>>> + pool->reserved = 0;
> >>>
> >>> This must be modified inside the lock, otherwise there's a race
> condition if a pool is reserved or not.
> >>>
> >>> Also, I'd like to have performance impact of the extra indirection
> verified.
> >>>
> >>> -Petri
> >>>
> >>
