From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <david.hunt@intel.com>
Received: from mga02.intel.com (mga02.intel.com [134.134.136.20])
by dpdk.org (Postfix) with ESMTP id 5557FCB66
for <dev@dpdk.org>; Wed, 15 Jun 2016 18:04:34 +0200 (CEST)
Received: from fmsmga002.fm.intel.com ([10.253.24.26])
by orsmga101.jf.intel.com with ESMTP; 15 Jun 2016 09:03:53 -0700
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.26,476,1459839600"; d="scan'208";a="1002582242"
Received: from dhunt5-mobl.ger.corp.intel.com (HELO [10.237.220.25])
([10.237.220.25])
by fmsmga002.fm.intel.com with ESMTP; 15 Jun 2016 09:03:46 -0700
To: Jan Viktorin <viktorin@rehivetech.com>,
Olivier MATZ <olivier.matz@6wind.com>
References: <1465919341-3209-1-git-send-email-david.hunt@intel.com>
<1465976824-83823-1-git-send-email-david.hunt@intel.com>
<20160615121358.5ef9f142@pcviktorin.fit.vutbr.cz>
<57614043.9090603@intel.com> <57614402.6020708@6wind.com>
<57614C41.1040107@intel.com> <57615D1F.40700@6wind.com>
<5761600A.7030801@intel.com> <576161C5.6060709@6wind.com>
<20160615164709.4410a382@pcviktorin.fit.vutbr.cz>
Cc: dev@dpdk.org, jerin.jacob@caviumnetworks.com, shreyansh.jain@nxp.com
From: "Hunt, David" <david.hunt@intel.com>
Message-ID: <57617C61.7060205@intel.com>
Date: Wed, 15 Jun 2016 17:03:45 +0100
User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:38.0) Gecko/20100101
Thunderbird/38.6.0
MIME-Version: 1.0
In-Reply-To: <20160615164709.4410a382@pcviktorin.fit.vutbr.cz>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit
Subject: Re: [dpdk-dev] [PATCH v12 0/3] mempool: add external mempool manager
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: patches and discussions about DPDK <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
X-List-Received-Date: Wed, 15 Jun 2016 16:04:34 -0000
On 15/6/2016 3:47 PM, Jan Viktorin wrote:
> On Wed, 15 Jun 2016 16:10:13 +0200
> Olivier MATZ <olivier.matz@6wind.com> wrote:
>
>> On 06/15/2016 04:02 PM, Hunt, David wrote:
>>>
>>> On 15/6/2016 2:50 PM, Olivier MATZ wrote:
>>>> Hi David,
>>>>
>>>> On 06/15/2016 02:38 PM, Hunt, David wrote:
>>>>>
>>>>> On 15/6/2016 1:03 PM, Olivier MATZ wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 06/15/2016 01:47 PM, Hunt, David wrote:
>>>>>>>
>>>>>>> On 15/6/2016 11:13 AM, Jan Viktorin wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> I've got one last question. Initially, I was interested in creating
>>>>>>>> my own external memory provider based on a Linux Kernel driver.
>>>>>>>> So, I've got an opened file descriptor that points to a device which
>>>>>>>> can mmap a memory regions for me.
>>>>>>>>
>>>>>>>> ...
>>>>>>>> int fd = open("/dev/uio0" ...);
>>>>>>>> ...
>>>>>>>> rte_mempool *pool = rte_mempool_create_empty(...);
>>>>>>>> rte_mempool_set_ops_byname(pool, "uio_allocator_ops");
>>>>>>>>
>>>>>>>> I am not sure how to pass the file descriptor pointer. I thought it
>>>>>>>> would
>>>>>>>> be possible by the rte_mempool_alloc but it's not... Is it possible
>>>>>>>> to solve this case?
>>>>>>>>
>>>>>>>> The allocator is device-specific.
>>>>>>>>
>>>>>>>> Regards
>>>>>>>> Jan
>>>>>>> This particular use case is not covered.
>>>>>>>
>>>>>>> We did discuss this before, and an opaque pointer was proposed, but
>>>>>>> did
>>>>>>> not make it in.
>>>>>>> http://article.gmane.org/gmane.comp.networking.dpdk.devel/39821
>>>>>>> (and following emails in that thread)
>>>>>>>
>>>>>>> So, the options for this use case are as follows:
>>>>>>> 1. Use the pool_data to pass data in to the alloc, then set the
>>>>>>> pool_data pointer before coming back from alloc. (It's a bit of a
>>>>>>> hack,
>>>>>>> but means no code change).
>>>>>>> 2. Add an extra parameter to the alloc function. The simplest way I
>>>>>>> can
>>>>>>> think of doing this is to
>>>>>>> take the *opaque passed into rte_mempool_populate_phys, and pass it on
>>>>>>> into the alloc function.
>>>>>>> This will have minimal impact on the public API,s as there is
>>>>>>> already an
>>>>>>> opaque there in the _populate_ funcs, we're just
>>>>>>> reusing it for the alloc.
>>>>>>>
>>>>>>> Do others think option 2 is OK to add this at this late stage? Even if
>>>>>>> the patch set has already been ACK'd?
>>>>>> Jan's use-case looks to be relevant.
>>>>>>
>>>>>> What about changing:
>>>>>>
>>>>>> rte_mempool_set_ops_byname(struct rte_mempool *mp, const char *name)
>>>>>>
>>>>>> into:
>>>>>>
>>>>>> rte_mempool_set_ops(struct rte_mempool *mp, const char *name,
>>>>>> void *opaque)
> Or a third function?
>
> rte_mempool_set_ops_arg(struct rte_mempool, *mp, void *arg)
I think if we tried to add another function, there would be some
opposition to that.
I think it's reasonable to add it to set_ops_byname, as we're setting
the ops and
the ops_args in the same call. We use them later in the alloc.
>
> Or isn't it really a task for a kind of rte_mempool_populate_*?
I was leaning towards that, but a different proposal was suggested. I'm
OK with
adding the *opaque to set_ops_byname
> This is a part of mempool I am not involved in yet.
>
>>>>>> ?
>>>>>>
>>>>>> The opaque pointer would be saved in mempool structure, and used
>>>>>> when the mempool is populated (calling mempool_ops_alloc).
>>>>>> The type of the structure pointed by the opaque has to be defined
>>>>>> (and documented) into each mempool_ops manager.
>>>>>>
>>>>> Yes, that was another option, which has the additional impact of
>>>>> needing an
>>>>> opaque added to the mempool struct. If we use the opaque from the
>>>>> _populate_
>>>>> function, we use it straight away in the alloc, no storage needed.
>>>>>
>>>>> Also, do you think we need to go ahead with this change, or can we add
>>>>> it later as an
>>>>> improvement?
>>>> The opaque in populate_phys() is already used for something else
>>>> (i.e. the argument for the free callback of the memory chunk).
>>>> I'm afraid it could cause confusion to have it used for 2 different
>>>> things.
>>>>
>>>> About the change, I think it could be good to have it in 16.11,
>>>> because it will probably change the API, and we should avoid to
>>>> change it each version ;)
>>>>
>>>> So I'd vote to have it in the patchset for consistency.
>>> Sure, we should avoid breaking API just after we created it. :)
>>>
>>> OK, here's a slightly different proposal.
>>>
>>> If we add a string, to the _ops_byname, yes, that will work for Jan's case.
> A string? No, I needed to pass a file descriptor or a pointer to some rte_device
> or something like that. So a void * is a way to go.
Apologies, I misread. *opaque it is.
>>> However, if we add a void*, that allow us the flexibility of passing
>>> anything we
>>> want. We can then store the void* in the mempool struct as void
>>> *pool_config,
> void *ops_context, ops_args, ops_data, ...
I think I'll go with ops_args
>>> so that when the alloc gets called, it can access whatever is stored at
>>> *pool_config
>>> and do the correct initialisation/allocation. In Jan's use case, this
>>> can simply be typecast
>>> to a string. In future cases, it can be a struct, which could including
>>> new flags.
> New flags? Does it mean an API extension?
No, I mean new flags within the opaque data, hidden from the mempool
manager.
>
>> Yep, agree. But not sure I'm seeing the difference with what I
>> proposed.
> Me neither... I think it is exactly the same :).
>
> Jan
Yes, misread on my part.
>>> I think that change is minimal enough to be low risk at this stage.
>>>
>>> Thoughts?
>> Agree. Thanks!
>>
>>
>> Olivier
>
>