Re: [PATCH] scsi: virtio_scsi: Remove unnecessary condition checks

Re: [PATCH] scsi: virtio_scsi: Remove unnecessary condition checks

[Markus Elfring]

> kmem_cache_destroy and mempool_destroy can correctly handle
> null pointer parameter, so there is no need to check if the
> parameter is null before calling kmem_cache_destroy and
> mempool_destroy.

Can another imperative wording be preferred for the change description?


…
> +++ b/drivers/scsi/virtio_scsi.c
> @@ -1003,14 +1003,10 @@ static int __init init(void)
>  	return 0;
>
>  error:

Can such a label be questionable?


…
> +	mempool_destroy(virtscsi_cmd_pool);
> +	virtscsi_cmd_pool = NULL;
> +	kmem_cache_destroy(virtscsi_cmd_cache);
> +	virtscsi_cmd_cache = NULL;
>  	return ret;
>  }

How do you think about to add a jump target so that the execution
of a few statements can be avoided according to a previous
null pointer check?

Regards,
Markus

Re: [PATCH] scsi: virtio_scsi: Remove unnecessary condition checks

[Paolo Bonzini]

On 09/07/20 19:16, Markus Elfring wrote:
>> +	mempool_destroy(virtscsi_cmd_pool);
>> +	virtscsi_cmd_pool = NULL;
>> +	kmem_cache_destroy(virtscsi_cmd_cache);
>> +	virtscsi_cmd_cache = NULL;
>>  	return ret;
>>  }
> 
> How do you think about to add a jump target so that the execution
> of a few statements can be avoided according to a previous
> null pointer check?

The point of the patch is precisely to simplify the code, executing a
couple more instruction is not an issue.

Paolo

Re: [PATCH] scsi: virtio_scsi: Remove unnecessary condition checks

[Markus Elfring]

>>> +	mempool_destroy(virtscsi_cmd_pool);
>>> +	virtscsi_cmd_pool = NULL;
>>> +	kmem_cache_destroy(virtscsi_cmd_cache);
>>> +	virtscsi_cmd_cache = NULL;
>>>  	return ret;
>>>  }
>>
>> How do you think about to add a jump target so that the execution
>> of a few statements can be avoided according to a previous
>> null pointer check?
>
> The point of the patch is precisely to simplify the code,

I propose another bit of fine-tuning there.


> executing a couple more instruction is not an issue.

Can an additional label help here besides a possible identifier renaming?

Regards,
Markus

Re: [PATCH] scsi: virtio_scsi: Remove unnecessary condition checks

[Markus Elfring]

>>> +	mempool_destroy(virtscsi_cmd_pool);
>>> +	virtscsi_cmd_pool = NULL;
>>> +	kmem_cache_destroy(virtscsi_cmd_cache);
>>> +	virtscsi_cmd_cache = NULL;
>>>  	return ret;
>>>  }
>>
>> How do you think about to add a jump target so that the execution
>> of a few statements can be avoided according to a previous
>> null pointer check?
>
> The point of the patch is precisely to simplify the code,

I suggest to reconsider also Linux coding style aspects
for the implementation of the function “init”.
https://elixir.bootlin.com/linux/v5.8-rc4/source/drivers/scsi/virtio_scsi.c#L980
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/scsi/virtio_scsi.c?id=42f82040ee66db13525dc6f14b8559890b2f4c1c#n980

 	if (!virtscsi_cmd_cache) {
 		pr_err("kmem_cache_create() for virtscsi_cmd_cache failed\n");
-		goto error;
+		return -ENOMEM;
	}


See also:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=42f82040ee66db13525dc6f14b8559890b2f4c1c#n461


> executing a couple more instruction is not an issue.

With which update steps would like to achieve such a code variant?

destroy_pool:
	mempool_destroy(virtscsi_cmd_pool);
	virtscsi_cmd_pool = NULL;
destroy_cache:
	kmem_cache_destroy(virtscsi_cmd_cache);
	virtscsi_cmd_cache = NULL;
	return ret;


Regards,
Markus

Re: [PATCH] scsi: virtio_scsi: Remove unnecessary condition checks

[Paolo Bonzini]

On 10/07/20 08:32, Markus Elfring wrote:
>>>> +	mempool_destroy(virtscsi_cmd_pool);
>>>> +	virtscsi_cmd_pool = NULL;
>>>> +	kmem_cache_destroy(virtscsi_cmd_cache);
>>>> +	virtscsi_cmd_cache = NULL;
>>>>  	return ret;
>>>>  }
>>>
>>> How do you think about to add a jump target so that the execution
>>> of a few statements can be avoided according to a previous
>>> null pointer check?
>>
>> The point of the patch is precisely to simplify the code,
> 
> I suggest to reconsider also Linux coding style aspects
> for the implementation of the function “init”.
> https://elixir.bootlin.com/linux/v5.8-rc4/source/drivers/scsi/virtio_scsi.c#L980
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/scsi/virtio_scsi.c?idBf82040ee66db13525dc6f14b8559890b2f4c1c#n980
> 
>  	if (!virtscsi_cmd_cache) {
>  		pr_err("kmem_cache_create() for virtscsi_cmd_cache failed\n");
> -		goto error;
> +		return -ENOMEM;
> 	}

Could be doable, but I don't see a particular benefit.  Having a single
error loop is an advantage by itself.

The coding style is a suggestion.  Note the difference between

		kfree(foo->bar);
		kfree(foo);

and

		kfree(bar);
		kfree(foo);

> See also:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?idBf82040ee66db13525dc6f14b8559890b2f4c1c#n461
> 
> 
>> executing a couple more instruction is not an issue.
> 
> With which update steps would like to achieve such a code variant?
> 
> destroy_pool:
> 	mempool_destroy(virtscsi_cmd_pool);
> 	virtscsi_cmd_pool = NULL;
> destroy_cache:
> 	kmem_cache_destroy(virtscsi_cmd_cache);
> 	virtscsi_cmd_cache = NULL;
> 	return ret;

... while there's no advantage in this.

Paolo

Re: [PATCH] scsi: virtio_scsi: Remove unnecessary condition checks

[Markus Elfring]

>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/scsi/virtio_scsi.c?id=42f82040ee66db13525dc6f14b8559890b2f4c1c#n980
>>
>>  	if (!virtscsi_cmd_cache) {
>>  		pr_err("kmem_cache_create() for virtscsi_cmd_cache failed\n");
>> -		goto error;
>> +		return -ENOMEM;
>>  	}
>
> Could be doable, but I don't see a particular benefit.

Can a bit more “compliance” (with the Linux coding style) matter here?


> Having a single error loop is an advantage by itself.

I do not see that a loop is involved in the implementation of the function “init”.


>> destroy_pool:
>> 	mempool_destroy(virtscsi_cmd_pool);
>> 	virtscsi_cmd_pool = NULL;
>> destroy_cache:
>> 	kmem_cache_destroy(virtscsi_cmd_cache);
>> 	virtscsi_cmd_cache = NULL;
>> 	return ret;
>
> ... while there's no advantage in this.

I propose again to improve the affected exception handling another bit
by using appropriate labels.
Will further software improvements be achieved by a corresponding patch series?

Regards,
Markus

Re: [PATCH] scsi: virtio_scsi: Remove unnecessary condition checks

[Paolo Bonzini]

On 10/07/20 09:40, Markus Elfring wrote:
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/scsi/virtio_scsi.c?idBf82040ee66db13525dc6f14b8559890b2f4c1c#n980
>>>
>>>  	if (!virtscsi_cmd_cache) {
>>>  		pr_err("kmem_cache_create() for virtscsi_cmd_cache failed\n");
>>> -		goto error;
>>> +		return -ENOMEM;
>>>  	}
>>
>> Could be doable, but I don't see a particular benefit.
> 
> Can a bit more “compliance” (with the Linux coding style) matter here?

No.

>> Having a single error loop is an advantage by itself.
> 
> I do not see that a loop is involved in the implementation of the function “init”.

s/loop/label/ sorry.

Paolo

Re: scsi: virtio_scsi: Remove unnecessary condition checks

[Markus Elfring]

>> Can a bit more “compliance” (with the Linux coding style) matter here?
>
> No.

Please take another look at the corresponding software documentation.


>>> Having a single error loop is an advantage by itself.
>>
>> I do not see that a loop is involved in the implementation of the function “init”.
>
> s/loop/label/ sorry.

Thanks for this information.

Can the development attention grow for corresponding implementation details
around the specifcation of efficient exception handling?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=42f82040ee66db13525dc6f14b8559890b2f4c1c#n465

Regards,
Markus