diff mbox series

net/virtio: fix wrong variable assignment in helper macro

Message ID 20200814092138.1692-1-vipul.ashri@oracle.com (mailing list archive)
State Superseded, archived
Delegated to: Maxime Coquelin
Headers
Series net/virtio: fix wrong variable assignment in helper macro |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS

Commit Message

Vipul Ashri Aug. 14, 2020, 9:21 a.m. UTC 
Inside Macro ASSIGN_UNLESS_EQUAL(var, val), assignment to var is always
failing as assignment done using var_ having local scope only.
This leads to TX packets not going out and found broken due to cleanup
malfunctioning. This patch fixes the wrong variable assignment.

Fixes: 57f90f894588 ("net/virtio: reuse packed ring functions")
Cc: stable@dpdk.org

Signed-off-by: Vipul Ashri <vipul.ashri@oracle.com>
---
 drivers/net/virtio/virtqueue.h | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Comments

Andrew Rybchenko Aug. 29, 2020, 11:22 a.m. UTC | #1 
On 8/14/20 12:21 PM, Vipul Ashri wrote:
> Inside Macro ASSIGN_UNLESS_EQUAL(var, val), assignment to var is always
> failing as assignment done using var_ having local scope only.
> This leads to TX packets not going out and found broken due to cleanup
> malfunctioning. This patch fixes the wrong variable assignment.
>
> Fixes: 57f90f894588 ("net/virtio: reuse packed ring functions")
> Cc: stable@dpdk.org
>
> Signed-off-by: Vipul Ashri <vipul.ashri@oracle.com>
> ---
>  drivers/net/virtio/virtqueue.h | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
> index 105a9c00c..20c95471e 100644
> --- a/drivers/net/virtio/virtqueue.h
> +++ b/drivers/net/virtio/virtqueue.h
> @@ -607,10 +607,8 @@ virtqueue_notify(struct virtqueue *vq)
>  
>  /* avoid write operation when necessary, to lessen cache issues */
>  #define ASSIGN_UNLESS_EQUAL(var, val) do {	\
> -	typeof(var) var_ = (var);		\
> -	typeof(val) val_ = (val);		\
> -	if ((var_) != (val_))			\
> -		(var_) = (val_);		\
> +	if ((var) != (val))			\
> +		(var) = (val);			\

Good catch. As I understand the old code tries to avoid
processing of var and val expressions twice. It looks it
could be kept for val at least. Just keep if condition as in
old code and fix the last line above:
    (var) = val_;
Since var_ and val_are local variables there is no necessity
to enclose it in parenthesis (but does not harm if done).
var_ may be really removed since since resulting code will
use it only once.
Edward Makarov Aug. 29, 2020, 10:17 p.m. UTC | #2 
On 8/29/20 2:22 PM, Andrew Rybchenko wrote:
> On 8/14/20 12:21 PM, Vipul Ashri wrote:
>> Inside Macro ASSIGN_UNLESS_EQUAL(var, val), assignment to var is always
>> failing as assignment done using var_ having local scope only.
>> This leads to TX packets not going out and found broken due to cleanup
>> malfunctioning. This patch fixes the wrong variable assignment.
>>
>> Fixes: 57f90f894588 ("net/virtio: reuse packed ring functions")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Vipul Ashri <vipul.ashri@oracle.com>
>> ---
>>   drivers/net/virtio/virtqueue.h | 6 ++----
>>   1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
>> index 105a9c00c..20c95471e 100644
>> --- a/drivers/net/virtio/virtqueue.h
>> +++ b/drivers/net/virtio/virtqueue.h
>> @@ -607,10 +607,8 @@ virtqueue_notify(struct virtqueue *vq)
>>   
>>   /* avoid write operation when necessary, to lessen cache issues */
>>   #define ASSIGN_UNLESS_EQUAL(var, val) do {	\
>> -	typeof(var) var_ = (var);		\
>> -	typeof(val) val_ = (val);		\
>> -	if ((var_) != (val_))			\
>> -		(var_) = (val_);		\
>> +	if ((var) != (val))			\
>> +		(var) = (val);			\
> 
> Good catch. As I understand the old code tries to avoid
> processing of var and val expressions twice. It looks it
> could be kept for val at least. Just keep if condition as in
> old code and fix the last line above:
>      (var) = val_;
> Since var_ and val_are local variables there is no necessity
> to enclose it in parenthesis (but does not harm if done).
> var_ may be really removed since since resulting code will
> use it only once.

I think there is a solution to avoid multiple evaluations of parameters:

// var is definitely an lvalue, so its address can definitely be taken
#define ASSIGN_UNLESS_EQUAL(var, val) do {  \
	typeof(var) *const var_ = &(var);   \
	typeof(val)  const val_ = (val);    \
	if (*var_ != val_)                  \
		*var_ = val_;               \
} while (0)

The solution relies on the compiler optimizer. Here is the comparison of
what the variants are compiled into:

https://godbolt.org/z/nnvq5q
Vipul Ashri Sept. 18, 2020, 10:41 a.m. UTC | #3 
Hi Andrew

I like your suggestion and applied with [v5] net/virtio: fix wrong variable assignment in helper macro
V4 had a extra line typos., V5 is tested compiled and pushed carefully
Thanks!

Regards
Vipul


-----Original Message-----
From: Andrew Rybchenko [mailto:arybchenko@solarflare.com] 
Sent: Saturday, 29 August, 2020 16:53
To: Vipul Ashri <vipul.ashri@oracle.com>; dev@dpdk.org
Cc: chenbo.xia@intel.com; zhihong.wang@intel.com; maxime.coquelin@redhat.com; stable@dpdk.org
Subject: Re: [dpdk-dev] [PATCH] net/virtio: fix wrong variable assignment in helper macro

On 8/14/20 12:21 PM, Vipul Ashri wrote:
> Inside Macro ASSIGN_UNLESS_EQUAL(var, val), assignment to var is 
> always failing as assignment done using var_ having local scope only.
> This leads to TX packets not going out and found broken due to cleanup 
> malfunctioning. This patch fixes the wrong variable assignment.
>
> Fixes: 57f90f894588 ("net/virtio: reuse packed ring functions")
> Cc: stable@dpdk.org
>
> Signed-off-by: Vipul Ashri <vipul.ashri@oracle.com>
> ---
>  drivers/net/virtio/virtqueue.h | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/virtio/virtqueue.h 
> b/drivers/net/virtio/virtqueue.h index 105a9c00c..20c95471e 100644
> --- a/drivers/net/virtio/virtqueue.h
> +++ b/drivers/net/virtio/virtqueue.h
> @@ -607,10 +607,8 @@ virtqueue_notify(struct virtqueue *vq)
>  
>  /* avoid write operation when necessary, to lessen cache issues */
>  #define ASSIGN_UNLESS_EQUAL(var, val) do {	\
> -	typeof(var) var_ = (var);		\
> -	typeof(val) val_ = (val);		\
> -	if ((var_) != (val_))			\
> -		(var_) = (val_);		\
> +	if ((var) != (val))			\
> +		(var) = (val);			\

Good catch. As I understand the old code tries to avoid processing of var and val expressions twice. It looks it could be kept for val at least. Just keep if condition as in old code and fix the last line above:
    (var) = val_;
Since var_ and val_are local variables there is no necessity to enclose it in parenthesis (but does not harm if done).
var_ may be really removed since since resulting code will use it only once.
Vipul Ashri Sept. 18, 2020, 10:46 a.m. UTC | #4 
Hi Edward / Andrew

I like your suggestions and applied with [v5] net/virtio: fix wrong variable assignment in helper macro
V4 had a extra line typos., V5 is tested compiled and pushed carefully Thanks!

Regards
Vipul

-----Original Message-----
From: Edward Makarov [mailto:makarov@kraftway.ru] 
Sent: Sunday, 30 August, 2020 3:48
To: Andrew Rybchenko <arybchenko@solarflare.com>; Vipul Ashri <vipul.ashri@oracle.com>; dev@dpdk.org
Cc: chenbo.xia@intel.com; zhihong.wang@intel.com; maxime.coquelin@redhat.com; stable@dpdk.org
Subject: Re: [dpdk-dev] [PATCH] net/virtio: fix wrong variable assignment in helper macro


On 8/29/20 2:22 PM, Andrew Rybchenko wrote:
> On 8/14/20 12:21 PM, Vipul Ashri wrote:
>> Inside Macro ASSIGN_UNLESS_EQUAL(var, val), assignment to var is 
>> always failing as assignment done using var_ having local scope only.
>> This leads to TX packets not going out and found broken due to 
>> cleanup malfunctioning. This patch fixes the wrong variable assignment.
>>
>> Fixes: 57f90f894588 ("net/virtio: reuse packed ring functions")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Vipul Ashri <vipul.ashri@oracle.com>
>> ---
>>   drivers/net/virtio/virtqueue.h | 6 ++----
>>   1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/virtio/virtqueue.h 
>> b/drivers/net/virtio/virtqueue.h index 105a9c00c..20c95471e 100644
>> --- a/drivers/net/virtio/virtqueue.h
>> +++ b/drivers/net/virtio/virtqueue.h
>> @@ -607,10 +607,8 @@ virtqueue_notify(struct virtqueue *vq)
>>   
>>   /* avoid write operation when necessary, to lessen cache issues */
>>   #define ASSIGN_UNLESS_EQUAL(var, val) do {	\
>> -	typeof(var) var_ = (var);		\
>> -	typeof(val) val_ = (val);		\
>> -	if ((var_) != (val_))			\
>> -		(var_) = (val_);		\
>> +	if ((var) != (val))			\
>> +		(var) = (val);			\
> 
> Good catch. As I understand the old code tries to avoid processing of 
> var and val expressions twice. It looks it could be kept for val at 
> least. Just keep if condition as in old code and fix the last line 
> above:
>      (var) = val_;
> Since var_ and val_are local variables there is no necessity to 
> enclose it in parenthesis (but does not harm if done).
> var_ may be really removed since since resulting code will use it only 
> once.

I think there is a solution to avoid multiple evaluations of parameters:

// var is definitely an lvalue, so its address can definitely be taken #define ASSIGN_UNLESS_EQUAL(var, val) do {  \
	typeof(var) *const var_ = &(var);   \
	typeof(val)  const val_ = (val);    \
	if (*var_ != val_)                  \
		*var_ = val_;               \
} while (0)

The solution relies on the compiler optimizer. Here is the comparison of what the variants are compiled into:

https://urldefense.com/v3/__https://godbolt.org/z/nnvq5q__;!!GqivPVa7Brio!MJZCNAgcCHB5A_T1mI2aA-2F9wvqh_WOfzkeN0IbDnsZSlNIPWqDF0b4YDmTc19HcA$
Maxime Coquelin Sept. 18, 2020, 10:53 a.m. UTC | #5 
On 9/18/20 12:46 PM, Vipul Ashri wrote:
> Hi Edward / Andrew
> 
> I like your suggestions and applied with [v5] net/virtio: fix wrong variable assignment in helper macro
> V4 had a extra line typos., V5 is tested compiled and pushed carefully Thanks!

Thanks Vipul, I plan to pick your patch later today.

So I will assume Andrew still ack the patch since v5 is only fixing
build.

Thanks,
Maxime

> Regards
> Vipul
> 
> -----Original Message-----
> From: Edward Makarov [mailto:makarov@kraftway.ru] 
> Sent: Sunday, 30 August, 2020 3:48
> To: Andrew Rybchenko <arybchenko@solarflare.com>; Vipul Ashri <vipul.ashri@oracle.com>; dev@dpdk.org
> Cc: chenbo.xia@intel.com; zhihong.wang@intel.com; maxime.coquelin@redhat.com; stable@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] net/virtio: fix wrong variable assignment in helper macro
> 
> 
> On 8/29/20 2:22 PM, Andrew Rybchenko wrote:
>> On 8/14/20 12:21 PM, Vipul Ashri wrote:
>>> Inside Macro ASSIGN_UNLESS_EQUAL(var, val), assignment to var is 
>>> always failing as assignment done using var_ having local scope only.
>>> This leads to TX packets not going out and found broken due to 
>>> cleanup malfunctioning. This patch fixes the wrong variable assignment.
>>>
>>> Fixes: 57f90f894588 ("net/virtio: reuse packed ring functions")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Vipul Ashri <vipul.ashri@oracle.com>
>>> ---
>>>   drivers/net/virtio/virtqueue.h | 6 ++----
>>>   1 file changed, 2 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/net/virtio/virtqueue.h 
>>> b/drivers/net/virtio/virtqueue.h index 105a9c00c..20c95471e 100644
>>> --- a/drivers/net/virtio/virtqueue.h
>>> +++ b/drivers/net/virtio/virtqueue.h
>>> @@ -607,10 +607,8 @@ virtqueue_notify(struct virtqueue *vq)
>>>   
>>>   /* avoid write operation when necessary, to lessen cache issues */
>>>   #define ASSIGN_UNLESS_EQUAL(var, val) do {	\
>>> -	typeof(var) var_ = (var);		\
>>> -	typeof(val) val_ = (val);		\
>>> -	if ((var_) != (val_))			\
>>> -		(var_) = (val_);		\
>>> +	if ((var) != (val))			\
>>> +		(var) = (val);			\
>>
>> Good catch. As I understand the old code tries to avoid processing of 
>> var and val expressions twice. It looks it could be kept for val at 
>> least. Just keep if condition as in old code and fix the last line 
>> above:
>>      (var) = val_;
>> Since var_ and val_are local variables there is no necessity to 
>> enclose it in parenthesis (but does not harm if done).
>> var_ may be really removed since since resulting code will use it only 
>> once.
> 
> I think there is a solution to avoid multiple evaluations of parameters:
> 
> // var is definitely an lvalue, so its address can definitely be taken #define ASSIGN_UNLESS_EQUAL(var, val) do {  \
> 	typeof(var) *const var_ = &(var);   \
> 	typeof(val)  const val_ = (val);    \
> 	if (*var_ != val_)                  \
> 		*var_ = val_;               \
> } while (0)
> 
> The solution relies on the compiler optimizer. Here is the comparison of what the variants are compiled into:
> 
> https://urldefense.com/v3/__https://godbolt.org/z/nnvq5q__;!!GqivPVa7Brio!MJZCNAgcCHB5A_T1mI2aA-2F9wvqh_WOfzkeN0IbDnsZSlNIPWqDF0b4YDmTc19HcA$
>
diff mbox series

Patch

diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
index 105a9c00c..20c95471e 100644
--- a/drivers/net/virtio/virtqueue.h
+++ b/drivers/net/virtio/virtqueue.h
@@ -607,10 +607,8 @@  virtqueue_notify(struct virtqueue *vq)
 
 /* avoid write operation when necessary, to lessen cache issues */
 #define ASSIGN_UNLESS_EQUAL(var, val) do {	\
-	typeof(var) var_ = (var);		\
-	typeof(val) val_ = (val);		\
-	if ((var_) != (val_))			\
-		(var_) = (val_);		\
+	if ((var) != (val))			\
+		(var) = (val);			\
 } while (0)
 
 #define virtqueue_clear_net_hdr(hdr) do {		\