On Tue, Jan 10, 2017 at 7:19 AM, Bill Fischofer
<bill.fischo...@linaro.org> wrote:
> On Tue, Jan 10, 2017 at 2:17 AM, Savolainen, Petri (Nokia - FI/Espoo)
> <petri.savolai...@nokia-bell-labs.com> wrote:
>>
>> 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
>
> There is no race condition, but I have no objection to reworking this
> as described since they are equivalent. I'll post a v2 with this and
> Maxim's changes.
>
>>
>>
>>> -----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.
Looking at this closer, the suggested rework greatly complicates the
generation of unique names for the reserved rings. I'd like to spend a
few minutes discussing this in today's ODP call and have added it to
the agenda.
>>> >
>>> >>
>>> >> 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
>>> >>>
>>> >>
>>
