diff mbox

[dpdk-dev,RFC,v2,03/17] mempool/octeontx: add callback to calculate memory size

Message ID 1516713372-10572-4-git-send-email-arybchenko@solarflare.com (mailing list archive)
State Superseded, archived
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation fail Compilation issues

Commit Message

Andrew Rybchenko Jan. 23, 2018, 1:15 p.m. UTC 
  The driver requires one and only one physically contiguous
memory chunk for all objects.

Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
---
 drivers/mempool/octeontx/rte_mempool_octeontx.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

Comments

Santosh Shukla Feb. 1, 2018, 10:01 a.m. UTC | #1 
Hi Andrew,


On Thursday 01 February 2018 11:48 AM, Jacob, Jerin wrote:
> The driver requires one and only one physically contiguous
> memory chunk for all objects.
>
> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
> ---
>   drivers/mempool/octeontx/rte_mempool_octeontx.c | 25 +++++++++++++++++++++++++
>   1 file changed, 25 insertions(+)
>
> diff --git a/drivers/mempool/octeontx/rte_mempool_octeontx.c 
> b/drivers/mempool/octeontx/rte_mempool_octeontx.c
> index d143d05..4ec5efe 100644
> --- a/drivers/mempool/octeontx/rte_mempool_octeontx.c
> +++ b/drivers/mempool/octeontx/rte_mempool_octeontx.c
> @@ -136,6 +136,30 @@ octeontx_fpavf_get_capabilities(const struct rte_mempool *mp,
>           return 0;
>   }
>
> +static ssize_t
> +octeontx_fpavf_calc_mem_size(const struct rte_mempool *mp,
> +                            uint32_t obj_num, uint32_t pg_shift,
> +                            size_t *min_chunk_size, size_t *align)
> +{
> +       ssize_t mem_size;
> +
> +       /*
> +        * Simply need space for one more object to be able to
> +        * fullfil alignment requirements.
> +        */
> +       mem_size = rte_mempool_calc_mem_size_def(mp, obj_num + 1, pg_shift,
> +      

I think, you don't need that (obj_num + 1) as because
rte_xmem_calc_int() will be checking flags for
_ALIGNED + _CAPA_PHYS_CONFIG i.e..

	mask = MEMPOOL_F_CAPA_BLK_ALIGNED_OBJECTS | MEMPOOL_F_CAPA_PHYS_CONTIG;
	if ((flags & mask) == mask)
		/* alignment need one additional object */
		elt_num += 1;

>                                           min_chunk_size, align);
> +       if (mem_size >= 0) {
> +               /*
> +                * The whole memory area containing the objects must be
> +                * physically contiguous.
> +                */
> +               *min_chunk_size = mem_size;
> +       }
> +
> +       return mem_size;
> +}
> +
>   static int
>   octeontx_fpavf_register_memory_area(const struct rte_mempool *mp,
>                                       char *vaddr, rte_iova_t paddr, size_t len)
> @@ -159,6 +183,7 @@ static struct rte_mempool_ops octeontx_fpavf_ops = {
>           .get_count = octeontx_fpavf_get_count,
>           .get_capabilities = octeontx_fpavf_get_capabilities,
>           .register_memory_area = octeontx_fpavf_register_memory_area,
> +       .calc_mem_size = octeontx_fpavf_calc_mem_size,
>   };
>
>   MEMPOOL_REGISTER_OPS(octeontx_fpavf_ops);
> -- 
> 2.7.4
>
Santosh Shukla Feb. 1, 2018, 1:40 p.m. UTC | #2 
On Thursday 01 February 2018 03:31 PM, santosh wrote:
> Hi Andrew,
>
>
> On Thursday 01 February 2018 11:48 AM, Jacob, Jerin wrote:
>> The driver requires one and only one physically contiguous
>> memory chunk for all objects.
>>
>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
>> ---
>>   drivers/mempool/octeontx/rte_mempool_octeontx.c | 25 +++++++++++++++++++++++++
>>   1 file changed, 25 insertions(+)
>>
>> diff --git a/drivers/mempool/octeontx/rte_mempool_octeontx.c 
>> b/drivers/mempool/octeontx/rte_mempool_octeontx.c
>> index d143d05..4ec5efe 100644
>> --- a/drivers/mempool/octeontx/rte_mempool_octeontx.c
>> +++ b/drivers/mempool/octeontx/rte_mempool_octeontx.c
>> @@ -136,6 +136,30 @@ octeontx_fpavf_get_capabilities(const struct rte_mempool *mp,
>>           return 0;
>>   }
>>
>> +static ssize_t
>> +octeontx_fpavf_calc_mem_size(const struct rte_mempool *mp,
>> +                            uint32_t obj_num, uint32_t pg_shift,
>> +                            size_t *min_chunk_size, size_t *align)
>> +{
>> +       ssize_t mem_size;
>> +
>> +       /*
>> +        * Simply need space for one more object to be able to
>> +        * fullfil alignment requirements.
>> +        */
>> +       mem_size = rte_mempool_calc_mem_size_def(mp, obj_num + 1, pg_shift,
>> +      
> I think, you don't need that (obj_num + 1) as because
> rte_xmem_calc_int() will be checking flags for
> _ALIGNED + _CAPA_PHYS_CONFIG i.e..
>
> 	mask = MEMPOOL_F_CAPA_BLK_ALIGNED_OBJECTS | MEMPOOL_F_CAPA_PHYS_CONTIG;
> 	if ((flags & mask) == mask)
> 		/* alignment need one additional object */
> 		elt_num += 1;

ok, You are removing above check in v2- 06/17, so ignore above comment.
I suggest to move this patch and keep it after 06/17. Or perhaps keep
common mempool changes first then followed by driver specifics changes in your
v3 series.

Thanks.
Andrew Rybchenko March 10, 2018, 3:49 p.m. UTC | #3 
Hi Santosh,

On 02/01/2018 04:40 PM, santosh wrote:
> On Thursday 01 February 2018 03:31 PM, santosh wrote:
>> Hi Andrew,
>>
>>
>> On Thursday 01 February 2018 11:48 AM, Jacob, Jerin wrote:
>>> The driver requires one and only one physically contiguous
>>> memory chunk for all objects.
>>>
>>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
>>> ---
>>>    drivers/mempool/octeontx/rte_mempool_octeontx.c | 25 +++++++++++++++++++++++++
>>>    1 file changed, 25 insertions(+)
>>>
>>> diff --git a/drivers/mempool/octeontx/rte_mempool_octeontx.c
>>> b/drivers/mempool/octeontx/rte_mempool_octeontx.c
>>> index d143d05..4ec5efe 100644
>>> --- a/drivers/mempool/octeontx/rte_mempool_octeontx.c
>>> +++ b/drivers/mempool/octeontx/rte_mempool_octeontx.c
>>> @@ -136,6 +136,30 @@ octeontx_fpavf_get_capabilities(const struct rte_mempool *mp,
>>>            return 0;
>>>    }
>>>
>>> +static ssize_t
>>> +octeontx_fpavf_calc_mem_size(const struct rte_mempool *mp,
>>> +                            uint32_t obj_num, uint32_t pg_shift,
>>> +                            size_t *min_chunk_size, size_t *align)
>>> +{
>>> +       ssize_t mem_size;
>>> +
>>> +       /*
>>> +        * Simply need space for one more object to be able to
>>> +        * fullfil alignment requirements.
>>> +        */
>>> +       mem_size = rte_mempool_calc_mem_size_def(mp, obj_num + 1, pg_shift,
>>> +
>> I think, you don't need that (obj_num + 1) as because
>> rte_xmem_calc_int() will be checking flags for
>> _ALIGNED + _CAPA_PHYS_CONFIG i.e..
>>
>> 	mask = MEMPOOL_F_CAPA_BLK_ALIGNED_OBJECTS | MEMPOOL_F_CAPA_PHYS_CONTIG;
>> 	if ((flags & mask) == mask)
>> 		/* alignment need one additional object */
>> 		elt_num += 1;
> ok, You are removing above check in v2- 06/17, so ignore above comment.
> I suggest to move this patch and keep it after 06/17. Or perhaps keep
> common mempool changes first then followed by driver specifics changes in your
> v3 series.

Finally I've decided to include these changes into the patch which
removes get_capabilities [1]. Please, take a look at suggested version.
I think it is the most transparent solution. Otherwise it is hard
to avoid the issue found by you above.

I'm sorry, I've forgot to include you in CC.

[1] https://dpdk.org/dev/patchwork/patch/35934/

Thanks,
Andrew.
Santosh Shukla March 11, 2018, 6:31 a.m. UTC | #4 
Hi Andrew,


On Saturday 10 March 2018 09:19 PM, Andrew Rybchenko wrote:
> Hi Santosh,
>
> On 02/01/2018 04:40 PM, santosh wrote:
>> On Thursday 01 February 2018 03:31 PM, santosh wrote:
>>> Hi Andrew,
>>>
>>>
>>> On Thursday 01 February 2018 11:48 AM, Jacob, Jerin wrote:
>>>> The driver requires one and only one physically contiguous
>>>> memory chunk for all objects.
>>>>
>>>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
>>>> ---
>>>>    drivers/mempool/octeontx/rte_mempool_octeontx.c | 25 +++++++++++++++++++++++++
>>>>    1 file changed, 25 insertions(+)
>>>>
>>>> diff --git a/drivers/mempool/octeontx/rte_mempool_octeontx.c
>>>> b/drivers/mempool/octeontx/rte_mempool_octeontx.c
>>>> index d143d05..4ec5efe 100644
>>>> --- a/drivers/mempool/octeontx/rte_mempool_octeontx.c
>>>> +++ b/drivers/mempool/octeontx/rte_mempool_octeontx.c
>>>> @@ -136,6 +136,30 @@ octeontx_fpavf_get_capabilities(const struct rte_mempool *mp,
>>>>            return 0;
>>>>    }
>>>>
>>>> +static ssize_t
>>>> +octeontx_fpavf_calc_mem_size(const struct rte_mempool *mp,
>>>> +                            uint32_t obj_num, uint32_t pg_shift,
>>>> +                            size_t *min_chunk_size, size_t *align)
>>>> +{
>>>> +       ssize_t mem_size;
>>>> +
>>>> +       /*
>>>> +        * Simply need space for one more object to be able to
>>>> +        * fullfil alignment requirements.
>>>> +        */
>>>> +       mem_size = rte_mempool_calc_mem_size_def(mp, obj_num + 1, pg_shift,
>>>> +
>>> I think, you don't need that (obj_num + 1) as because
>>> rte_xmem_calc_int() will be checking flags for
>>> _ALIGNED + _CAPA_PHYS_CONFIG i.e..
>>>
>>>     mask = MEMPOOL_F_CAPA_BLK_ALIGNED_OBJECTS | MEMPOOL_F_CAPA_PHYS_CONTIG;
>>>     if ((flags & mask) == mask)
>>>         /* alignment need one additional object */
>>>         elt_num += 1;
>> ok, You are removing above check in v2- 06/17, so ignore above comment.
>> I suggest to move this patch and keep it after 06/17. Or perhaps keep
>> common mempool changes first then followed by driver specifics changes in your
>> v3 series.
>
> Finally I've decided to include these changes into the patch which
> removes get_capabilities [1]. Please, take a look at suggested version.
> I think it is the most transparent solution. Otherwise it is hard
> to avoid the issue found by you above.
>
Sure. I'll review.

> I'm sorry, I've forgot to include you in CC.
>
NP,

Thanks.

> [1] https://dpdk.org/dev/patchwork/patch/35934/
>
> Thanks,
> Andrew.
diff mbox

Patch

diff --git a/drivers/mempool/octeontx/rte_mempool_octeontx.c b/drivers/mempool/octeontx/rte_mempool_octeontx.c
index d143d05..4ec5efe 100644
--- a/drivers/mempool/octeontx/rte_mempool_octeontx.c
+++ b/drivers/mempool/octeontx/rte_mempool_octeontx.c
@@ -136,6 +136,30 @@  octeontx_fpavf_get_capabilities(const struct rte_mempool *mp,
 	return 0;
 }
 
+static ssize_t
+octeontx_fpavf_calc_mem_size(const struct rte_mempool *mp,
+			     uint32_t obj_num, uint32_t pg_shift,
+			     size_t *min_chunk_size, size_t *align)
+{
+	ssize_t mem_size;
+
+	/*
+	 * Simply need space for one more object to be able to
+	 * fullfil alignment requirements.
+	 */
+	mem_size = rte_mempool_calc_mem_size_def(mp, obj_num + 1, pg_shift,
+						 min_chunk_size, align);
+	if (mem_size >= 0) {
+		/*
+		 * The whole memory area containing the objects must be
+		 * physically contiguous.
+		 */
+		*min_chunk_size = mem_size;
+	}
+
+	return mem_size;
+}
+
 static int
 octeontx_fpavf_register_memory_area(const struct rte_mempool *mp,
 				    char *vaddr, rte_iova_t paddr, size_t len)
@@ -159,6 +183,7 @@  static struct rte_mempool_ops octeontx_fpavf_ops = {
 	.get_count = octeontx_fpavf_get_count,
 	.get_capabilities = octeontx_fpavf_get_capabilities,
 	.register_memory_area = octeontx_fpavf_register_memory_area,
+	.calc_mem_size = octeontx_fpavf_calc_mem_size,
 };
 
 MEMPOOL_REGISTER_OPS(octeontx_fpavf_ops);