* Re: [dpdk-stable] [dpdk-dev] [PATCH] net/virtio: fix wrong variable assignment in helper macro [not found] <20200814092138.1692-1-vipul.ashri@oracle.com> @ 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; 5+ 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] 5+ messages in thread
* Re: [dpdk-stable] [dpdk-dev] [PATCH] net/virtio: fix wrong variable assignment in helper macro 2020-08-29 11:22 ` [dpdk-stable] [dpdk-dev] [PATCH] net/virtio: fix wrong variable assignment in helper macro 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; 5+ 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] 5+ messages in thread
* Re: [dpdk-stable] [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; 5+ 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] 5+ messages in thread
* Re: [dpdk-stable] [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; 5+ 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] 5+ messages in thread
* Re: [dpdk-stable] [dpdk-dev] [PATCH] net/virtio: fix wrong variable assignment in helper macro 2020-08-29 11:22 ` [dpdk-stable] [dpdk-dev] [PATCH] net/virtio: fix wrong variable assignment in helper macro Andrew Rybchenko 2020-08-29 22:17 ` Edward Makarov @ 2020-09-18 10:41 ` Vipul Ashri 1 sibling, 0 replies; 5+ 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] 5+ messages in thread
end of thread, other threads:[~2020-09-18 10:53 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20200814092138.1692-1-vipul.ashri@oracle.com>
2020-08-29 11:22 ` [dpdk-stable] [dpdk-dev] [PATCH] net/virtio: fix wrong variable assignment in helper macro 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
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).