Re: [PATCH 5/6] ide: remove ide_execute_pkt_cmd()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hello.

Borislav Petkov wrote:

or similar instead of wasting stack space?

 It doesn't necessarily waste stack space. Haven't you heard about
compiler
putting local vairables into registers?

Yes, have you heard of unnecessary register spilling?

 No -- only about stack spilling on CPUs "caching" the top of stack in their
register file (like SPARC).
 Linux runs not only on x86 and many RISCs can store several local variables
in the dedicated registers -- it's the part of say MIPS ABIs...

Oh, it's really the same on x86, only there are only 3 registers dedicated to that. RISCs typically have more, however x86_64 wiuth 16 its registers is close to that already. However, you're right -- these registers need saving at the function entry, so they effectively take the stack space.

It'll also read better in the if() check:

if (drv_can_irq_interrupt(drive)) { ...


 It's faster to checj a local variable than to dereference drv several
times
-- unless gcc optimizes that away (by creating an implicit local variable
:-).


Let's look at an example:

In ide-cd.c:cdrom_newpc_intr() you have the following code snippet:

<ide-cd.c>
 799         thislen = blk_fs_request(rq) ? len : rq->data_len;
 800         if (thislen > len)
 801                 thislen = len;
 802
 803         ide_debug_log(IDE_DBG_PC, "%s: DRQ: stat: 0x%x, thislen: %d\n",
 804                       __func__, stat, thislen);
 805
 806         /* If DRQ is clear, the command has completed. */
 807         if ((stat & ATA_DRQ) == 0) {
 808                 if (blk_fs_request(rq)) {
</ide-cd.c>

Now watch the blk_fs_request() thing.

Here's what my gcc¹ spits out:

  Thsi code is somewhat confused. Also, I was of a better opinion of gcc...

<ide-cd.s>
.LVL174:
        .loc 1 799 0
        movl    76(%r12), %ecx  # <variable>.cmd_type, prephitmp.1128
        cmpl    $1, %ecx        #, prephitmp.1128
        movl    %ecx, %r8d      # prephitmp.1128, prephitmp.1047
        je      .L225   #,

 Now where is that label?

.LVL175:
        .loc 1 800 0
        movzwl  -44(%rbp), %r15d        # len, thislen

Oh, that AT&T syntax... it took me a while to realize that it's a movzx insn. :-)

.LVL176:
        .loc 1 799 0
        movl    280(%r12), %edx # <variable>.data_len, thislen.1129
.LVL177:
        .loc 1 800 0
        cmpl    %r15d, %edx     # thislen, thislen.1129
        movl    %r15d, %ebx     # thislen, thislen.1163
        jg      .L145   #,
.LVL178:
        movl    %edx, %r15d     # thislen.1129, thislen

  I wonder why it doesn't generate cmovng ISO jg and mov...

.LVL179:
.L145:
        .loc 1 807 0
        testb   $8, -64(%rbp)   #, stat
        jne     .L147   #,
.LVL180:
        .loc 1 808 0
        cmpl    $1, %ecx        #, prephitmp.1128
        je      .L226   #,
        .loc 1 825 0
        cmpl    $2, %ecx        #, prephitmp.1128
        .p2align 4,,3
        .p2align 3
        je      .L152   #,
.LBB408:
</ide-cd.s>
and at label .LVL174 you see the blk_fs_request() check from line
799 above. Later, at label .LVL180 you see the next blk_fs_request() check from
line 808 and this is cached in %ecx so gcc is smart enough to do that. So,

  Yes, CSE optimization does work...

actually you get the same thing/or even better with variables in registers
instead of on stack

I still don't undestand why you assume that such variable will be alloceted on stack -- gcc has 3 registers available for local variables (which doesn't have to save across function calls). However, the register variables have to take stack space indeed as they need to be saved on funciton entry... though I'm not sure that gcc will necessary put such variable in one of those 3 registers if it figures out that there are no function calls going to happen during its life time.

and the code is more readable. A win-win situation, I'd say :).

You haven't presented the code which gets generated when the local variable is used, so it's impossible to compare.

MBR, Sergei


--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux Filesystems]     [Linux SCSI]     [Linux RAID]     [Git]     [Kernel Newbies]     [Linux Newbie]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Samba]     [Device Mapper]

  Powered by Linux