Re: [PATCH] iommu/arm-smmu-v3: Optimize checking for invalid values

[Robin Murphy]

On 2022-11-13 13:38, Harry Song wrote:
> Move the check of invalid value (iotlb_gather->pgsize = 0) from
> arm_smmu_iotlb_sync() to __arm_smmu_tlb_inv_range() for iotlb sync
> to make the code clearer.

How is this an optimisation? It adds a redundant check on paths that 
don't need it, and even the unmap error paths now have to do *more* work 
to figure out when there's nothing to sync. Furthermore, to me "clear 
code" means making decisions at the appropriate level of abstraction, 
not deliberately passing invalid arguments down through 3 more levels of 
callstack to eventually do nothing.

If you think that the gather->pgsize logic is hard to follow and needs 
more explanation then by all means add a comment to call out how 
iommu_iotlb_gather_init() and iommu_iotlb_gather_add_page() join it up, 
but I don't agree with this patch as it is, sorry.

Thanks,
Robin.

> Signed-off-by: Harry Song <jundongsong1@xxxxxxxxx>
> ---
>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 5 +----
>   1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index 6d5df91c5..e51b9f506 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -1873,7 +1873,7 @@ static void __arm_smmu_tlb_inv_range(struct arm_smmu_cmdq_ent *cmd,
>   	size_t inv_range = granule;
>   	struct arm_smmu_cmdq_batch cmds;
>   
> -	if (!size)
> +	if (!size || !inv_range)
>   		return;
>   
>   	if (smmu->features & ARM_SMMU_FEAT_RANGE_INV) {
> @@ -2507,9 +2507,6 @@ static void arm_smmu_iotlb_sync(struct iommu_domain *domain,
>   {
>   	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>   
> -	if (!gather->pgsize)
> -		return;
> -
>   	arm_smmu_tlb_inv_range_domain(gather->start,
>   				      gather->end - gather->start + 1,
>   				      gather->pgsize, true, smmu_domain);