Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1633444 > unrolled thread

[PATCH v4] irqchip/gicv3-its: Avoid memory over allocation for ITEs

Back to article view | Back to linux.kernel

Contents

  [PATCH v4] irqchip/gicv3-its: Avoid memory over allocation for ITEs Shanker Donthineni <shankerd@codeaurora.org> - 2017-04-30 16:40 +0200
    Re: [PATCH v4] irqchip/gicv3-its: Avoid memory over allocation for ITEs Marc Zyngier <marc.zyngier@arm.com> - 2017-05-02 18:30 +0200
      Re: [PATCH v4] irqchip/gicv3-its: Avoid memory over allocation for  ITEs Shanker Donthineni <shankerd@codeaurora.org> - 2017-05-06 00:10 +0200
        Re: [PATCH v4] irqchip/gicv3-its: Avoid memory over allocation for ITEs Marc Zyngier <marc.zyngier@arm.com> - 2017-05-06 13:30 +0200

#1633444 — [PATCH v4] irqchip/gicv3-its: Avoid memory over allocation for ITEs

We are always allocating extra 255Bytes of memory to handle ITE
physical address alignment requirement. The kmalloc() satisfies
the ITE alignment since the ITS driver is requesting a minimum
size of ITS_ITT_ALIGN bytes.

Let's try to allocate the exact amount of memory that is required
for ITEs to avoid wastage.

Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
---
Changes:
v2: removed 'Change-Id: Ia8084189833f2081ff13c392deb5070c46a64038' from commit.
v3: changed from IITE to ITE.
v3: removed fallback since kmalloc() guarantees the right alignment.

 drivers/irqchip/irq-gic-v3-its.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 45ea1933..72e56f03 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -261,7 +261,6 @@ static struct its_collection *its_build_mapd_cmd(struct its_cmd_block *cmd,
 	u8 size = ilog2(desc->its_mapd_cmd.dev->nr_ites);
 
 	itt_addr = virt_to_phys(desc->its_mapd_cmd.dev->itt);
-	itt_addr = ALIGN(itt_addr, ITS_ITT_ALIGN);
 
 	its_encode_cmd(cmd, GITS_CMD_MAPD);
 	its_encode_devid(cmd, desc->its_mapd_cmd.dev->device_id);
@@ -1329,13 +1328,14 @@ static struct its_device *its_create_device(struct its_node *its, u32 dev_id,
 	 */
 	nr_ites = max(2UL, roundup_pow_of_two(nvecs));
 	sz = nr_ites * its->ite_size;
-	sz = max(sz, ITS_ITT_ALIGN) + ITS_ITT_ALIGN - 1;
+	sz = max(sz, ITS_ITT_ALIGN);
 	itt = kzalloc(sz, GFP_KERNEL);
 	lpi_map = its_lpi_alloc_chunks(nvecs, &lpi_base, &nr_lpis);
 	if (lpi_map)
 		col_map = kzalloc(sizeof(*col_map) * nr_lpis, GFP_KERNEL);
 
-	if (!dev || !itt || !lpi_map || !col_map) {
+	if (!dev || !itt || !lpi_map || !col_map ||
+	    !IS_ALIGNED(virt_to_phys(itt), ITS_ITT_ALIGN)) {
 		kfree(dev);
 		kfree(itt);
 		kfree(lpi_map);
-- 
Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[toc] | [next] | [standalone]

#1634513

On Sun, Apr 30 2017 at  3:36:15 pm BST, Shanker Donthineni <shankerd@codeaurora.org> wrote:
> We are always allocating extra 255Bytes of memory to handle ITE
> physical address alignment requirement. The kmalloc() satisfies
> the ITE alignment since the ITS driver is requesting a minimum
> size of ITS_ITT_ALIGN bytes.
>
> Let's try to allocate the exact amount of memory that is required
> for ITEs to avoid wastage.
>
> Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
> ---
> Changes:
> v2: removed 'Change-Id: Ia8084189833f2081ff13c392deb5070c46a64038' from commit.
> v3: changed from IITE to ITE.
> v3: removed fallback since kmalloc() guarantees the right alignment.
>
>  drivers/irqchip/irq-gic-v3-its.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index 45ea1933..72e56f03 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -261,7 +261,6 @@ static struct its_collection *its_build_mapd_cmd(struct its_cmd_block *cmd,
>  	u8 size = ilog2(desc->its_mapd_cmd.dev->nr_ites);
>  
>  	itt_addr = virt_to_phys(desc->its_mapd_cmd.dev->itt);
> -	itt_addr = ALIGN(itt_addr, ITS_ITT_ALIGN);
>  
>  	its_encode_cmd(cmd, GITS_CMD_MAPD);
>  	its_encode_devid(cmd, desc->its_mapd_cmd.dev->device_id);
> @@ -1329,13 +1328,14 @@ static struct its_device *its_create_device(struct its_node *its, u32 dev_id,
>  	 */
>  	nr_ites = max(2UL, roundup_pow_of_two(nvecs));
>  	sz = nr_ites * its->ite_size;
> -	sz = max(sz, ITS_ITT_ALIGN) + ITS_ITT_ALIGN - 1;
> +	sz = max(sz, ITS_ITT_ALIGN);
>  	itt = kzalloc(sz, GFP_KERNEL);
>  	lpi_map = its_lpi_alloc_chunks(nvecs, &lpi_base, &nr_lpis);
>  	if (lpi_map)
>  		col_map = kzalloc(sizeof(*col_map) * nr_lpis, GFP_KERNEL);
>  
> -	if (!dev || !itt || !lpi_map || !col_map) {
> +	if (!dev || !itt || !lpi_map || !col_map ||
> +	    !IS_ALIGNED(virt_to_phys(itt), ITS_ITT_ALIGN)) {
>  		kfree(dev);
>  		kfree(itt);
>  		kfree(lpi_map);

I'm confused. Either the alignment is guaranteed (and you should
document why it is so), or it is not, and we need to handle the
non-alignment (instead of failing).

Thanks,

	M.
-- 
Jazz is not dead, it just smell funny.

[toc] | [prev] | [next] | [standalone]

#1636761 — Re: [PATCH v4] irqchip/gicv3-its: Avoid memory over allocation for ITEs

Hi Marc,


On 05/02/2017 11:16 AM, Marc Zyngier wrote:
> On Sun, Apr 30 2017 at  3:36:15 pm BST, Shanker Donthineni <shankerd@codeaurora.org> wrote:
>> We are always allocating extra 255Bytes of memory to handle ITE
>> physical address alignment requirement. The kmalloc() satisfies
>> the ITE alignment since the ITS driver is requesting a minimum
>> size of ITS_ITT_ALIGN bytes.
>>
>> Let's try to allocate the exact amount of memory that is required
>> for ITEs to avoid wastage.
>>
>> Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
>> ---
>> Changes:
>> v2: removed 'Change-Id: Ia8084189833f2081ff13c392deb5070c46a64038' from commit.
>> v3: changed from IITE to ITE.
>> v3: removed fallback since kmalloc() guarantees the right alignment.
>>
>>  drivers/irqchip/irq-gic-v3-its.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
>> index 45ea1933..72e56f03 100644
>> --- a/drivers/irqchip/irq-gic-v3-its.c
>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>> @@ -261,7 +261,6 @@ static struct its_collection *its_build_mapd_cmd(struct its_cmd_block *cmd,
>>  	u8 size = ilog2(desc->its_mapd_cmd.dev->nr_ites);
>>  
>>  	itt_addr = virt_to_phys(desc->its_mapd_cmd.dev->itt);
>> -	itt_addr = ALIGN(itt_addr, ITS_ITT_ALIGN);
>>  
>>  	its_encode_cmd(cmd, GITS_CMD_MAPD);
>>  	its_encode_devid(cmd, desc->its_mapd_cmd.dev->device_id);
>> @@ -1329,13 +1328,14 @@ static struct its_device *its_create_device(struct its_node *its, u32 dev_id,
>>  	 */
>>  	nr_ites = max(2UL, roundup_pow_of_two(nvecs));
>>  	sz = nr_ites * its->ite_size;
>> -	sz = max(sz, ITS_ITT_ALIGN) + ITS_ITT_ALIGN - 1;
>> +	sz = max(sz, ITS_ITT_ALIGN);
>>  	itt = kzalloc(sz, GFP_KERNEL);
>>  	lpi_map = its_lpi_alloc_chunks(nvecs, &lpi_base, &nr_lpis);
>>  	if (lpi_map)
>>  		col_map = kzalloc(sizeof(*col_map) * nr_lpis, GFP_KERNEL);
>>  
>> -	if (!dev || !itt || !lpi_map || !col_map) {
>> +	if (!dev || !itt || !lpi_map || !col_map ||
>> +	    !IS_ALIGNED(virt_to_phys(itt), ITS_ITT_ALIGN)) {
>>  		kfree(dev);
>>  		kfree(itt);
>>  		kfree(lpi_map);
> I'm confused. Either the alignment is guaranteed (and you should
> document why it is so), or it is not, and we need to handle the
> non-alignment (instead of failing).

Sorry for confusion, alignment is guaranteed by kmalloc(), added a check for readability purpose only can be removed.
 
> Thanks,
>
> 	M.

-- 
Shanker Donthineni
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[toc] | [prev] | [next] | [standalone]

#1636873

On Fri, May 05 2017 at 11:04:22 pm BST, Shanker Donthineni <shankerd@codeaurora.org> wrote:
> Hi Marc,
>
>
> On 05/02/2017 11:16 AM, Marc Zyngier wrote:
>> On Sun, Apr 30 2017 at  3:36:15 pm BST, Shanker Donthineni <shankerd@codeaurora.org> wrote:
>>> We are always allocating extra 255Bytes of memory to handle ITE
>>> physical address alignment requirement. The kmalloc() satisfies
>>> the ITE alignment since the ITS driver is requesting a minimum
>>> size of ITS_ITT_ALIGN bytes.
>>>
>>> Let's try to allocate the exact amount of memory that is required
>>> for ITEs to avoid wastage.
>>>
>>> Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
>>> ---
>>> Changes:
>>> v2: removed 'Change-Id: Ia8084189833f2081ff13c392deb5070c46a64038' from commit.
>>> v3: changed from IITE to ITE.
>>> v3: removed fallback since kmalloc() guarantees the right alignment.
>>>
>>>  drivers/irqchip/irq-gic-v3-its.c | 6 +++---
>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
>>> index 45ea1933..72e56f03 100644
>>> --- a/drivers/irqchip/irq-gic-v3-its.c
>>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>>> @@ -261,7 +261,6 @@ static struct its_collection *its_build_mapd_cmd(struct its_cmd_block *cmd,
>>>  	u8 size = ilog2(desc->its_mapd_cmd.dev->nr_ites);
>>>  
>>>  	itt_addr = virt_to_phys(desc->its_mapd_cmd.dev->itt);
>>> -	itt_addr = ALIGN(itt_addr, ITS_ITT_ALIGN);
>>>  
>>>  	its_encode_cmd(cmd, GITS_CMD_MAPD);
>>>  	its_encode_devid(cmd, desc->its_mapd_cmd.dev->device_id);
>>> @@ -1329,13 +1328,14 @@ static struct its_device *its_create_device(struct its_node *its, u32 dev_id,
>>>  	 */
>>>  	nr_ites = max(2UL, roundup_pow_of_two(nvecs));
>>>  	sz = nr_ites * its->ite_size;
>>> -	sz = max(sz, ITS_ITT_ALIGN) + ITS_ITT_ALIGN - 1;
>>> +	sz = max(sz, ITS_ITT_ALIGN);
>>>  	itt = kzalloc(sz, GFP_KERNEL);
>>>  	lpi_map = its_lpi_alloc_chunks(nvecs, &lpi_base, &nr_lpis);
>>>  	if (lpi_map)
>>>  		col_map = kzalloc(sizeof(*col_map) * nr_lpis, GFP_KERNEL);
>>>  
>>> -	if (!dev || !itt || !lpi_map || !col_map) {
>>> +	if (!dev || !itt || !lpi_map || !col_map ||
>>> +	    !IS_ALIGNED(virt_to_phys(itt), ITS_ITT_ALIGN)) {
>>>  		kfree(dev);
>>>  		kfree(itt);
>>>  		kfree(lpi_map);
>> I'm confused. Either the alignment is guaranteed (and you should
>> document why it is so), or it is not, and we need to handle the
>> non-alignment (instead of failing).
>
> Sorry for confusion, alignment is guaranteed by kmalloc(), added a
> check for readability purpose only can be removed.

My question still remains. Where exactly is that alignment guarantee
documented and enforced? I can't see anything giving that certainty.

I would expect kmalloc to give you something that is cache-line aligned,
but probably nothing more than that. Now, I'd happily be proven wrong,
but so far, all I can see is that:

- ARCH_KMALLOC_MINALIGN is defined as ARCH_DMA_MINALIGN
- ARCH_DMA_MINALIGN is defined as L1_CACHE_BYTES
- L1_CACHE_BYTES is 128 on arm64, and either 32, 64, or 128 on arm.

What am I missing?

Thanks,

	M.
-- 
Jazz is not dead, it just smell funny.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web