DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] net/virtio: fix wrong variable assignment in helper macro
@ 2020-08-14  9:21 Vipul Ashri
  2020-08-29 11:22 ` Andrew Rybchenko
  0 siblings, 1 reply; 7+ messages in thread
From: Vipul Ashri @ 2020-08-14  9:21 UTC (permalink / raw)
  To: dev; +Cc: chenbo.xia, vipul.ashri, zhihong.wang, maxime.coquelin, stable

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);			\
 } while (0)
 
 #define virtqueue_clear_net_hdr(hdr) do {		\
-- 
2.28.0.windows.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH] net/virtio: fix wrong variable assignment in helper macro
  2020-08-14  9:21 [dpdk-dev] [PATCH] net/virtio: fix wrong variable assignment in helper macro Vipul Ashri
@ 2020-08-29 11:22 ` Andrew Rybchenko
  2020-08-29 22:17   ` Edward Makarov
  2020-09-18 10:41   ` Vipul Ashri
  0 siblings, 2 replies; 7+ messages in thread
From: Andrew Rybchenko @ 2020-08-29 11:22 UTC (permalink / raw)
  To: Vipul Ashri, dev; +Cc: chenbo.xia, zhihong.wang, maxime.coquelin, stable

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.


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH] net/virtio: fix wrong variable assignment in helper macro
  2020-08-29 11:22 ` Andrew Rybchenko
@ 2020-08-29 22:17   ` Edward Makarov
  2020-09-18 10:46     ` Vipul Ashri
  2020-09-18 10:41   ` Vipul Ashri
  1 sibling, 1 reply; 7+ messages in thread
From: Edward Makarov @ 2020-08-29 22:17 UTC (permalink / raw)
  To: Andrew Rybchenko, Vipul Ashri, dev
  Cc: chenbo.xia, zhihong.wang, maxime.coquelin, stable


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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH] net/virtio: fix wrong variable assignment in helper macro
  2020-08-29 11:22 ` Andrew Rybchenko
  2020-08-29 22:17   ` Edward Makarov
@ 2020-09-18 10:41   ` Vipul Ashri
  1 sibling, 0 replies; 7+ messages in thread
From: Vipul Ashri @ 2020-09-18 10:41 UTC (permalink / raw)
  To: Andrew Rybchenko, dev; +Cc: chenbo.xia, zhihong.wang, maxime.coquelin, stable

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.


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH] net/virtio: fix wrong variable assignment in helper macro
  2020-08-29 22:17   ` Edward Makarov
@ 2020-09-18 10:46     ` Vipul Ashri
  2020-09-18 10:53       ` Maxime Coquelin
  0 siblings, 1 reply; 7+ messages in thread
From: Vipul Ashri @ 2020-09-18 10:46 UTC (permalink / raw)
  To: Edward Makarov, Andrew Rybchenko, dev
  Cc: chenbo.xia, zhihong.wang, maxime.coquelin, stable

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$

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH] net/virtio: fix wrong variable assignment in helper macro
  2020-09-18 10:46     ` Vipul Ashri
@ 2020-09-18 10:53       ` Maxime Coquelin
  0 siblings, 0 replies; 7+ messages in thread
From: Maxime Coquelin @ 2020-09-18 10:53 UTC (permalink / raw)
  To: Vipul Ashri, Edward Makarov, Andrew Rybchenko, dev
  Cc: chenbo.xia, zhihong.wang, stable



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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [dpdk-dev] [PATCH] net/virtio: fix wrong variable assignment in helper macro
@ 2020-08-17  5:16 Vipul Ashri
  0 siblings, 0 replies; 7+ messages in thread
From: Vipul Ashri @ 2020-08-17  5:16 UTC (permalink / raw)
  To: dev; +Cc: stable

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);			\
 } while (0)
 
 #define virtqueue_clear_net_hdr(hdr) do {		\
-- 
2.28.0.windows.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-09-18 10:53 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-14  9:21 [dpdk-dev] [PATCH] net/virtio: fix wrong variable assignment in helper macro Vipul Ashri
2020-08-29 11:22 ` Andrew Rybchenko
2020-08-29 22:17   ` Edward Makarov
2020-09-18 10:46     ` Vipul Ashri
2020-09-18 10:53       ` Maxime Coquelin
2020-09-18 10:41   ` Vipul Ashri
2020-08-17  5:16 Vipul Ashri

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).