Re: [PATCH] iommu/arm-smmu-v3: Remove some unneeded init in arm_smmu_cmdq_issue_cmdlist()

On 2021-08-05 16:16, John Garry wrote:

On 05/08/2021 15:41, Robin Murphy wrote:

quoted

quoted

I suppose they could be combined into a smaller sub-struct and loaded 
in a single operation, but it looks messy, and prob without much gain.

Indeed I wouldn't say that saving memory is the primary concern here, 
and any more convoluted code is hardly going to help performance. Plus 
it still wouldn't help the other cases where we're just copying the 
size into a fake queue to do some prod arithmetic - I hadn't fully 
clocked what was going on there when I skimmed through things earlier.

Disregarding the bogus layout change, though, do you reckon the rest 
of my idea makes sense?

I tried the similar change to avoid zero-init the padding in 
arm_smmu_cmdq_write_entries() and the 
_arm_smmu_cmdq_poll_set_valid_map(), but the disassembly was the same. 
So the compiler must have got smart there.

Yeah, in my build __arm_smmu_cmdq_poll_set_valid_map() only uses 32 
bytes of stack, so clearly it's managed to see through the macro magic 
once queue_inc_prod_n() is inlined and elide the whole struct. 
arm_smmu_cmdq_write_entries() is inlined already, but logically must be 
the same deal since it's a similarly inlined queue_inc_prod_n().

However, that may all change if different compiler flags or a different 
compiler lead to different inlining decisions, so I'd argue that if this 
can matter anywhere then it's worth treating consistently everywhere.

But for the original change in this patch, it did make a difference. 
It's nice to remove what was a memcpy:

     1770: a9077eff stp xzr, xzr, [x23, #112]
}, head = llq;
     1774: 94000000 bl 0 <memcpy>

And performance was very fractionally better.

Heh, mine was this beauty:

         struct arm_smmu_ll_queue llq = {
     17d4:       a9017f7f        stp     xzr, xzr, [x27, #16]
     17d8:       a9027f7f        stp     xzr, xzr, [x27, #32]
     17dc:       a9037f7f        stp     xzr, xzr, [x27, #48]
     17e0:       a9047f7f        stp     xzr, xzr, [x27, #64]
         }, head = llq;
     17e4:       b900c340        str     w0, [x26, #192]
{
     17e8:       290d0be1        stp     w1, w2, [sp, #104]
         }, head = llq;
     17ec:       a9440f62        ldp     x2, x3, [x27, #64]
     17f0:       a9007f5f        stp     xzr, xzr, [x26]
     17f4:       a9017f5f        stp     xzr, xzr, [x26, #16]
     17f8:       a9027f5f        stp     xzr, xzr, [x26, #32]
     17fc:       a9037f5f        stp     xzr, xzr, [x26, #48]
     1800:       a9040f42        stp     x2, x3, [x26, #64]
         struct arm_smmu_ll_queue llq = {
     1804:       a9057f7f        stp     xzr, xzr, [x27, #80]
         }, head = llq;
     1808:       a9057f5f        stp     xzr, xzr, [x26, #80]
         struct arm_smmu_ll_queue llq = {
     180c:       a9067f7f        stp     xzr, xzr, [x27, #96]
         }, head = llq;
     1810:       a9067f5f        stp     xzr, xzr, [x26, #96]
         struct arm_smmu_ll_queue llq = {
     1814:       a9077f7f        stp     xzr, xzr, [x27, #112]
         }, head = llq;
     1818:       a9077f5f        stp     xzr, xzr, [x26, #112]
         struct arm_smmu_ll_queue llq = {
     181c:       a9087f5f        stp     xzr, xzr, [x26, #128]

As for pre-evaluating "nents", I'm not sure how much that can help, but 
I am not too optimistic. I can try some testing when I get a chance. 
Having said that, I would need to check the disassembly also.

It'll just turn MOV,LDR,LSL sequences into plain LDRs - a small saving 
but with no real downside, and a third of it is in the place where doing 
less work matters most:

add/remove: 0/0 grow/shrink: 0/8 up/down: 0/-100 (-100)
Function                                     old     new   delta
arm_smmu_priq_thread                         532     528      -4
arm_smmu_evtq_thread                         368     364      -4
arm_smmu_device_probe                       4564    4556      -8
__arm_smmu_cmdq_poll_set_valid_map.isra      316     308      -8
arm_smmu_init_one_queue.isra                 320     308     -12
queue_remove_raw                             192     176     -16
arm_smmu_gerror_handler                      752     736     -16
arm_smmu_cmdq_issue_cmdlist                 1812    1780     -32
Total: Before=23776, After=23676, chg -0.42%


Robin.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel