Re: [dpdk-stable] [dpdk-dev] [PATCH] net/virtio: fix wrong variable assignment in helper macro

Re: [dpdk-stable] [dpdk-dev] [PATCH] net/virtio: fix wrong variable assignment in helper macro

[Andrew Rybchenko]

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.

Re: [dpdk-stable] [dpdk-dev] [PATCH] net/virtio: fix wrong variable assignment in helper macro

[Edward Makarov]

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

Re: [dpdk-stable] [dpdk-dev] [PATCH] net/virtio: fix wrong variable assignment in helper macro

[Vipul Ashri]

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.

Re: [dpdk-stable] [dpdk-dev] [PATCH] net/virtio: fix wrong variable assignment in helper macro

[Vipul Ashri]

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$

Re: [dpdk-stable] [dpdk-dev] [PATCH] net/virtio: fix wrong variable assignment in helper macro

[Maxime Coquelin]

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$
>