[dpdk-dev] [PATCH] ivshmem: add all memzones of mempool to metada

[dpdk-dev] [PATCH] ivshmem: add all memzones of mempool to metada

[Ferruh Yigit]

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

Re: [dpdk-dev] [PATCH] ivshmem: add all memzones of mempool to metada

[Burakov, Anatoly]

> 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>

Re: [dpdk-dev] [PATCH] ivshmem: add all memzones of mempool to metada

[Olivier MATZ]

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

Re: [dpdk-dev] [PATCH] ivshmem: add all memzones of mempool to metada

[Ferruh Yigit]

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

Re: [dpdk-dev] [PATCH] ivshmem: add all memzones of mempool to metada

[Ferruh Yigit]

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
> 

[dpdk-dev] [PATCH v2] ivshmem: add all memzones of mempool to metadata

[Ferruh Yigit]

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>
Acked-by: Anatoly  Burakov <anatoly.burakov@intel.com>
Acked-by: Olivier Matz <olivier.matz@6wind.com>

---

v2:
* fix patch title metada -> metadata
---
 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

Re: [dpdk-dev] [PATCH v2] ivshmem: add all memzones of mempool to metadata

[Thomas Monjalon]

2016-06-03 17:38, Ferruh Yigit:
> 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>
> Acked-by: Anatoly  Burakov <anatoly.burakov@intel.com>
> Acked-by: Olivier Matz <olivier.matz@6wind.com>

Applied, thanks