Re: [PATCH 4/4] iommu/arm-smmu-v3: Remove cmpxchg() in arm_smmu_cmdq_issue_cmdlist()

John Garry
2020-06-24T08:16:50Z

>>
>> I'd say that GENMASK_INPUT_CHECK() should be able to handle a l=0 and
>> h=unsigned value, so I doubt this warn.
>>
>> Using GENMASK((int)cmdq->q.llq.max_n_shift, 0) resolves it, but it looks
>> like GENMASK_INPUT_CHECK() could be improved.
>
> That said, I think this particular case might be even better off dodging
> GENMASK() entirely, by doing something like this first. Untested...
>
> Robin.
>
> ----->8-----
> Subject: [PATCH] iommu/arm-smmu-v3: Streamline queue calculations
>
> Beyond the initial queue setup based on the log2 values from ID
> registers, the log2 queue size is only ever used in the form of
> (1 << max_n_shift) to repeatedly recalculate the number of queue
> elements. Simply storing it in that form leads to slightly more
> efficient code, particularly in the low-level queue accessors
> where it counts most:
>
> add/remove: 0/0 grow/shrink: 1/7 up/down: 4/-120 (-116)
> Function old new delta
> arm_smmu_init_one_queue 360 364 +4
> arm_smmu_priq_thread 512 508 -4
> arm_smmu_evtq_thread 300 292 -8
> __arm_smmu_cmdq_poll_set_valid_map.isra 296 288 -8
> queue_remove_raw 180 164 -16
> arm_smmu_gerror_handler 732 716 -16
> arm_smmu_device_probe 4312 4284 -28
> arm_smmu_cmdq_issue_cmdlist 1892 1852 -40
> Total: Before=20135, After=20019, chg -0.58%
>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---

[...]

> }
>
> - smmu->evtq.q.llq.max_n_shift = min_t(u32, EVTQ_MAX_SZ_SHIFT,
> - FIELD_GET(IDR1_EVTQS, reg));
> - smmu->priq.q.llq.max_n_shift = min_t(u32, PRIQ_MAX_SZ_SHIFT,
> - FIELD_GET(IDR1_PRIQS, reg));
> + max_n_shift = min_t(u32, EVTQ_MAX_SZ_SHIFT, FIELD_GET(IDR1_EVTQS, reg));
> + smmu->evtq.q.llq.max_n = 1 << max_n_shift;

So I require the bitmask of this for the prod, which would be (max_n <<
1) - 1.

I don't feel too strongly either way, and the other big changes in this
series need to be considered first...

Thanks,
John

> +
> + max_n_shift = min_t(u32, PRIQ_MAX_SZ_SHIFT, FIELD_GET(IDR1_PRIQS, reg));
> + smmu->priq.q.llq.max_n = 1 << max_n_shift;
>
> /* SID/SSID sizes */
> smmu->ssid_bits = FIELD_GET(IDR1_SSIDSIZE, reg);
>