linux-kernel - Re: [PATCH 2/5] soc/fsl/qe: qe.c: reduce static memory footprint by 1.7K

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <cb6a20d0-5ff1-8be9-5e3a-bd5c881dbae6@prevas.dk>
Date:   Wed, 1 May 2019 05:51:26 +0000
From:   Rasmus Villemoes <rasmus.villemoes@...vas.dk>
To:     Christophe Leroy <christophe.leroy@....fr>,
        Qiang Zhao <qiang.zhao@....com>, Li Yang <leoyang.li@....com>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Scott Wood <oss@...error.net>,
        Rasmus Villemoes <Rasmus.Villemoes@...vas.se>,
        Rob Herring <robh+dt@...nel.org>,
        "linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH 2/5] soc/fsl/qe: qe.c: reduce static memory footprint by
 1.7K

On 30/04/2019 19.12, Christophe Leroy wrote:
>  
> Le 30/04/2019 à 15:36, Rasmus Villemoes a écrit :
>> The current array of struct qe_snum use 256*4 bytes for just keeping
>> track of the free/used state of each index, and the struct layout
>> means there's another 768 bytes of padding. If we just unzip that
>> structure, the array of snum values just use 256 bytes, while the
>> free/inuse state can be tracked in a 32 byte bitmap.
>>
>> So this reduces the .data footprint by 1760 bytes. It also serves as
>> preparation for introducing another DT binding for specifying the snum
>> values.
>>
>> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@...vas.dk>
>> ---
>> -
>>   /* We allocate this here because it is used almost exclusively for
>>    * the communication processor devices.
>>    */
>>   struct qe_immap __iomem *qe_immr;
>>   EXPORT_SYMBOL(qe_immr);
>>   -static struct qe_snum snums[QE_NUM_OF_SNUM];    /* Dynamically
>> allocated SNUMs */
>> +static u8 snums[QE_NUM_OF_SNUM];    /* Dynamically allocated SNUMs */
>> +static DECLARE_BITMAP(snum_state, QE_NUM_OF_SNUM);
>>   static unsigned int qe_num_of_snum;
>>     static phys_addr_t qebase = -1;
>> @@ -308,6 +298,7 @@ static void qe_snums_init(void)
>>       };
>>       const u8 *snum_init;
>>   +    bitmap_zero(snum_state, QE_NUM_OF_SNUM);
> 
> Doesn't make much importance, but wouldn't it be more logical to add
> this line where the setting of .state = QE_SNUM_STATE_FREE was done
> previously, ie around the for() loop below ?

This was on purpose, to avoid having to move it up in patch 4, where we
don't necessarily reach the for loop.

>>       qe_num_of_snum = qe_get_num_of_snums();
>>         if (qe_num_of_snum == 76)
>> @@ -315,10 +306,8 @@ static void qe_snums_init(void)
>>       else
>>           snum_init = snum_init_46;
>>   -    for (i = 0; i < qe_num_of_snum; i++) {
>> -        snums[i].num = snum_init[i];
>> -        snums[i].state = QE_SNUM_STATE_FREE;
>> -    }
>> +    for (i = 0; i < qe_num_of_snum; i++)
>> +        snums[i] = snum_init[i];
> 
> Could use memcpy() instead ?

Yes, I switch to that in 5/5. Sure, I could do it here already, but I
did it this way to keep close to the current style. I don't care either
way, so if you prefer introducing memcpy here, fine by me.


>>       spin_unlock_irqrestore(&qe_lock, flags);
>>   @@ -346,8 +333,8 @@ void qe_put_snum(u8 snum)
>>       int i;
>>         for (i = 0; i < qe_num_of_snum; i++) {
>> -        if (snums[i].num == snum) {
>> -            snums[i].state = QE_SNUM_STATE_FREE;
>> +        if (snums[i] == snum) {
>> +            clear_bit(i, snum_state);
>>               break;
>>           }
>>       }
> 
> Can we replace this loop by memchr() ?

Hm, yes. So that would be

  const u8 *p = memchr(snums, snum, qe_num_of_snum)
  if (p)
    clear_bit(p - snums, snum_state);

I guess. Let me fold that in and see how it looks.

Thanks,
Rasmus