[dpdk-dev] ivshmem: add all memzones of mempool to metada
Commit Message
Mempool consist of multiple memzones, at least from two of them.
ivshmem assumes mempool and elements are all in same memzone.
Updating code to add all memzones when a mempool added.
Fixes: d1d914ebbc25 ("mempool: allocate in several memory chunks by
default")
Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
lib/librte_ivshmem/rte_ivshmem.c | 30 ++++++++++++++++++++++--------
1 file changed, 22 insertions(+), 8 deletions(-)
Comments
> From: Yigit, Ferruh
> Sent: Wednesday, June 1, 2016 2:18 PM
> To: dev@dpdk.org
> Cc: Burakov, Anatoly <anatoly.burakov@intel.com>; Olivier Matz
> <olivier.matz@6wind.com>; Yigit, Ferruh <ferruh.yigit@intel.com>
> Subject: [PATCH] ivshmem: add all memzones of mempool to metada
>
> Mempool consist of multiple memzones, at least from two of them.
> ivshmem assumes mempool and elements are all in same memzone.
>
> Updating code to add all memzones when a mempool added.
>
> Fixes: d1d914ebbc25 ("mempool: allocate in several memory chunks by
> default")
>
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> ---
> lib/librte_ivshmem/rte_ivshmem.c | 30 ++++++++++++++++++++++--------
> 1 file changed, 22 insertions(+), 8 deletions(-)
>
> diff --git a/lib/librte_ivshmem/rte_ivshmem.c
> b/lib/librte_ivshmem/rte_ivshmem.c
> index c8b332c..5c83920 100644
> --- a/lib/librte_ivshmem/rte_ivshmem.c
> +++ b/lib/librte_ivshmem/rte_ivshmem.c
> @@ -548,25 +548,39 @@ add_ring_to_metadata(const struct rte_ring * r,
> }
>
> static int
> -add_mempool_to_metadata(const struct rte_mempool * mp,
> - struct ivshmem_config * config)
> +add_mempool_memzone_to_metadata(const void *addr,
> + struct ivshmem_config *config)
> {
> - struct rte_memzone * mz;
> - int ret;
> + struct rte_memzone *mz;
>
> - mz = get_memzone_by_addr(mp);
> - ret = 0;
> + mz = get_memzone_by_addr(addr);
>
> if (!mz) {
> RTE_LOG(ERR, EAL, "Cannot find memzone for
> mempool!\n");
> return -1;
> }
>
> - /* mempool consists of memzone and ring */
> - ret = add_memzone_to_metadata(mz, config);
> + return add_memzone_to_metadata(mz, config);
> +}
> +
> +static int
> +add_mempool_to_metadata(const struct rte_mempool *mp,
> + struct ivshmem_config *config)
> +{
> + struct rte_mempool_memhdr *memhdr;
> + int ret;
> +
> + ret = add_mempool_memzone_to_metadata(mp, config);
> if (ret < 0)
> return -1;
>
> + STAILQ_FOREACH(memhdr, &mp->mem_list, next) {
> + ret = add_mempool_memzone_to_metadata(memhdr-
> >addr, config);
> + if (ret < 0)
> + return -1;
> + }
> +
> + /* mempool consists of memzone and ring */
> return add_ring_to_metadata(mp->ring, config);
> }
>
> --
> 2.5.5
Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
Hi Ferruh,
Thank you for fixing this issue.
On 06/01/2016 03:18 PM, Ferruh Yigit wrote:
> [PATCH] ivshmem: add all memzones of mempool to metada
Minor comment: it seems the title is truncated
> +static int
> +add_mempool_to_metadata(const struct rte_mempool *mp,
> + struct ivshmem_config *config)
> +{
> + struct rte_mempool_memhdr *memhdr;
> + int ret;
> +
> + ret = add_mempool_memzone_to_metadata(mp, config);
> if (ret < 0)
> return -1;
>
> + STAILQ_FOREACH(memhdr, &mp->mem_list, next) {
> + ret = add_mempool_memzone_to_metadata(memhdr->addr, config);
> + if (ret < 0)
> + return -1;
> + }
> +
> + /* mempool consists of memzone and ring */
> return add_ring_to_metadata(mp->ring, config);
> }
>
In case you missed it: there is a function
rte_mempool_mem_iter() that can be used to browse the
memory chunks of a mempool. It's probably less convenient
to use compared to directly browsing the list, but it
may be more resistant to api changes.
Apart from that:
Acked-by: Olivier Matz <olivier.matz@6wind.com>
Thanks
On 6/2/2016 8:04 AM, Olivier MATZ wrote:
> Hi Ferruh,
>
> Thank you for fixing this issue.
>
> On 06/01/2016 03:18 PM, Ferruh Yigit wrote:
>> [PATCH] ivshmem: add all memzones of mempool to metada
>
> Minor comment: it seems the title is truncated
>
Right, I will fix in next version of patch.
>> +static int
>> +add_mempool_to_metadata(const struct rte_mempool *mp,
>> + struct ivshmem_config *config)
>> +{
>> + struct rte_mempool_memhdr *memhdr;
>> + int ret;
>> +
>> + ret = add_mempool_memzone_to_metadata(mp, config);
>> if (ret < 0)
>> return -1;
>>
>> + STAILQ_FOREACH(memhdr, &mp->mem_list, next) {
>> + ret = add_mempool_memzone_to_metadata(memhdr->addr, config);
>> + if (ret < 0)
>> + return -1;
>> + }
>> +
>> + /* mempool consists of memzone and ring */
>> return add_ring_to_metadata(mp->ring, config);
>> }
>>
>
> In case you missed it: there is a function
> rte_mempool_mem_iter() that can be used to browse the
> memory chunks of a mempool. It's probably less convenient
> to use compared to directly browsing the list, but it
> may be more resistant to api changes.
I wasn't aware rte_mempool_mem_iter(), I will update the patch to use this.
>
> Apart from that:
> Acked-by: Olivier Matz <olivier.matz@6wind.com>
>
> Thanks
>
Thanks,
ferruh
On 6/3/2016 12:05 PM, Ferruh Yigit wrote:
> On 6/2/2016 8:04 AM, Olivier MATZ wrote:
>> Hi Ferruh,
>>
>> Thank you for fixing this issue.
>>
>> On 06/01/2016 03:18 PM, Ferruh Yigit wrote:
>>> [PATCH] ivshmem: add all memzones of mempool to metada
>>
>> Minor comment: it seems the title is truncated
>>
>
> Right, I will fix in next version of patch.
>
>>> +static int
>>> +add_mempool_to_metadata(const struct rte_mempool *mp,
>>> + struct ivshmem_config *config)
>>> +{
>>> + struct rte_mempool_memhdr *memhdr;
>>> + int ret;
>>> +
>>> + ret = add_mempool_memzone_to_metadata(mp, config);
>>> if (ret < 0)
>>> return -1;
>>>
>>> + STAILQ_FOREACH(memhdr, &mp->mem_list, next) {
>>> + ret = add_mempool_memzone_to_metadata(memhdr->addr, config);
>>> + if (ret < 0)
>>> + return -1;
>>> + }
>>> +
>>> + /* mempool consists of memzone and ring */
>>> return add_ring_to_metadata(mp->ring, config);
>>> }
>>>
>>
>> In case you missed it: there is a function
>> rte_mempool_mem_iter() that can be used to browse the
>> memory chunks of a mempool. It's probably less convenient
>> to use compared to directly browsing the list, but it
>> may be more resistant to api changes.
>
> I wasn't aware rte_mempool_mem_iter(), I will update the patch to use this.
>
Although I prefer using the rte_mempool_mem_iter(); it is not
straightforward because of the const qualifier of mp, and
rte_mempool_mem_iter() doesn't accepts the const type, and casting const
to non-const causes a compile warning:
_"error: cast discards ‘const’ qualifier from pointer target type"_
An option is duplicating mp, (or part of it mp->mem_list) but that is
ugly and can cause potentials defects in the future, another option is
to use uint64_t to do pointer conversion, even uglier :), so I don't
want to use this API for this case.
So, I will only update the patch title and send a v2.
>>
>> Apart from that:
>> Acked-by: Olivier Matz <olivier.matz@6wind.com>
>>
>> Thanks
>>
>
> Thanks,
> ferruh
>
@@ -548,25 +548,39 @@ add_ring_to_metadata(const struct rte_ring * r,
}
static int
-add_mempool_to_metadata(const struct rte_mempool * mp,
- struct ivshmem_config * config)
+add_mempool_memzone_to_metadata(const void *addr,
+ struct ivshmem_config *config)
{
- struct rte_memzone * mz;
- int ret;
+ struct rte_memzone *mz;
- mz = get_memzone_by_addr(mp);
- ret = 0;
+ mz = get_memzone_by_addr(addr);
if (!mz) {
RTE_LOG(ERR, EAL, "Cannot find memzone for mempool!\n");
return -1;
}
- /* mempool consists of memzone and ring */
- ret = add_memzone_to_metadata(mz, config);
+ return add_memzone_to_metadata(mz, config);
+}
+
+static int
+add_mempool_to_metadata(const struct rte_mempool *mp,
+ struct ivshmem_config *config)
+{
+ struct rte_mempool_memhdr *memhdr;
+ int ret;
+
+ ret = add_mempool_memzone_to_metadata(mp, config);
if (ret < 0)
return -1;
+ STAILQ_FOREACH(memhdr, &mp->mem_list, next) {
+ ret = add_mempool_memzone_to_metadata(memhdr->addr, config);
+ if (ret < 0)
+ return -1;
+ }
+
+ /* mempool consists of memzone and ring */
return add_ring_to_metadata(mp->ring, config);
}